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

PyIndex_Check conflicts with PEP 384 #77919

Closed
ctismer opened this issue Jun 1, 2018 · 26 comments
Closed

PyIndex_Check conflicts with PEP 384 #77919

ctismer opened this issue Jun 1, 2018 · 26 comments
Labels
3.8 only security fixes build The build process and cross-build extension-modules C modules in the Modules dir

Comments

@ctismer
Copy link
Contributor

ctismer commented Jun 1, 2018

BPO 33738
Nosy @pitrou, @vstinner, @larryhastings, @ned-deily, @ericsnowcurrently, @serhiy-storchaka, @ctismer
PRs
  • bpo-33738: Fix macros which contradict PEP 384 #7477
  • bpo-33738: Fix macros which contradict PEP 384 #7585
  • bpo-33818: PyExceptionClass_Name() will now return "const char *". #7581
  • Clean up after bpo-33738. #7627
  • 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 2020-06-22.09:12:20.637>
    created_at = <Date 2018-06-01.17:14:58.908>
    labels = ['extension-modules', 'build', '3.8']
    title = 'PyIndex_Check conflicts with PEP 384'
    updated_at = <Date 2020-06-22.09:12:20.636>
    user = 'https://github.com/ctismer'

    bugs.python.org fields:

    activity = <Date 2020-06-22.09:12:20.636>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-06-22.09:12:20.637>
    closer = 'vstinner'
    components = ['Extension Modules']
    creation = <Date 2018-06-01.17:14:58.908>
    creator = 'Christian.Tismer'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33738
    keywords = ['patch']
    message_count = 26.0
    messages = ['318436', '318449', '318458', '318461', '318462', '318465', '318516', '318908', '318909', '318910', '318925', '318951', '318952', '319018', '319073', '319169', '319174', '319197', '319207', '319217', '319256', '319259', '319297', '319301', '372048', '372057']
    nosy_count = 7.0
    nosy_names = ['pitrou', 'vstinner', 'larry', 'ned.deily', 'eric.snow', 'serhiy.storchaka', 'Christian.Tismer']
    pr_nums = ['7477', '7585', '7581', '7627']
    priority = 'critical'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue33738'
    versions = ['Python 3.8']

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 1, 2018

    The file number.rst on python 3.6 says

    """
    .. c:function:: int PyIndex_Check(PyObject *o)

    Returns 1 if o is an index integer (has the nb_index slot of the
    tp_as_number structure filled in), and 0 otherwise.
    """

    But in reality, this is a macro:

    """
    #define PyIndex_Check(obj) \
       ((obj)->ob_type->tp_as_number != NULL && \
        (obj)->ob_type->tp_as_number->nb_index != NULL)
    """

    But such a macro does not work with the limited API, since
    non-heaptypes cannot use PyType_GetSlot.

    The patch is trivial: Define the function instead of the macro
    when Py_LIMITED_API is set.

    I also think the documentation makes more sense when it is correct.

    @ctismer ctismer added release-blocker 3.7 (EOL) end of life extension-modules C modules in the Modules dir build The build process and cross-build labels Jun 1, 2018
    @ned-deily
    Copy link
    Member

    This is not a security issue so a change would not be applicable to the 3.4 or 3.5 branches, currently in security-fix-only mode.

    @ned-deily ned-deily added the 3.8 only security fixes label Jun 1, 2018
    @serhiy-storchaka
    Copy link
    Member

    Seems PyIter_Check and PySequence_ITEM have the same problem. And maybe more.

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 1, 2018

    Ok, I tried to submit a patch (not yet successful),
    but as it stands, there are more locations in the code where
    this will show up with similar problems.

    Should I take care of these all as much as I can,
    or is it better to leave it to one of you?

    Whatever you prefer is ok. I just want the problem to disappear
    in 3.7, because I hate to break the API any longer.

    Cheers - Chris

    @serhiy-storchaka
    Copy link
    Member

    Would be nice if you fix as much similar as you can with a single PR.

    For performance it makes sense to keep macros if the limited API is not used.

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 1, 2018

    Yes, sure, I will submit a patch that tries to reach as much
    as possible locations that have a similar problem. Of course,
    the code will only be turned into functions for the PEP context.

    Takes a day because I need to re-learn a bit how to do this :-)
    (last patch was 2010, I think... )

    -- related personal note - does someone know what happened to Martin?

    @pitrou
    Copy link
    Member

    pitrou commented Jun 2, 2018

    related personal note - does someone know what happened to Martin?

    I don't think anything bad happened. He still seems to be teaching:
    http://www.beuth-hochschule.de/people/detail/1398/

    @ned-deily
    Copy link
    Member

    Christian, any progress on this? 3.7.0rc1 is planned to be tagged in 4 days, on Monday 2018-06-11. Do you think you will be able to provide a PR before then?

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 7, 2018

    Hi Ned,

    we had a delivery date yesterday for PySide.
    The PR is almost ready and will go out today.

    Ciao - Chris

    On 07.06.18 09:43, Ned Deily wrote:

    Ned Deily <nad@python.org> added the comment:

    Christian, any progress on this? 3.7.0rc1 is planned to be tagged in 4 days, on Monday 2018-06-11. Do you think you will be able to provide a PR before then?

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue33738\>


    @ned-deily
    Copy link
    Member

    Thanks, Christian!

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 7, 2018

    I did not understand the Misc/NEWS.d thing.
    What should go where, or where would a "skip news" label go?

    @ericsnowcurrently
    Copy link
    Member

    @christian, you can use the "blurb" tool to create the NEWS entry. You can use pip to install it. See:

    https://devguide.python.org/committing/?highlight=blurb#what-s-new-and-news-entries

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 7, 2018

    On 07.06.18 17:59, Eric Snow wrote:

    Eric Snow <ericsnowcurrently@gmail.com> added the comment:

    @christian, you can use the "blurb" tool to create the NEWS entry. You can use pip to install it. See:

    https://devguide.python.org/committing/?highlight=blurb#what-s-new-and-news-entries

    ----------
    nosy: +eric.snow


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue33738\>


    Thank you, Eric! Will do so.
    I had something to change, anyway :-)

    @serhiy-storchaka
    Copy link
    Member

    Excluding names from limited API can break existing code that use them with defined Py_LIMITED_API. I wondering if corresponding functions should be added for PySequence_ITEM, PyObject_IS_GC, PyType_SUPPORTS_WEAKREFS, PyObject_GET_WEAKREFS_LISTPTR. Perhaps this should be discussed on Python-Dev.

    Since PyList_GET_SIZE and PyList_GET_ITEM are defined only for non-limited API, it is better to wrap definitions of macros that use them (like PySequence_Fast_GET_SIZE and PySequence_Fast_GET_ITEM) in "#ifndef Py_LIMITED_API" ... "#endif".

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 8, 2018

    """Excluding names from limited API can break existing code that use them with defined Py_LIMITED_API."""

    How is that different? Right now, the code would break at compile time,
    because the macros are accessing opaque type fields. Excluding them has
    the same effect but is much cleaner.

    My current first approach is conservative, because it only makes things
    work that didn't work before.
    Core that is clearly meant as macro is obviously not meant for the limited
    API. And if it should be better included, I'm all open for it.

    Right now I want to remove quickly the breakage that was a showstopper.
    Maybe I misunderstood you and did exactly what you proposed?

    Please let us start a discussion on python-dev. I think there is more to
    do to make the limited API really usable in large projects.

    @ned-deily
    Copy link
    Member

    New changeset ea62ce7 by Ned Deily (Christian Tismer) in branch 'master':
    bpo-33738: Fix macros which contradict PEP-384 (GH-7477)
    ea62ce7

    @ned-deily
    Copy link
    Member

    As I noted on PR 7477, I decided to merge the change to master so it would get some buildbot exposure before deciding whether to backport for 3.7.0rc1 which is coming up shortly. I notice that the change introduced a compiler warning. Should that be fixed?

    ..\Objects\exceptions.c(349): warning C4090: 'return': different 'const' qualifiers [D:\buildarea\3.x.ware-win81-release\build\PCbuild\pythoncore.vcxproj]

    http://buildbot.python.org/all/#/builders/12/builds/960

    @serhiy-storchaka
    Copy link
    Member

    You are right Christian. I missed that PyTypeObject is opaque if Py_LIMITED_API is defined.

    @vstinner
    Copy link
    Member

    It seems like the change broke compilation on Windows, but I'm not sure:

    http://buildbot.python.org/all/#/builders/12/builds/959

       (Link target) -> 
         python3.def : error LNK2001: unresolved external symbol PyExceptionClass_Name [D:\buildarea\3.x.ware-win81-release\build\PCbuild\python3dll.vcxproj]
         python3.def : error LNK2001: unresolved external symbol PyIndex_Check [D:\buildarea\3.x.ware-win81-release\build\PCbuild\python3dll.vcxproj]
         python3.def : error LNK2001: unresolved external symbol PyIter_Check [D:\buildarea\3.x.ware-win81-release\build\PCbuild\python3dll.vcxproj]
         D:\buildarea\3.x.ware-win81-release\build\PCbuild\amd64\python3.lib : fatal error LNK1120: 3 unresolved externals [D:\buildarea\3.x.ware-win81-release\build\PCbuild\python3dll.vcxproj]
    

    On the buildbot-status, Ned Deily wrote:

    Build failure caused by known failure to rebuild files during compile phase. Worked around by clearing out stale files. (I *think* there is still an open issue on this.)

    Ned: you are thinking at bpo-33614, commit e97ba4c?

    @ctismer
    Copy link
    Contributor Author

    ctismer commented Jun 10, 2018

    @victor
    I cannot test on Windows because I'm in vacation.
    But it is very likely similar to bpo-33614 .
    The three missing symbols which are listed in python3.def
    do clearly come into existence when the limited API is active.

    @ned-deily
    Copy link
    Member

    New changeset 8398713 by Ned Deily (Christian Tismer) in branch 'master':
    bpo-33738: Address review comments in GH bpo-7477 (GH-7585)
    8398713

    @ned-deily
    Copy link
    Member

    Sigh! I was hoping we could get this in for 3.7.0 but I think we have run out of time and we really should not be making potential user-visible API changes at this last minute. I did notice the new compile warning for the Windows non-debug build but I overlooked that some other non-Windows buildbots were getting them, too. Serihy proposes a more general fix in bpo-33818 but I don't think we should be making a change like that just prior to the release candidate.

    And, in retrospect, I should not have considered trying to fix the stable ABI support at this late date, either. It looks like it has been broken for some time now so 3.7.0 will not be any worse. At this point, we have Christian's two PRs in master now; if necessary, they could be reverted. I will bow out of this discussion and let you all figure out what is best for master/3.8. Once the changes for master are in and working, we could revisit the question of backports to 3.7 and/or 3.6 maintenance releases. I would like to both thank and apologize to Christian, in particular, and to Serhiy and everyone else who went out of their ways to try to get this in.

    Lowering priority to "critical".

    @vstinner
    Copy link
    Member

    AMD64 Windows8.1 Non-Debug 3.x buildbot is back to green.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 5cbefa9 by Serhiy Storchaka in branch 'master':
    Clean up after bpo-33738. (GH-7627)
    5cbefa9

    @ned-deily
    Copy link
    Member

    @serhiy, @christian: Is there anything more needed to be done for this issue or can we close it?

    @vstinner
    Copy link
    Member

    Ned:

    @serhiy, @christian: Is there anything more needed to be done for this issue or can we close it?

    The issue is fixed in Python 3.8, I close it.

    See also bpo-40170 "[C API] Make PyTypeObject structure an opaque structure in the public C API" which goes further.

    @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
    Labels
    3.8 only security fixes build The build process and cross-build extension-modules C modules in the Modules dir
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants