classification
Title: Fix usage of PyMODINIT_FUNC
Type: Stage:
Components: Build, Windows Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: loewis, pitrou, python-dev, steve.dower, tim.golden, twouters, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2015-03-17 10:13 by vstinner, last changed 2015-03-20 09:59 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
PyMODINIT_FUNC.patch vstinner, 2015-03-17 10:13 review
builtin_modules.patch vstinner, 2015-03-17 16:44 review
Messages (10)
msg238272 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 10:13
Attached patch fixes the usage of the PyMODINIT_FUNC macro.

My patch is based on Thomas Wouters's patch of the issue #11410.

I don't understand why Modules/pyexpat.c redefined PyMODINIT_FUNC if not defined. In which case PyMODINIT_FUNC was not defined?

I'm not sure that PC/config.c should use PyMODINIT_FUNC instead of use "PyObject*".

@Steve.Dower: Does my change to PC/config.c look correct or not?
msg238302 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-17 15:29
I don't think we should be using PyMODINIT_FUNC for builtin modules, since that will make the init functions publicly available from python35.dll. That said, I do like being able to be consistent here... can we define PyMODINIT_FUNC differently when building pythoncore? That at least would keep the definitions in the same place if we ever change it (and since there's an open PEP regarding extension modules, it doesn't seem impossible).

The change for pyexpat looks right, since that's an external module. I have no ideas why PyMODINIT_FUNC wouldn't always be defined there.
msg238303 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 15:44
"I don't think we should be using PyMODINIT_FUNC for builtin modules, since that will make the init functions publicly available from python35.dll."

Do you mean that my change on PC/config.c is wrong?

For example, Modules/arraymodule.c already contains:

"PyMODINIT_FUNC PyInit_array(void)".

Is the PyInit_array symbol exported today or not on Windows?

If you think that the PyInit_array symbol should be private, maybe there is already an issue in Modules/arraymodule.c?

"can we define PyMODINIT_FUNC differently when building pythoncore?"

We can add a different macro for builtin modules. Using the issue #11410, we may use it to hide the symbols: __attribute__((visibility("hidden"))).
msg238306 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-17 15:51
Just had a look in Include/pyport.h and we're already defining PyMODINIT_FUNC differently for building core, so all your changes should be fine. The redefinition you removed from pyexpat.c was clearly never meant to be used for builtin modules.
msg238310 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 16:44
builtin_modules.patch: add _PyBUILTIN_MODINIT_FUNC macro and use it on modules which are builtins on POSIX.

I checked: it's not necessary to modify Modules/config.c to make the symbol hidden: only the C code in Modules/ need to set the attribute on the function.

So Modules/config.c, Modules/config.c.in, Modules/makesetup, PC/config.c, Tools/freeze/makeconfig.py, ... are unchanged by the patch.

--

On Linux with GCC >= 4.0, you can test manually to hide PyInit_xxx symbols using:

#define _PyBUILTIN_MODINIT_FUNC __attribute__((visibility("hidden"))) PyObject*
msg238311 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-03-17 16:53
New changeset 22a0c925a7c2 by Victor Stinner in branch 'default':
Issue #23685: Fix usage of PyMODINIT_FUNC in _json, _scproxy, nis, pyexpat
https://hg.python.org/cpython/rev/22a0c925a7c2
msg238312 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-17 16:54
I commited PyMODINIT_FUNC.patch without the useless change on PC/config.c.
msg238318 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-17 17:08
Sounds good. Wasn't quite sure if you were after any effect or just consistency :)
msg238397 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-18 10:25
builtin_modules.patch looks complex. It can be simplified by using the Py_BUILD_CORE define.
msg238634 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-20 09:59
> builtin_modules.patch looks complex. It can be simplified by using the Py_BUILD_CORE define.

In this case, it can be done in the issue #11410. I consider that the initial issue is fixed, so I close the issue.
History
Date User Action Args
2015-03-20 09:59:35vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg238634
2015-03-18 10:25:42vstinnersetmessages: + msg238397
2015-03-17 17:08:56steve.dowersetmessages: + msg238318
2015-03-17 16:54:11vstinnersetmessages: + msg238312
2015-03-17 16:53:27python-devsetnosy: + python-dev
messages: + msg238311
2015-03-17 16:44:02vstinnersetfiles: + builtin_modules.patch

messages: + msg238310
2015-03-17 15:51:55steve.dowersetmessages: + msg238306
2015-03-17 15:44:14vstinnersetmessages: + msg238303
2015-03-17 15:29:32steve.dowersetmessages: + msg238302
2015-03-17 10:13:53vstinnercreate