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

PyCFunction_New is not exported with PyAPI_FUNC #87405

Closed
encukou opened this issue Feb 16, 2021 · 8 comments
Closed

PyCFunction_New is not exported with PyAPI_FUNC #87405

encukou opened this issue Feb 16, 2021 · 8 comments
Assignees
@encukou
Labels
3.10 only security fixes topic-C-API

Comments

@encukou
Copy link
Member

encukou commented Feb 16, 2021

BPO 43239
Nosy @vstinner, @encukou, @shihai1991, @erlend-aasland
PRs
  • bpo-43239: Export PyCFunction_New with PyAPI_FUNC #24551
    		•

    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 = 'https://github.com/encukou'
closed_at = None
created_at = <Date 2021-02-16.16:07:52.116>
labels = ['expert-C-API', '3.10']
title = 'PyCFunction_New is not exported with PyAPI_FUNC'
updated_at = <Date 2021-10-20.08:59:18.825>
user = 'https://github.com/encukou'

    bugs.python.org fields:

    activity = <Date 2021-10-20.08:59:18.825>
actor = 'erlendaasland'
assignee = 'petr.viktorin'
closed = False
closed_date = None
closer = None
components = ['C API']
creation = <Date 2021-02-16.16:07:52.116>
creator = 'petr.viktorin'
dependencies = []
files = []
hgrepos = []
issue_num = 43239
keywords = ['patch']
message_count = 7.0
messages = ['387119', '387120', '387121', '387122', '387163', '387564', '404406']
nosy_count = 4.0
nosy_names = ['vstinner', 'petr.viktorin', 'shihai1991', 'erlendaasland']
pr_nums = ['24551']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'pending'
superseder = None
type = None
url = 'https://bugs.python.org/issue43239'
versions = ['Python 3.10']
    @encukou
    Copy link
    Member Author

    encukou commented Feb 16, 2021

    PyCFunction_New is part of the Stable ABI (see bpo-21354), but it is not declared as PyAPI_FUNC, and thus not exported with GCC's -fvisibility=hidden.
    To match Windows, it should be declared.

    (I don't think this has any real impact, I just want to fix it before enabling automatic tests on such issues.)

    @encukou encukou added the 3.10 only security fixes label Feb 16, 2021
    @encukou encukou self-assigned this Feb 16, 2021
    @encukou encukou added topic-C-API 3.10 only security fixes labels Feb 16, 2021
    @encukou encukou self-assigned this Feb 16, 2021
    @vstinner
    Copy link
    Member

    Oh, it's not listed by Doc/data/stable_abi.dat and so not checked by Tools/scripts/stable_abi.py.

    It's implemented explicitly as a function only for the stable ABI:

    PyObject *
PyCFunction_New(PyMethodDef *ml, PyObject *self)
{
    return PyCFunction_NewEx(ml, self, NULL);
}

    So yeah, it should be exported.

    By the way, I would suggest to move "#define PyCFunction_New(ML, SELF) PyCFunction_NewEx((ML), (SELF), NULL)" to Include/cpython/methodobject.h to clarify that the limited C API should call "PyCFunction_New()" and not call "PyCFunction_NewEx()".

    @encukou
    Copy link
    Member Author

    encukou commented Feb 16, 2021

    By the way, I would suggest to move "#define PyCFunction_New(ML, SELF) PyCFunction_NewEx((ML), (SELF), NULL)" to Include/cpython/methodobject.h

    OK, I'll add that move to the PR.

    Oh, it's not listed by Doc/data/stable_abi.dat and so not checked by Tools/scripts/stable_abi.py.

    I'm planning to rework this for PEP-652 (which is how I found the issue in the first place), so I'll not add it there now.

    @encukou
    Copy link
    Member Author

    encukou commented Feb 16, 2021

    By the way, I would suggest to move "#define PyCFunction_New(ML, SELF) PyCFunction_NewEx((ML), (SELF), NULL)" to Include/cpython/methodobject.h

    Wait no, I wrote too soon.
    PyCFunction_NewEx is part of the limited API as well, even though it's undocumented. People can call it.

    @shihai1991
    Copy link
    Member

    > PyCFunction_NewEx is part of the limited API as well, even though it's undocumented. People can call it.

    Wait no, I wrote too soon.
    PyCFunction_NewEx is part of the limited API as well, even though it's >undocumented. People can call it.

    Find some 3rd user calling this function in: https://searchcode.com/?q=PyCFunction_NewEx+. So I prefer to keep it too.
    If calling PyCFunction_New() is better than PyCFunction_NewEx(), MAYBE we can add some description info in PR 24551.

    @encukou
    Copy link
    Member Author

    encukou commented Feb 23, 2021

    New changeset 7bb1caf by Petr Viktorin in branch 'master':
    bpo-43239: Export PyCFunction_New with PyAPI_FUNC (GH-24551)
    7bb1caf

    @erlend-aasland
    Copy link
    Contributor

    It seems to me that this issue can be closed as fixed. Do you have further PR's for this, Petr?

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label May 20, 2022
    @kumaraditya303
    Copy link
    Contributor

    Closing as it seems fixed, feel free to reopen if required.

    @AA-Turner AA-Turner removed the pending The issue will be closed if no feedback is provided label Jun 19, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @encukou encukou

    Labels
    3.10 only security fixes topic-C-API
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    6 participants
    @vstinner @encukou @AA-Turner @shihai1991 @erlend-aasland @kumaraditya303