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

Undefined behaviour in Objects/complexobject.c's complex_pow #88864

Closed
Yhg1s opened this issue Jul 21, 2021 · 19 comments
Closed

Undefined behaviour in Objects/complexobject.c's complex_pow #88864

Yhg1s opened this issue Jul 21, 2021 · 19 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@Yhg1s
Copy link
Member

Yhg1s commented Jul 21, 2021

BPO 44698
Nosy @tim-one, @Yhg1s, @gpshead, @mdickinson, @ambv, @seberg, @pablogsal, @miss-islington
PRs
  • bpo-44698: Fix undefined behaviour in complex exponentiation. #27278
  • [3.10] bpo-44698: Fix undefined behaviour in complex exponentiation. (GH-27278) #27366
  • [3.9] bpo-44698: Fix undefined behaviour in complex exponentiation. (GH-27278) #27367
  • bpo-44698: Restore complex pow behaviour for small integral exponents #27772
  • [3.10] bpo-44698: Restore complex pow behaviour for small integral exponents (GH-27772) #27796
  • [3.9] bpo-44698: Restore complex pow behaviour for small integral exponents (GH-27772) #27797
  • 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/mdickinson'
    closed_at = <Date 2021-08-17.18:22:58.553>
    created_at = <Date 2021-07-21.15:06:49.955>
    labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', '3.7']
    title = "Undefined behaviour in Objects/complexobject.c's complex_pow"
    updated_at = <Date 2021-08-17.18:22:58.552>
    user = 'https://github.com/Yhg1s'

    bugs.python.org fields:

    activity = <Date 2021-08-17.18:22:58.552>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2021-08-17.18:22:58.553>
    closer = 'mark.dickinson'
    components = []
    creation = <Date 2021-07-21.15:06:49.955>
    creator = 'twouters'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44698
    keywords = ['patch']
    message_count = 19.0
    messages = ['397947', '398239', '398249', '398250', '398928', '398930', '398932', '398935', '398937', '398940', '398941', '399610', '399620', '399621', '399666', '399767', '399775', '399776', '399779']
    nosy_count = 8.0
    nosy_names = ['tim.peters', 'twouters', 'gregory.p.smith', 'mark.dickinson', 'lukasz.langa', 'seberg', 'pablogsal', 'miss-islington']
    pr_nums = ['27278', '27366', '27367', '27772', '27796', '27797']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue44698'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @Yhg1s
    Copy link
    Member Author

    Yhg1s commented Jul 21, 2021

    Objects/complexobject.c's complex_pow uses undefined behaviour, by casting a float of unknown magnitude to a long:

        int_exponent = (long)exponent.real;

    At Google we build with clang and -fsanitize=float-cast-overflow by default, which catches this particular kind of undefined behaviour. We didn't notice, however, because the only code we've come across that exercises this behaviour was a commented-out test in test_complex, which was uncommented in 3.8. Running the test, or just '1e19+1j ** 1e19', is enough to trigger the undefined behaviour. I'll prepare a PR to fix it.

    @Yhg1s Yhg1s added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes type-bug An unexpected behavior, bug, or error labels Jul 21, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Jul 26, 2021

    New changeset 1d582bb by T. Wouters in branch 'main':
    bpo-44698: Fix undefined behaviour in complex exponentiation. (GH-27278)
    1d582bb

    @ambv
    Copy link
    Contributor

    ambv commented Jul 26, 2021

    New changeset 85ac814 by Miss Islington (bot) in branch '3.9':
    bpo-44698: Fix undefined behaviour in complex exponentiation. (GH-27278) (GH-27367)
    85ac814

    @ambv
    Copy link
    Contributor

    ambv commented Jul 26, 2021

    New changeset 256d97c by Miss Islington (bot) in branch '3.10':
    bpo-44698: Fix undefined behaviour in complex exponentiation. (GH-27278) (bpo-27366)
    256d97c

    @ambv ambv closed this as completed Jul 26, 2021
    @ambv ambv closed this as completed Jul 26, 2021
    @seberg
    Copy link
    Mannequin

    seberg mannequin commented Aug 4, 2021

    The fix broke NumPy (see also numpy/numpy#19612)

    It seems incorrect. After all, it doesn't matter much whether the float can be converted to an integer correctly (or even if it returns an undefined value), so long int_value == float_value remains sensible.

    The old cast cast integers to complex when they were out of range (which is fine), the new code raises an error instead.

    @seberg
    Copy link
    Mannequin

    seberg mannequin commented Aug 4, 2021

    Hmm, sorry, I overshot/misread :(.

    The thing that the NumPy test-suite trips over is that:

        c_powi(inf+0j, 3)

    seems to not raise, but:

        _Py_c_pow(inf+0j, 3.+0j)

    (or nan+0.j rather then inf+0j)

    does seem to raise (returning nan+nanj in both cases). If this is the inf code path, raising an error may actually be fix and the integer code path should maybe also do it.

    @seberg
    Copy link
    Mannequin

    seberg mannequin commented Aug 4, 2021

    (Sorry for the spam. I think we can/should just hard-code the expected values in the NumPy test-suite. So this is not actually an issue for NumPy and probably just warrants a double-check that the behaviour change is desirable.)

    @mdickinson
    Copy link
    Member

    Thanks, @seberg. I'll take a look and see if we can restore the old behaviour, at least for Python 3.9 and 3.10. I'm not convinced that the new behaviour is objectively wrong, but I agree that the *change* in behaviour is problematic.

    I do find it a little odd that we're special-casing integer exponents for complex numbers when no such special-casing exists for floats. It's not purely for optimization (despite what the code might say). And all the messing around with trying to figure out exactly _which_ doubles can safely be cast to long (which was the original cause of this issue) seems like a symptom of a deeper problem, which is that it doesn't make a whole lot of sense to be distinguishing in the first place. But if we're going to change the behaviour, we should only be doing that in 3.11.

    OTOH, there is real value in z**2 being fast and accurate; we definitely want to keep that. (Perhaps the math and cmath modules should grow a square function.)

    @mdickinson
    Copy link
    Member

    a double-check that the behaviour change is desirable

    I'm not sure I'm brave or foolhardy enough to try to figure out what all of the special case results *should* be for complex pow. The special cases seem reasonably clear cut for everything else in the cmath module, but for pow it's pretty much going to be a nightmare. I'm not sure there's much Python can do here in the way of guarantees, other than "not obviously horribly wrong". (And while I have reasonable confidence we can achieve the "not obviously horribly wrong" goal for everything else, I still have doubts for pow.)

    @mdickinson mdickinson reopened this Aug 4, 2021
    @mdickinson mdickinson reopened this Aug 4, 2021
    @mdickinson mdickinson assigned mdickinson and unassigned Yhg1s Aug 4, 2021
    @mdickinson mdickinson self-assigned this Aug 4, 2021
    @mdickinson
    Copy link
    Member

    brave or foolhardy enough to try to figure out what all of the special case results *should* be for complex pow

    Addendum: even the C standards give up at this point. For everything else the special cases are spelled out in detail, but for cpow, from §G.6.4.1 of C17 (in Annex G), we have:

    The cpow functions raise floating-point exceptions if appropriate for the calculation of the parts of the result, and may also raise spurious floating-point exceptions.

    And that's it. (Well, not quite: there's a footnote, which says:

    This allows cpow(z, c) to be implemented as cexp(cclog(z)) without precluding implementations that treat special cases more carefully.

    )

    @seberg
    Copy link
    Mannequin

    seberg mannequin commented Aug 4, 2021

    Thanks for looking into it!

    cpow is indeed complicated. We had some discussion in NumPy about 0**y branch cuts (I did yet not finish it, because thinking about the branch cuts is tricky).

    It is almost reassuring that the C standard also hedges out :). Although, it means that there is no reliance on math libraries if we want to treat the special cases more carefully :(.

    @mdickinson
    Copy link
    Member

    @pablo: I'm adding you to the nosy list to get your opinion on whether (the backport of) #71959 can go into 3.10.0.rc2.

    Summary: #71465 (which I reviewed and approved) and its backports fixed some undefined behaviour in complex exponentiation, but also introduced a behaviour change that affected NumPy: some exponentiation expressions that returned a result in 3.10.0b4 raise an OverflowError in 3.10.0.rc1.

    #71959 reverts the behaviour change but keeps the fix for the undefined behaviour. Can the backport of #71959 to the 3.10 branch go in before 3.10.0 final?

    @pablogsal
    Copy link
    Member

    #71959 reverts the behaviour change but keeps the fix for the undefined behaviour. Can the backport of #71959 to the 3.10 branch go in before 3.10.0 final?

    Thanks for checking with me!

    Given that this is fixing a bug that we introduced in rc1, I think it makes sense to backport to fix it. I have reviewed the code in #71959 and is contained enough, so I think is a good idea to go ahead.

    Thanks Mark for all the work!

    @pablogsal
    Copy link
    Member

    I'm keeping track to the 3.10 backports to the second rc so could you add me as a reviewer for the backport?

    @mdickinson
    Copy link
    Member

    Thanks, Pablo!

    could you add me as a reviewer for the backport

    Will do! I'll waiting on review for the PR against the main branch, but hope to get the backport PR up by this coming weekend at the latest.

    @mdickinson
    Copy link
    Member

    New changeset 4b9a2dc by Mark Dickinson in branch 'main':
    bpo-44698: Restore complex pow behaviour for small integral exponents (GH-27772)
    4b9a2dc

    @mdickinson
    Copy link
    Member

    New changeset 3f81e96 by Miss Islington (bot) in branch '3.10':
    bpo-44698: Restore complex pow behaviour for small integral exponents (GH-27772) (GH-27796)
    3f81e96

    @mdickinson
    Copy link
    Member

    New changeset beb3a83 by Miss Islington (bot) in branch '3.9':
    bpo-44698: Restore complex pow behaviour for small integral exponents (GH-27772) (GH-27797)
    beb3a83

    @mdickinson
    Copy link
    Member

    Re-closing; we should be good now.

    @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.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants