Skip to content
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.

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

Jump to bottom

Fix usage of PyMODINIT_FUNC #67873

Closed
vstinner opened this issue Mar 17, 2015 · 10 comments
Closed

Fix usage of PyMODINIT_FUNC #67873

vstinner opened this issue Mar 17, 2015 · 10 comments
Labels
build The build process and cross-build OS-windows

Comments

@vstinner
Copy link
Member

BPO 23685
Nosy @loewis, @Yhg1s, @pitrou, @vstinner, @tjguk, @zware, @zooba
Files
  • PyMODINIT_FUNC.patch
  • builtin_modules.patch
    		•

    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 2015-03-20.09:59:35.967>
created_at = <Date 2015-03-17.10:13:53.146>
labels = ['build', 'OS-windows']
title = 'Fix usage of PyMODINIT_FUNC'
updated_at = <Date 2015-03-20.09:59:35.965>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2015-03-20.09:59:35.965>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2015-03-20.09:59:35.967>
closer = 'vstinner'
components = ['Build', 'Windows']
creation = <Date 2015-03-17.10:13:53.146>
creator = 'vstinner'
dependencies = []
files = ['38517', '38527']
hgrepos = []
issue_num = 23685
keywords = ['patch']
message_count = 10.0
messages = ['238272', '238302', '238303', '238306', '238310', '238311', '238312', '238318', '238397', '238634']
nosy_count = 8.0
nosy_names = ['loewis', 'twouters', 'pitrou', 'vstinner', 'tim.golden', 'python-dev', 'zach.ware', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue23685'
versions = ['Python 3.5']
    @vstinner
    Copy link
    Member Author

    Attached patch fixes the usage of the PyMODINIT_FUNC macro.

    My patch is based on Thomas Wouters's patch of the issue bpo-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?

    @vstinner vstinner added build The build process and cross-build OS-windows labels Mar 17, 2015
    @zooba
    Copy link
    Member

    zooba commented Mar 17, 2015

    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.

    @vstinner
    Copy link
    Member Author

    "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 bpo-11410, we may use it to hide the symbols: __attribute__((visibility("hidden"))).

    @zooba
    Copy link
    Member

    zooba commented Mar 17, 2015

    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.

    @vstinner
    Copy link
    Member Author

    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*

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 17, 2015

    New changeset 22a0c925a7c2 by Victor Stinner in branch 'default':
    Issue bpo-23685: Fix usage of PyMODINIT_FUNC in _json, _scproxy, nis, pyexpat
    https://hg.python.org/cpython/rev/22a0c925a7c2

    @vstinner
    Copy link
    Member Author

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

    @zooba
    Copy link
    Member

    zooba commented Mar 17, 2015

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

    @vstinner
    Copy link
    Member Author

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

    @vstinner
    Copy link
    Member Author

    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 bpo-11410. I consider that the initial issue is fixed, so I close the issue.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    build The build process and cross-build OS-windows
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @zooba