Repository navigation
Use GCC visibility attrs in PyAPI_* #55619
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2019-10-31.14:17:46.435>
created_at = <Date 2011-03-06.06:12:25.183>
labels = ['type-feature', '3.9']
title = 'Use GCC visibility attrs in PyAPI_*'
updated_at = <Date 2019-10-31.14:17:46.434>
user = 'https://github.com/Yhg1s'bugs.python.org fields:
activity = <Date 2019-10-31.14:17:46.434>
actor = 'vinay.sajip'
assignee = 'none'
closed = True
closed_date = <Date 2019-10-31.14:17:46.435>
closer = 'vinay.sajip'
components = []
creation = <Date 2011-03-06.06:12:25.183>
creator = 'twouters'
dependencies = []
files = ['21024']
hgrepos = []
issue_num = 11410
keywords = ['patch', 'needs review']
message_count = 16.0
messages = ['130143', '130145', '130163', '130194', '130196', '130208', '130211', '238257', '238273', '238274', '238275', '238322', '238422', '301093', '353055', '354688']
nosy_count = 10.0
nosy_names = ['loewis', 'twouters', 'vinay.sajip', 'pitrou', 'vstinner', 'Arfrever', 'njs', 'dmalcolm', 'SamB', 'tromey']
pr_nums = ['16347']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue11410'
versions = ['Python 3.9']
Activity
Yhg1s commentedon Mar 6, 2011
This patch adds support for the GCC visibility attributes to the PyAPI_* macros (currently only used for Windows.) GCC's default visibility is 'public', but can be changed to 'hidden' with the '-fvisibility=hidden' argument; see http://gcc.gnu.org/wiki/Visibility. This patch does not make the build use that, it merely makes Python function correctly when the default visibility *is* changed. (The benefit of building Python with -fvisibility=hidden is very small, as it causes only a handful of symbols to not be exported. When embedding Python, though, this can make a lot of difference.)
The patch also fixes a few modules that don't use PyMODINIT_FUNC for their module-init function definitions, like they should.
Yhg1s commentedon Mar 6, 2011
Uploaded to Rietveld, too: http://codereview.appspot.com/4260052/
pitrou commentedon Mar 6, 2011
Why the cygwin changes? Are they related? Also, is the change in Python/getargs.c necessary?
Yhg1s commentedon Mar 6, 2011
The cygwin changes are no-ops, just refactoring the needlessly nested if statement for clarity. I can revert them.
The getargs.c change *is* necessary, although it doesn't have to be that exact change. The problem is that the functions in that block are not declared in any file in Include/, although I don't know why not (it's true that these function shouldn't be called directly, but they are symbols that should be exported. The ifdef the patch removes makes the export happen only for Windows, but I see no reason to do that conditionally.) To be clear, the #define of (for example) PyArg_Parse to _PyArg_Parse_SizeT in Include/modsupport.h doesn't apply, because Python/getargs.c does not (and must not) define PY_SSIZE_T_CLEAN (or we wouldn't be able to define both PyArg_Parse and _PyArg_Parse_SizeT.)
We could just list these functions in Include/modsupport.h, along with the 'public' (non-PY_SSIZE_T_CLEAN) ones -- but that only makes sense if we want code to call the ssize_t functions directly, which I don't think we want.
pitrou commentedon Mar 6, 2011
Perhaps you can commit them separately? I don't think they need review.
As for the getargs.c change, perhaps Martin has an opinion. I bet he's more knowledgeable than me on the matter.
loewis commentedon Mar 7, 2011
Exporting the SizeT functions on all systems is fine. It's not true that they aren't declared: they are declared in modsupport.h if PY_SSIZE_T_CLEAN is defined. Else they are not declared.
The patch looks fine to. I agree that mere cleanup shouldn't get committed along with substantial changes.
Yhg1s commentedon Mar 7, 2011
Windows/Cygwin parts of the patch reverted and new patch uploaded. My point about the _Py*_SizeT functions is that they're only declared when you define PY_SSIZE_T_CLEAN, and I don't know if we want to change that (I don't think it makes sense to.)
samb commentedon Mar 17, 2015
Um ... any progress on reviewing this?
vstinner commentedon Mar 17, 2015
I extracted the changes on the PyMODINIT_FUNC macro and I opened the issue bpo-23685.
vstinner commentedon Mar 17, 2015
+#if defined(GNUC) && __GNUC__ >= 4
+# define HAVE_ATTRIBUTE_VISIBILITY
+#endif
Clang now also supports __attribute__((visibility("..."))). I don't know since which version.
I'm not sure because I don't see it:
http://clang.llvm.org/docs/AttributeReference.html
I see it there:
http://llvm.org/docs/LangRef.html#visibility-styles
vstinner commentedon Mar 17, 2015
Why not moving these declarations to Include/modsupport.h where _PyArg_Parse...() are already used?
vstinner commentedon Mar 17, 2015
In the issue bpo-23685, I proposed a patch to add _PyBUILTIN_MODINIT_FUNC for builtin modules. It makes possible to hide PyInit_xxx symbols of builtin symbols with __attribute__((visibility("hidden"))). It also avoids to export these privates symbols on Windows in the Python DLL (and on Linux in the Python .so library).
vstinner commentedon Mar 18, 2015
See also the issue bpo-19569: "Use __attribute__((deprecated)) to warn usage of deprecated functions and macros".
njsmith commentedon Sep 1, 2017
Was this actually fixed, or did everyone just get tired and give up on the original patch?
vsajip commentedon Sep 24, 2019
Reopening, as a new patch is available.
vsajip commentedon Oct 15, 2019
New changeset 0b60f64 by Vinay Sajip in branch 'master':
bpo-11410: Standardize and use symbol visibility attributes across POSIX and Windows. (GH-16347)
0b60f64