Issue 23685: Fix usage of PyMODINIT_FUNC

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?

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.

"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"))).

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.

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*

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

I commited PyMODINIT_FUNC.patch without the useless change on PC/config.c.

Sounds good. Wasn't quite sure if you were after any effect or just consistency :)

builtin_modules.patch looks complex. It can be simplified by using the Py_BUILD_CORE define.

> 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.

messages: + msg238310