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

PySlice_GetIndicesEx change broke ABI in 3.5 and 3.6 branches #74129

Closed
njsmith opened this issue Mar 29, 2017 · 27 comments
Closed

PySlice_GetIndicesEx change broke ABI in 3.5 and 3.6 branches #74129

njsmith opened this issue Mar 29, 2017 · 27 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-bug An unexpected behavior, bug, or error

Comments

@njsmith
Copy link
Contributor

njsmith commented Mar 29, 2017

BPO 29943
Nosy @arigo, @mdickinson, @larryhastings, @benjaminp, @ned-deily, @njsmith, @skrah, @serhiy-storchaka, @zooba, @MojoVampire, @stratakis, @hroncok, @jjhelmus
PRs
  • [3.5] bpo-29943: Do not replace the function PySlice_GetIndicesEx() with a macro #1049
  • [2.7] bpo-29943: Remove the PySlice_GetIndicesEx() macro. #1050
  • [3.6] bpo-29943: Do not replace the function PySlice_GetIndicesEx() with a macro (GH-1049) #1813
  • bpo-30604: Move co_extra_freefuncs to interpreter state to avoid crashes in threads #2015
  • 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/serhiy-storchaka'
    closed_at = <Date 2017-05-25.12:34:20.260>
    created_at = <Date 2017-03-29.23:36:28.041>
    labels = ['interpreter-core', 'type-bug', 'release-blocker']
    title = 'PySlice_GetIndicesEx change broke ABI in 3.5 and 3.6 branches'
    updated_at = <Date 2017-09-27.16:19:36.041>
    user = 'https://github.com/njsmith'

    bugs.python.org fields:

    activity = <Date 2017-09-27.16:19:36.041>
    actor = 'josh.r'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2017-05-25.12:34:20.260>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2017-03-29.23:36:28.041>
    creator = 'njs'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29943
    keywords = []
    message_count = 27.0
    messages = ['290796', '290797', '290806', '290815', '291118', '291331', '291406', '291408', '291743', '291746', '291979', '292276', '292543', '292544', '292715', '292716', '292718', '292726', '292727', '293434', '293452', '294367', '294390', '294474', '295481', '295487', '303101']
    nosy_count = 14.0
    nosy_names = ['arigo', 'mark.dickinson', 'larry', 'benjamin.peterson', 'ned.deily', 'njs', 'cgohlke', 'skrah', 'serhiy.storchaka', 'steve.dower', 'josh.r', 'cstratak', 'hroncok', 'jhelmus']
    pr_nums = ['1049', '1050', '1813', '2015']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29943'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Mar 29, 2017

    In the process of fixing bpo-27867, a new function PySlice_AdjustIndices was added, and PySlice_GetIndicesEx was converted into a macro that calls this new function. The patch was backported to both the 3.5 and 3.6 branches, was released in 3.6.1, and is currently slated to be released as part of 3.5.4.

    Unfortunately, this breaks our normal ABI stability guarantees for micro releases: it means that if a C module that uses PySlice_GetIndicesEx is compiled against e.g. 3.6.1, then it cannot be imported on 3.6.0. This affects a number of high-profile packages (cffi, pandas, mpi4py, dnf, ...). The only workaround is that if you are distributing binary extension modules (e.g. wheels), then you need to be careful not to upgrade to 3.6.1. It's not possible for a wheel to declare that it requires 3.6.1-or-better, because CPython normally follows the rule that we don't make these kinds of changes. Oops.

    CC'ing Ned and Larry, because it's possible this should trigger a 3.6.2, and I think it's a blocker for 3.5.4. CC'ing Serhiy as the author of the original patch, since you probably have the best idea how this could be unwound with minimal breakage :-).

    python-dev discussion: https://mail.python.org/pipermail/python-dev/2017-March/147707.html

    Fedora bug: https://bugzilla.redhat.com/show_bug.cgi?id=1435135

    @larryhastings
    Copy link
    Contributor

    Let's make it a release blocker for now.

    @serhiy-storchaka
    Copy link
    Member

    My apologies for breaking the world.

    The workaround is to undefine PySlice_GetIndicesEx after #include "Python.h".

    #if PY_VERSION_HEX < 0x03070000 && defined(PySlice_GetIndicesEx)
    #undef PySlice_GetIndicesEx
    #endif

    But this restores the initial bug.

    Other obvious solution -- declare 3.6.0 broken and require upgrading to 3.6.1.

    For 3.5.4 we can disable defining the macro when Py_LIMITED_API is not defined. This will restore the initial bug in many extensions. And we need other way to detect if we compile the CPython core or standard extensions for fixing the bug in standard collections. Or expand all uses of PySlice_GetIndicesEx to PySlice_Unpack+PySlice_AdjustIndices (the patch can be generated automatically).

    @serhiy-storchaka
    Copy link
    Member

    2.7 is affected too.

    Nathaniel, could you add references to this issue and to original bpo-27867 on the Fedora bug tracker?

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Apr 4, 2017

    Currently we haven't updated to Python 3.6.1 at Fedora 26 due to this issue.

    While it is a release blocker for 3.6.2, what can be done for 3.6.1?

    @serhiy-storchaka
    Copy link
    Member

    PR 1049 and PR 1050 restore ABI compatibility in 3.5 and 2.7. Unfortunately this restores the original bug with using PySlice_GetIndicesEx in third-party code (but CPython core and standard extensions no longer use PySlice_GetIndicesEx).

    Can we consider 3.6.0 rather than 3.6.1 as broken release?

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Apr 8, 2017
    @njsmith
    Copy link
    Contributor Author

    njsmith commented Apr 10, 2017

    Can we consider 3.6.0 rather than 3.6.1 as broken release?

    In the last week, pypi downloads were about evenly split between 3.6.0 and 3.6.1 (2269969 for "3.6.1", 1927189 for "3.6.0", and those two were ~2 orders of magnitude more common than other strings like "3.6.1+", "3.6.0b2", etc. [1]). Not sure what that to conclude from that, but certainly if people start uploading 3.6.1-only wheels right now then it will break things for a lot of end users.

    With my manylinux docker image maintainer hat on: we're currently shipping 3.6.0. I'm extremely confident that if we stick with this we'll never get any complaints about the obscure bug with malicious __index__ implementations that's being fixed here. OTOH if we upgrade to 3.6.1, or any version with this ABI change, then we'll definitely get many complaints so long as there's anyone at all still using 3.6.0, which is probably forever. So I'm not sure not sure what incentive we would have to ever upgrade to 3.6.1+ if this ABI change is kept?

    (This isn't saying the bug is unimportant! But it sure is hard to sell its importance to folks trying to ship packages and support end-users...)

    --------

    [1] Somewhat crude query I used in case it's useful for future reference:

    SELECT
    REGEXP_EXTRACT(details.python, r"^([^\\.]+\.[^\\.]+\.[^\\.]+)") as python_version,
    COUNT(*) as download_count,
    FROM
    TABLE_DATE_RANGE(
    [the-psf:pypi.downloads],
    DATE_ADD(CURRENT_TIMESTAMP(), -7, "day"),
    DATE_ADD(CURRENT_TIMESTAMP(), 0, "day")
    )
    WHERE
    REGEXP_MATCH(details.python, r"^3\.6\.")
    GROUP BY
    python_version,
    ORDER BY
    download_count DESC
    LIMIT 100

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Apr 10, 2017

    It looks like PyTorch got bitten by this: https://discuss.pytorch.org/t/pyslice-adjustindices-error/1475/11

    @serhiy-storchaka
    Copy link
    Member

    New changeset 89f9eb5 by Serhiy Storchaka in branch '2.7':
    bpo-29943: Remove the PySlice_GetIndicesEx() macro. (bpo-1050)
    89f9eb5

    @serhiy-storchaka
    Copy link
    Member

    New changeset 49a9059 by Serhiy Storchaka in branch '3.5':
    [3.5] bpo-29943: Do not replace the function PySlice_GetIndicesEx() with a macro (bpo-1049)
    49a9059

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Apr 20, 2017

    FYI: pandas-dev/pandas#16066

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Apr 25, 2017

    Apparently this also broke pyqt for multiple users; here's the maintainers at conda-forge struggling to figure out the best workaround: conda-forge/pyqt-feedstock#25

    I really think it would be good to fix this in 3.6 sooner rather than later. Downstairs projects are accumulating workarounds and pinning 3.6.0 as we speak.

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Apr 28, 2017

    Pillow also had broken wheels up on pypi for a while; they've now put out a bug fix release that #undef's PySlice_GetIndicesEx, basically monkeypatching out the bugfix to get back to the 3.6.0 behavior: python-pillow/Pillow#2479

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Apr 28, 2017

    I'm with Nathaniel here: The fixed-bug is probably too obscure to warrant ABI breakage.

    @njsmith
    Copy link
    Contributor Author

    njsmith commented May 2, 2017

    More collateral damage: apparently the workaround that Pandas used for this bug (#undef'ing PySlice_GetIndicesEx) broke PyPy, so now they need a workaround for the workaround: pandas-dev/pandas#16192

    Recording this here partly as a nudge since IIUC the breaking change is still in the 3.6 tree, and partly for the benefit of future projects that are also trying to work around this.

    @serhiy-storchaka
    Copy link
    Member

    Either 3.6.1 or 3.6.0 should be officially declared broken. If declare 3.6.1 broken we must port PR 1049 to 3.6 and release 3.6.1.1 or 3.6.2 as early as possible. Many third-party extensions built with 3.6.1 should be rebuild with a new release for compatibility with 3.6.0. Distributors should update Python to a new release. If declare 3.6.0 broken we shouldn't do anything beside this. Distributors should update Python to 3.6.1, as planned.

    Ned, what is your decision as RM?

    @njsmith
    Copy link
    Contributor Author

    njsmith commented May 2, 2017

    I don't find it helpful to think of it as declaring 3.6.0 broken vs declaring 3.6.1 broken. 3.6.0 is definitely good in the sense that if you build a module on it then it will import on both 3.6.0 and 3.6.1, and 3.6.1 is definitely good in the sense that if you're using it then you can import any module that was built on 3.6.0 or 3.6.1.

    I think 3.6.2 should combine the best aspects of 3.6.0 and 3.6.1, in the sense that modules built on 3.6.2 should be importable on any 3.6.x, and 3.6.2 should be able to import modules built on any 3.6.x.

    Fortunately this is very easy – it just means that 3.6.2 should continue to export the PySlice_AdjustIndices symbol (because existing 3.6.1 builds will need it to import on 3.6.2), but should remove the #define of PySlice_GetIndicesEx (so code built against 3.6.2 will not *require* PySlice_AdjustIndices to be present).

    Removing 3.6.0 and 3.6.1 from circulation is simply not possible; the best 3.6.2 can hope for is to interoperate with them as well as possible.

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented May 2, 2017

    For what it's worth, in Fedora 26 we already rebased Python to 3.6.1, so this issue now is non existent for our ecosystem, and we are not shipping 3.6.0 in any way now.

    @hroncok
    Copy link
    Mannequin

    hroncok mannequin commented May 2, 2017

    For what it's worth, in Fedora 26 we already rebased Python to 3.6.1, so this issue now is non existent for our ecosystem, and we are not shipping 3.6.0 in any way now.

    Except of course if 3.6.2 would be fixed in a way that 3.6.1 would be considered broken, and released after Fedora 26 is released. In that case, we would need to deal with the situation in Fedora somehow (e.g. patch it back in or so).

    @jjhelmus
    Copy link
    Mannequin

    jjhelmus mannequin commented May 10, 2017

    In Anaconda we ship both Python 3.6.0 and 3.6.1 and have run into this issue when building packages with 3.6.1 and importing them on 3.6.0. We are discussing adding #undef PySlice_GetIndicesEx to the Python.h we ship with Python 3.6.1. From the discussion here this seems like a possible solution but we are worried about ABI compatibility with future releases in the 3.6 branch. Is there a consensus on how this will be addressed in 3.6.2?

    @njsmith
    Copy link
    Contributor Author

    njsmith commented May 10, 2017

    @jonathan: Even 3.6.1 was careful to retain compatibility with code built by 3.6.0. And your proposed 3.6.1-patched will generate binaries equivalent to the ones 3.6.0 generates. So I don't think you need to worry; 3.6.2 is not going to add a new and worse compatibility break, which is what it would take for your proposal to cause a problem. In fact, your 3.6.1-patched is equivalent to what I think 3.6.2 should do. I say go for it; patching 3.6.1 seems like a good solution.

    @ned-deily
    Copy link
    Member

    Thanks everyone for the input on this issue. We've had some discussions here at PyCon US and I think the consensus is that we all agree with Nathaniel's comment above that, for 3.6.x, that modules built on 3.6.2 (and later 3.6.x) should be importable on any 3.6.x (including 3.6.0), and 3.6.2 (and later 3.6.x) should be able to import modules built on any 3.6.x. Steve Dower has suggested that it would be nice to provide a compile option so that users could compile an extension with the benefit of the 3.6.1 fix if they do not have the need for 3.6.0 compatibility. I think that would be nice but not essential. But we do need to ensure that 3.6.2 is fixed to be compatible as above prior to 3.6.2rc currently scheduled for 2017-06-26. Serhiy, is that doable?

    Otherwise, I believe no further changes are needed for the other branches, correct?

    @serhiy-storchaka
    Copy link
    Member

    I'll apply to 3.6 the same patch as for 3.5 tomorrow.

    @serhiy-storchaka serhiy-storchaka self-assigned this May 24, 2017
    @serhiy-storchaka
    Copy link
    Member

    New changeset f43b293 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-29943: Do not replace the function PySlice_GetIndicesEx() with a macro (GH-1049) (bpo-1813)
    f43b293

    @ned-deily
    Copy link
    Member

    As a followup, Nathanial, are you satisfied with the resolution here for the upcoming 3.6.2?

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Jun 9, 2017

    Looks good to me, thanks Serhiy.

    @arigo
    Copy link
    Mannequin

    arigo mannequin commented Sep 27, 2017

    An update to Serhiy's proposed fix:

    #if PY_VERSION_HEX < 0x03070000 && defined(PySlice_GetIndicesEx)
    #if !defined(PYPY_VERSION)
    #undef PySlice_GetIndicesEx
    #endif
    #endif

    All PyXxx functions are macros on PyPy, and undefining a macro just makes everything go wrong. And there is not much PyPy can do about that.

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants