bpo-11410: Standardize and use symbol visibility attributes across POSIX and Windows.#16347

vsajip · 2019-09-24T04:34:59Z

https://bugs.python.org/issue11410

vstinner · 2019-09-24T10:02:27Z

Python/graminit.c

+#include "exports.h"
 #include "grammar.h"
-grammar _PyParser_Grammar;
+Py_EXPORTED_SYMBOL grammar _PyParser_Grammar;


I would prefer to not export private symbols (names starting with "_Py"). Or is there a good reason to export that one?

If you don't export it, e.g. by using just plain extern, the parser module fails to build:

building 'parser' extension gcc -pthread -fPIC -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I./Include -I/home/vinay/.local/include -I. -I/usr/include/x86_64-linux-gnu -I/usr/local/include -I/home/vinay/projects/python/master/Include -I/home/vinay/projects/python/master -c /home/vinay/projects/python/master/Modules/parsermodule.c -o build/temp.linux-x86_64-3.9/home/vinay/projects/python/master/Modules/parsermodule.o gcc -pthread -shared build/temp.linux-x86_64-3.9/home/vinay/projects/python/master/Modules/parsermodule.o -L/home/vinay/.local/lib -L/usr/lib/x86_64-linux-gnu -L/usr/local/lib -o build/lib.linux-x86_64-3.9/parser.cpython-39-x86_64-linux-gnu.so *** WARNING: renaming "parser" since importing it failed: build/lib.linux-x86_64-3.9/parser.cpython-39-x86_64-linux-gnu.so: undefined symbol: _PyParser_Grammar

vstinner · 2019-09-24T10:03:07Z

Python/getargs.c


-int
+PyAPI_FUNC(int)
 _PyArg_Parse_SizeT(PyObject *args, const char *format, ...)


Why do we have to export _PyArg symbols in specific? Can't we only configure the visibility in header files?

The problem here is that these aren't declared in any header file - there are forward declarations at the top of Python/getargs.c with PyAPI_FUNC (example), but the actual definitions didn't have PyAPI_FUNC, with the end result that the symbols weren't exported. Adding PyAPI_FUNC to the definitions fixed that.

To add to the fun, Include/modsupport.h contains definitions like

#define PyArg_Parse _PyArg_Parse_SizeT

if PY_SSIZE_T_CLEAN is defined. So a public API is aliased in a common case to what looks like a private API through a #define! Of course, Include/modsupport.h is included in Include/Python.h, so these are used in a lot of places.

vstinner · 2019-09-24T10:03:31Z

Modules/_io/_iomodule.h

 extern PyObject *_PyIO_empty_bytes;

-extern PyTypeObject _PyBytesIOBuffer_Type;
+extern Py_EXPORTED_SYMBOL PyTypeObject _PyBytesIOBuffer_Type;


Why do you export these _io types, they should be private, no?

Even if I remove Py_EXPORTED_SYMBOL here in the header, it's exported via the definition in /Modules/_io/bytesio.c. If I remove it there too, the _testcapi module fails to build:

running build running build_ext INFO: Can't locate Tcl/Tk libs and/or headers *** WARNING: renaming "_testcapi" since importing it failed: build/lib.linux-x86_64-3.9/_testcapi.cpython-39-x86_64-linux-gnu.so: undefined symbol: _PyBytesIOBuffer_Type

vstinner · 2019-09-24T10:03:50Z

Include/exports.h

@@ -0,0 +1,20 @@
+#ifndef Py_EXPORTS_H
+#define Py_EXPORTS_H


I would prefer to restrict the number of header files: why not reusing pyport.h?

Because exporting symbols is orthogonal to other things. Consider graminit.c - it's a very low-level module which needs to export a symbol, but doesn't really use any other Python-specific or system-specific stuff. If you replace "exports.h" with "pyport.h" in graminit.c, you get an error when compiling:

gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I. -I./Include -DPy_BUILD_CORE -o Python/graminit.o Python/graminit.c In file included from Python/graminit.c:3:0: ./Include/pyport.h:105:9: error: unknown type name ‘ssize_t’ typedef ssize_t Py_ssize_t; ^ ./Include/pyport.h:117:9: error: unknown type name ‘size_t’ typedef size_t Py_uhash_t; ^ Makefile:1711: recipe for target 'Python/graminit.o' failed

Including <stddef.h> before "pyport.h" doesn't fix things:

gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I. -I./Include -DPy_BUILD_CORE -o Python/graminit.o Python/graminit.c In file included from Python/graminit.c:3:0: ./Include/pyport.h:105:9: error: unknown type name ‘ssize_t’ typedef ssize_t Py_ssize_t; ^ Makefile:1711: recipe for target 'Python/graminit.o' failed

So you end up having to include "Python.h" instead, which appears to be overkill as it exposes a lot of things to code in graminit.c which aren't needed by that code.

bedevere-bot · 2019-09-24T10:03:58Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vsajip · 2019-09-24T15:22:14Z

I have made the requested changes; please review again.

bedevere-bot · 2019-09-24T15:22:17Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vsajip · 2019-09-24T15:23:58Z

BTW I've no idea why GitHub thinks I've started a review of my own PR - I was only replying to Victor's comments, so it's not clear what happened. I can't seem to cancel my spurious review, either.

vsajip · 2019-09-24T15:41:00Z

OK, refreshing the page seems to have got rid of my spurious review :-)

ronaldoussoren · 2019-09-29T16:27:16Z

Include/exports.h

+    #define Py_EXPORTED_SYMBOL __declspec(dllexport)
+    #define Py_LOCAL_SYMBOL
+#else
+    #if __GNUC__ >= 4


clang also supports this attribute.

Using #if __has_attribute(visibility) should work with both clang and gcc without hardcoding compiler names. This does require GCC 5 or later though.

Some versions of e.g. Ubuntu and CentOS are still on GCC 4.8. I'll try to generalise it.

visibility attribute is available.

vsajip · 2019-10-14T09:37:26Z

I'd like to merge this - I believe I've adequately addressed comments by both @vstinner and @ronaldoussoren - if there are any objections to merging, please let me know!

pablogsal · 2019-10-14T09:57:40Z

I would suggest checking with the buildbots before merging, as the pr is changing the autotools scripts.

vsajip · 2019-10-14T20:09:36Z

Pushed this branch to buildbot-custom and looked at the results of the resulting builds. While there are some failures, none appear to be related to this change.

pablogsal · 2019-10-14T20:16:07Z

@vsajip Thanks for checking!

…SIX and Windows. (pythonGH-16347)

vsajip added 4 commits September 23, 2019 15:58

Initial attempt at visibility changes.

cbdada0

Added parentheses to #if defined(...) syntax.

50c661c

Refined export definitions to avoid repetition.

6421318

Added NEWS entry.

049cd45

vsajip added the type-feature A feature request or enhancement label Sep 24, 2019

vsajip requested review from njsmith and vstinner September 24, 2019 04:34

vsajip requested a review from pablogsal as a code owner September 24, 2019 04:35

the-knights-who-say-ni added the CLA signed label Sep 24, 2019

bedevere-bot added the awaiting core review label Sep 24, 2019

Removed unnecessary preprocessor condition.

dc85dbb

vstinner requested changes Sep 24, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 24, 2019

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 24, 2019

bedevere-bot requested a review from vstinner September 24, 2019 15:22

ronaldoussoren reviewed Sep 29, 2019

View reviewed changes

Generalised the conditions under which it is assumed that the

f4a3429

visibility attribute is available.

Added a definition for non-clang compilers.

886a68e

vsajip merged commit 0b60f64 into python:master Oct 15, 2019

vsajip deleted the visibility branch October 15, 2019 07:26

bedevere-bot removed the awaiting change review label Oct 15, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-11410: Standardize and use symbol visibility attributes across PO…

162aa77

…SIX and Windows. (pythonGH-16347)

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-11410: Standardize and use symbol visibility attributes across PO…

56eb072

…SIX and Windows. (pythonGH-16347)

jd mentioned this pull request Apr 8, 2020

ci: test profiler on Python 3.9 DataDog/dd-trace-py#1348

Merged

Uh oh!

Conversation

vsajip commented Sep 24, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 24, 2019

Uh oh!

vsajip commented Sep 24, 2019

Uh oh!

bedevere-bot commented Sep 24, 2019

Uh oh!

vsajip commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsajip commented Sep 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsajip commented Oct 14, 2019

Uh oh!

pablogsal commented Oct 14, 2019

Uh oh!

vsajip commented Oct 14, 2019

Uh oh!

pablogsal commented Oct 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vsajip commented Sep 24, 2019 •

edited by bedevere-bot

Loading

vsajip commented Sep 24, 2019 •

edited

Loading