Use GCC visibility attrs in PyAPI_*

[Yhg1s]

BPO	11410
Nosy	@loewis, @Yhg1s, @vsajip, @pitrou, @vstinner, @njsmith, @davidmalcolm
PRs	bpo-11410: Standardize and use symbol visibility attributes across POSIX and Windows. #16347
Files	gcc-visibility.diff

^{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']

[Yhg1s]

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]

Uploaded to Rietveld, too: http://codereview.appspot.com/4260052/

[pitrou]

Why the cygwin changes? Are they related? Also, is the change in Python/getargs.c necessary?

[Yhg1s]

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]

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.

[loewis]

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]

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]

Um ... any progress on reviewing this?

[vstinner]

I extracted the changes on the PyMODINIT_FUNC macro and I opened the issue bpo-23685.

[vstinner]

+#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]

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?

[vstinner]

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]

See also the issue bpo-19569: "Use __attribute__((deprecated)) to warn usage of deprecated functions and macros".