Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 15, 2019
+132 −35
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cbdada0
Initial attempt at visibility changes.
vsajip Sep 23, 2019
50c661c
Added parentheses to #if defined(...) syntax.
vsajip Sep 24, 2019
6421318
Refined export definitions to avoid repetition.
vsajip Sep 24, 2019
049cd45
Added NEWS entry.
vsajip Sep 24, 2019
dc85dbb
Removed unnecessary preprocessor condition.
vsajip Sep 24, 2019
f4a3429
Generalised the conditions under which it is assumed that the
vsajip Sep 30, 2019
886a68e
Added a definition for non-clang compilers.
vsajip Oct 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions Include/exports.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#ifndef Py_EXPORTS_H
#define Py_EXPORTS_H
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#if defined(_WIN32) || defined(__CYGWIN__)
#define Py_IMPORTED_SYMBOL __declspec(dllimport)
#define Py_EXPORTED_SYMBOL __declspec(dllexport)
#define Py_LOCAL_SYMBOL
#else
#if __GNUC__ >= 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#define Py_IMPORTED_SYMBOL __attribute__ ((visibility ("default")))
#define Py_EXPORTED_SYMBOL __attribute__ ((visibility ("default")))
#define Py_LOCAL_SYMBOL __attribute__ ((visibility ("hidden")))
#else
#define Py_IMPORTED_SYMBOL
#define Py_EXPORTED_SYMBOL
#define Py_LOCAL_SYMBOL
#endif
#endif

#endif /* Py_EXPORTS_H */
24 changes: 13 additions & 11 deletions Include/pyport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,16 +638,18 @@ extern char * _getpty(int *, int, mode_t, int);
# define HAVE_DECLSPEC_DLL
#endif

#include "exports.h"

/* only get special linkage if built as shared or platform is Cygwin */
#if defined(Py_ENABLE_SHARED) || defined(__CYGWIN__)
# if defined(HAVE_DECLSPEC_DLL)
# if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE)
# define PyAPI_FUNC(RTYPE) __declspec(dllexport) RTYPE
# define PyAPI_DATA(RTYPE) extern __declspec(dllexport) RTYPE
# define PyAPI_FUNC(RTYPE) Py_EXPORTED_SYMBOL RTYPE
# define PyAPI_DATA(RTYPE) extern Py_EXPORTED_SYMBOL RTYPE
/* module init functions inside the core need no external linkage */
/* except for Cygwin to handle embedding */
# if defined(__CYGWIN__)
# define PyMODINIT_FUNC __declspec(dllexport) PyObject*
# define PyMODINIT_FUNC Py_EXPORTED_SYMBOL PyObject*
# else /* __CYGWIN__ */
# define PyMODINIT_FUNC PyObject*
# endif /* __CYGWIN__ */
Expand All @@ -658,31 +660,31 @@ extern char * _getpty(int *, int, mode_t, int);
/* failures similar to those described at the bottom of 4.1: */
/* http://docs.python.org/extending/windows.html#a-cookbook-approach */
# if !defined(__CYGWIN__)
# define PyAPI_FUNC(RTYPE) __declspec(dllimport) RTYPE
# define PyAPI_FUNC(RTYPE) Py_IMPORTED_SYMBOL RTYPE
# endif /* !__CYGWIN__ */
# define PyAPI_DATA(RTYPE) extern __declspec(dllimport) RTYPE
# define PyAPI_DATA(RTYPE) extern Py_IMPORTED_SYMBOL RTYPE
/* module init functions outside the core must be exported */
# if defined(__cplusplus)
# define PyMODINIT_FUNC extern "C" __declspec(dllexport) PyObject*
# define PyMODINIT_FUNC extern "C" Py_EXPORTED_SYMBOL PyObject*
# else /* __cplusplus */
# define PyMODINIT_FUNC __declspec(dllexport) PyObject*
# define PyMODINIT_FUNC Py_EXPORTED_SYMBOL PyObject*
# endif /* __cplusplus */
# endif /* Py_BUILD_CORE */
# endif /* HAVE_DECLSPEC_DLL */
#endif /* Py_ENABLE_SHARED */

/* If no external linkage macros defined by now, create defaults */
#ifndef PyAPI_FUNC
# define PyAPI_FUNC(RTYPE) RTYPE
# define PyAPI_FUNC(RTYPE) Py_EXPORTED_SYMBOL RTYPE
#endif
#ifndef PyAPI_DATA
# define PyAPI_DATA(RTYPE) extern RTYPE
# define PyAPI_DATA(RTYPE) extern Py_EXPORTED_SYMBOL RTYPE
#endif
#ifndef PyMODINIT_FUNC
# if defined(__cplusplus)
# define PyMODINIT_FUNC extern "C" PyObject*
# define PyMODINIT_FUNC extern "C" Py_EXPORTED_SYMBOL PyObject*
# else /* __cplusplus */
# define PyMODINIT_FUNC PyObject*
# define PyMODINIT_FUNC Py_EXPORTED_SYMBOL PyObject*
# endif /* __cplusplus */
#endif

Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2019-09-24-05-32-27.bpo-11410.vS182p.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Better control over symbol visibility is provided through use of the
visibility attributes available in gcc >= 4.0, provided in a uniform way
across POSIX and Windows. The POSIX build files have been updated to compile
with -fvisibility=hidden, minimising exported symbols.
6 changes: 1 addition & 5 deletions Modules/_ctypes/_ctypes_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
#include <windows.h>
#endif

#if defined(MS_WIN32) || defined(__CYGWIN__)
#define EXPORT(x) __declspec(dllexport) x
#else
#define EXPORT(x) x
#endif
#define EXPORT(x) Py_EXPORTED_SYMBOL x

/* some functions handy for testing */

Expand Down
4 changes: 3 additions & 1 deletion Modules/_io/_iomodule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
* Declarations shared between the different parts of the io module
*/

#include "exports.h"

/* ABCs */
extern PyTypeObject PyIOBase_Type;
extern PyTypeObject PyRawIOBase_Type;
Expand Down Expand Up @@ -183,4 +185,4 @@ extern PyObject *_PyIO_str_write;
extern PyObject *_PyIO_empty_str;
extern PyObject *_PyIO_empty_bytes;

extern PyTypeObject _PyBytesIOBuffer_Type;
extern Py_EXPORTED_SYMBOL PyTypeObject _PyBytesIOBuffer_Type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

2 changes: 1 addition & 1 deletion Modules/_io/bytesio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,7 @@ static PyBufferProcs bytesiobuf_as_buffer = {
(releasebufferproc) bytesiobuf_releasebuffer,
};

PyTypeObject _PyBytesIOBuffer_Type = {
Py_EXPORTED_SYMBOL PyTypeObject _PyBytesIOBuffer_Type = {
PyVarObject_HEAD_INIT(NULL, 0)
"_io._BytesIOBuffer", /*tp_name*/
sizeof(bytesiobuf), /*tp_basicsize*/
Expand Down
5 changes: 3 additions & 2 deletions Parser/pgen/grammar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,14 @@ def produce_graminit_h(self, writer):
def produce_graminit_c(self, writer):
writer("/* Generated by Parser/pgen */\n\n")

writer('#include "exports.h"\n')
writer('#include "grammar.h"\n')
writer("grammar _PyParser_Grammar;\n")
writer("Py_EXPORTED_SYMBOL grammar _PyParser_Grammar;\n")

self.print_dfas(writer)
self.print_labels(writer)

writer("grammar _PyParser_Grammar = {\n")
writer("Py_EXPORTED_SYMBOL grammar _PyParser_Grammar = {\n")
writer(" {n_dfas},\n".format(n_dfas=len(self.dfas)))
writer(" dfas,\n")
writer(" {{{n_labels}, labels}},\n".format(n_labels=len(self.labels)))
Expand Down
24 changes: 12 additions & 12 deletions Python/getargs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ PyArg_Parse(PyObject *args, const char *format, ...)
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_Parse_SizeT(PyObject *args, const char *format, ...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
int retval;
Expand All @@ -131,7 +131,7 @@ PyArg_ParseTuple(PyObject *args, const char *format, ...)
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_ParseTuple_SizeT(PyObject *args, const char *format, ...)
{
int retval;
Expand All @@ -156,7 +156,7 @@ _PyArg_ParseStack(PyObject *const *args, Py_ssize_t nargs, const char *format, .
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_ParseStack_SizeT(PyObject *const *args, Py_ssize_t nargs, const char *format, ...)
{
int retval;
Expand All @@ -182,7 +182,7 @@ PyArg_VaParse(PyObject *args, const char *format, va_list va)
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_VaParse_SizeT(PyObject *args, const char *format, va_list va)
{
va_list lva;
Expand Down Expand Up @@ -1442,7 +1442,7 @@ PyArg_ParseTupleAndKeywords(PyObject *args,
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_ParseTupleAndKeywords_SizeT(PyObject *args,
PyObject *keywords,
const char *format,
Expand Down Expand Up @@ -1493,7 +1493,7 @@ PyArg_VaParseTupleAndKeywords(PyObject *args,
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_VaParseTupleAndKeywords_SizeT(PyObject *args,
PyObject *keywords,
const char *format,
Expand All @@ -1519,7 +1519,7 @@ _PyArg_VaParseTupleAndKeywords_SizeT(PyObject *args,
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_ParseTupleAndKeywordsFast(PyObject *args, PyObject *keywords,
struct _PyArg_Parser *parser, ...)
{
Expand All @@ -1532,7 +1532,7 @@ _PyArg_ParseTupleAndKeywordsFast(PyObject *args, PyObject *keywords,
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_ParseTupleAndKeywordsFast_SizeT(PyObject *args, PyObject *keywords,
struct _PyArg_Parser *parser, ...)
{
Expand All @@ -1545,7 +1545,7 @@ _PyArg_ParseTupleAndKeywordsFast_SizeT(PyObject *args, PyObject *keywords,
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_ParseStackAndKeywords(PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames,
struct _PyArg_Parser *parser, ...)
{
Expand All @@ -1558,7 +1558,7 @@ _PyArg_ParseStackAndKeywords(PyObject *const *args, Py_ssize_t nargs, PyObject *
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_ParseStackAndKeywords_SizeT(PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames,
struct _PyArg_Parser *parser, ...)
{
Expand All @@ -1572,7 +1572,7 @@ _PyArg_ParseStackAndKeywords_SizeT(PyObject *const *args, Py_ssize_t nargs, PyOb
}


int
PyAPI_FUNC(int)
_PyArg_VaParseTupleAndKeywordsFast(PyObject *args, PyObject *keywords,
struct _PyArg_Parser *parser, va_list va)
{
Expand All @@ -1586,7 +1586,7 @@ _PyArg_VaParseTupleAndKeywordsFast(PyObject *args, PyObject *keywords,
return retval;
}

int
PyAPI_FUNC(int)
_PyArg_VaParseTupleAndKeywordsFast_SizeT(PyObject *args, PyObject *keywords,
struct _PyArg_Parser *parser, va_list va)
{
Expand Down
5 changes: 3 additions & 2 deletions Python/graminit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/* Generated by Parser/pgen */

#include "exports.h"
#include "grammar.h"
grammar _PyParser_Grammar;
Py_EXPORTED_SYMBOL grammar _PyParser_Grammar;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static const arc arcs_0_0[3] = {
{2, 1},
{3, 2},
Expand Down Expand Up @@ -2693,7 +2694,7 @@ static const label labels[183] = {
{346, 0},
{347, 0},
};
grammar _PyParser_Grammar = {
Py_EXPORTED_SYMBOL grammar _PyParser_Grammar = {
92,
dfas,
{183, labels},
Expand Down
2 changes: 1 addition & 1 deletion Python/pythonrun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ _Py_static_string(PyId_string, "<string>");
extern "C" {
#endif

extern grammar _PyParser_Grammar; /* From graminit.c */
extern Py_EXPORTED_SYMBOL grammar _PyParser_Grammar; /* From graminit.c */

/* Forward */
static void flush_io(void);
Expand Down
41 changes: 41 additions & 0 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7341,6 +7341,47 @@ $as_echo "$ac_cv_enable_implicit_function_declaration_error" >&6; }
CFLAGS_NODIST="$CFLAGS_NODIST -Werror=implicit-function-declaration"
fi

{ $as_echo "$as_me:${as_lineno-$LINENO}: checking if we can use visibility in $CC" >&5
$as_echo_n "checking if we can use visibility in $CC... " >&6; }
ac_save_cc="$CC"
CC="$CC -fvisibility=hidden"
if ${ac_cv_enable_visibility+:} false; then :
$as_echo_n "(cached) " >&6
else
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */


int
main ()
{

;
return 0;
}

_ACEOF
if ac_fn_c_try_compile "$LINENO"; then :

ac_cv_enable_visibility=yes

else

ac_cv_enable_visibility=no

fi
rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
fi

CC="$ac_save_cc"
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_enable_visibility" >&5
$as_echo "$ac_cv_enable_visibility" >&6; }

if test $ac_cv_enable_visibility = yes
then
CFLAGS_NODIST="$CFLAGS_NODIST -fvisibility=hidden"
fi

# if using gcc on alpha, use -mieee to get (near) full IEEE 754
# support. Without this, treatment of subnormals doesn't follow
# the standard.
Expand Down
20 changes: 20 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,26 @@ yes)
CFLAGS_NODIST="$CFLAGS_NODIST -Werror=implicit-function-declaration"
fi

AC_MSG_CHECKING(if we can use visibility in $CC)
ac_save_cc="$CC"
CC="$CC -fvisibility=hidden"
AC_CACHE_VAL(ac_cv_enable_visibility,
AC_COMPILE_IFELSE(
[
AC_LANG_PROGRAM([[]], [[]])
],[
ac_cv_enable_visibility=yes
],[
ac_cv_enable_visibility=no
]))
CC="$ac_save_cc"
AC_MSG_RESULT($ac_cv_enable_visibility)

if test $ac_cv_enable_visibility = yes
then
CFLAGS_NODIST="$CFLAGS_NODIST -fvisibility=hidden"
fi

# if using gcc on alpha, use -mieee to get (near) full IEEE 754
# support. Without this, treatment of subnormals doesn't follow
# the standard.
Expand Down