classification
Title: Use GCC visibility attrs in PyAPI_*
Type: enhancement Stage: resolved
Components: Versions: Python 3.3
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, SamB, dmalcolm, haypo, loewis, pitrou, tromey, twouters
Priority: normal Keywords: needs review, patch

Created on 2011-03-06 06:12 by twouters, last changed 2017-05-22 19:26 by twouters. This issue is now closed.

Files
File name Uploaded Description Edit
gcc-visibility.diff twouters, 2011-03-07 00:21 review
Messages (13)
msg130143 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2011-03-06 06:12
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.
msg130145 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2011-03-06 06:16
Uploaded to Rietveld, too: http://codereview.appspot.com/4260052/
msg130163 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-06 10:38
Why the cygwin changes? Are they related? Also, is the change in Python/getargs.c necessary?
msg130194 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2011-03-06 21:36
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.
msg130196 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-06 22:03
> The cygwin changes are no-ops, just refactoring the needlessly nested
> if statement for clarity. I can revert them.

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.
msg130208 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-03-07 00:00
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.
msg130211 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2011-03-07 00:21
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.)
msg238257 - (view) Author: Samuel Bronson (SamB) Date: 2015-03-17 03:07
Um ... any progress on reviewing this?
msg238273 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-17 10:14
I extracted the changes on the PyMODINIT_FUNC macro and I opened the issue #23685.
msg238274 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-17 10:22
+#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
msg238275 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-17 10:25
> The getargs.c change *is* necessary, although it doesn't have to be that exact change.

Why not moving these declarations to Include/modsupport.h where _PyArg_Parse...() are already used?
msg238322 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-17 17:16
In the issue #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).
msg238422 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-18 12:44
See also the issue #19569: "Use __attribute__((deprecated)) to warn usage of deprecated functions and macros".
History
Date User Action Args
2017-05-22 19:26:35twouterssetstatus: open -> closed
resolution: out of date
stage: patch review -> resolved
2015-03-22 12:54:14Arfreversetnosy: + Arfrever
2015-03-18 12:44:59hayposetmessages: + msg238422
2015-03-17 17:16:54hayposetmessages: + msg238322
2015-03-17 10:25:04hayposetmessages: + msg238275
2015-03-17 10:22:37hayposetmessages: + msg238274
2015-03-17 10:14:29hayposetmessages: + msg238273
2015-03-17 03:07:06SamBsetnosy: + SamB
messages: + msg238257
2013-12-04 01:14:44hayposetnosy: + haypo
2013-12-02 16:19:04tromeysetnosy: + tromey
2011-03-07 17:30:03dmalcolmsetnosy: + dmalcolm
2011-03-07 00:21:13twouterssetfiles: - gcc-visibility.diff
2011-03-07 00:21:03twouterssetfiles: + gcc-visibility.diff

messages: + msg130211
2011-03-07 00:00:29loewissetmessages: + msg130208
2011-03-06 22:03:39pitrousetnosy: + loewis
messages: + msg130196
2011-03-06 21:36:08twouterssetmessages: + msg130194
2011-03-06 10:38:53pitrousetstage: patch review
type: enhancement
versions: + Python 3.3
2011-03-06 10:38:43pitrousetnosy: + pitrou
messages: + msg130163
2011-03-06 06:16:15twouterssetmessages: + msg130145
2011-03-06 06:14:09twouterssetmessages: - msg130144
2011-03-06 06:14:01twouterssetmessages: + msg130144
2011-03-06 06:12:25twouterscreate