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

[C API] Remove Py_OVERFLOWED(), Py_SET_ERRNO_ON_MATH_ERROR(), Py_ADJUST_ERANGE1() #89575

Closed
vstinner opened this issue Oct 8, 2021 · 17 comments
Labels
3.11 only security fixes topic-C-API

Comments

@vstinner
Copy link
Member

vstinner commented Oct 8, 2021

BPO 45412
Nosy @tim-one, @mdickinson, @vstinner, @serhiy-storchaka
PRs
  • bpo-45412: Remove Py_SET_ERRNO_ON_MATH_ERROR() macro #28820
  • bpo-45412: Move _Py_SET_53BIT_PRECISION_START to pycore_pymath.h #28882
  • bpo-45412: Update _Py_ADJUST_ERANGE1() comment #28884
  • bpo-45412: Move copysign() define to pycore_pymath.h #28889
  • bpo-45412: Add _PY_SHORT_FLOAT_REPR macro #31171
  • 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 2021-10-11.22:12:30.265>
    created_at = <Date 2021-10-08.13:39:11.799>
    labels = ['expert-C-API', '3.11']
    title = '[C API] Remove Py_OVERFLOWED(), Py_SET_ERRNO_ON_MATH_ERROR(), Py_ADJUST_ERANGE1()'
    updated_at = <Date 2022-02-23.17:18:00.034>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2022-02-23.17:18:00.034>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-10-11.22:12:30.265>
    closer = 'vstinner'
    components = ['C API']
    creation = <Date 2021-10-08.13:39:11.799>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45412
    keywords = ['patch']
    message_count = 17.0
    messages = ['403473', '403474', '403475', '403476', '403477', '403490', '403522', '403660', '403670', '403674', '403676', '403687', '403688', '403693', '403694', '413826', '413827']
    nosy_count = 4.0
    nosy_names = ['tim.peters', 'mark.dickinson', 'vstinner', 'serhiy.storchaka']
    pr_nums = ['28820', '28882', '28884', '28889', '31171']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue45412'
    versions = ['Python 3.11']

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 8, 2021

    I propose to remove the following macros from the Python C API:

    • Py_OVERFLOWED()
    • _Py_SET_EDOM_FOR_NAN()
    • Py_SET_ERRNO_ON_MATH_ERROR()
    • Py_SET_ERANGE_IF_OVERFLOW()
    • Py_ADJUST_ERANGE1()
    • Py_ADJUST_ERANGE2()

    Only Py_ADJUST_ERANGE1() and Py_ADJUST_ERANGE2() are still used by Python itself, other macros are no longer used since Python 2.7:

    • Py_OVERFLOWED(): no longer used since Python 2.7
    • _Py_SET_EDOM_FOR_NAN(): used by Py_SET_ERRNO_ON_MATH_ERROR() which is no longer used
    • Py_SET_ERRNO_ON_MATH_ERROR(): no longer used since Python 2.6
    • Py_SET_ERANGE_IF_OVERFLOW(): no longer used since Python 2.4
    • Py_ADJUST_ERANGE1(): used by Objects/floatobject.c
    • Py_ADJUST_ERANGE2(): used by Objects/complexobject.c

    I searched for these macros in the PyPI top 5000 modules: none of these macros are used. There is a single match: frozendict-2.0.6 which contains a Include/pyport.h copy, but it doesn't use these macros.

    --

    Py_OVERFLOWED() was used by long_true_divide() and PyLong_AsDouble() in Python 2.6, but Python 2.7 no longer used them.

    (1) Py_OVERFLOWED() call in long_true_divide() was removed in Python 2.7 by bpo-1811:

    commit 4657283
    Author: Mark Dickinson <dickinsm@gmail.com>
    Date: Sun Dec 27 14:55:57 2009 +0000

    Issue bpo-1811:  Improve accuracy and consistency of true division for integers.
    

    (2) Py_OVERFLOWED() call in PyLong_AsDouble() was removed in Python 2.7 by bpo-3166:

    commit 6736cf8
    Author: Mark Dickinson <dickinsm@gmail.com>
    Date: Mon Apr 20 21:13:33 2009 +0000

    Issue bpo-3166: Make long -> float (and int -> float) conversions
    correctly rounded, using round-half-to-even.  This ensures that the
    value of float(n) doesn't depend on whether we're using 15-bit digits
    or 30-bit digits for Python longs.
    

    --

    Py_SET_ERRNO_ON_MATH_ERROR() and Py_SET_ERANGE_IF_OVERFLOW() were used in Objects/mathmodule.c in Python 2.5.

    (1) The last call to Py_SET_ERRNO_ON_MATH_ERROR() was removed by in Python 2.6 by:

    commit 6f34109
    Author: Christian Heimes <christian@cheimes.de>
    Date: Fri Apr 18 23:13:07 2008 +0000

    I finally got the time to update and merge Mark's and my trunk-math branch. The patch is collaborated work of Mark Dickinson and me. It was mostly done a few months ago. The patch fixes a lot of loose ends and edge cases related to operations with NaN, INF, very small values and complex math.
    
    The patch also adds acosh, asinh, atanh, log1p and copysign to all platforms. Finally it fixes differences between platforms like different results or exceptions for edge cases. Have fun :)
    

    (2) The last call to Py_SET_ERANGE_IF_OVERFLOW() was removed in Python 2.4 by:

    commit 77d9a3e
    Author: Hye-Shik Chang <hyeshik@gmail.com>
    Date: Mon Mar 22 08:43:55 2004 +0000

    Patch bpo-871657: Set EDOM for `nan' return values on FreeBSD and OpenBSD.
    This fixes a problem that math.sqrt(-1) doesn't raise math.error.
    

    @vstinner vstinner added 3.11 only security fixes topic-C-API labels Oct 8, 2021
    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 8, 2021

    Py_OVERFLOWED() documentation says that the function is not reliable since C99. Python is using C99 since Python 3.6.

    /* Py_OVERFLOWED(X)

    • Return 1 iff a libm function overflowed. Set errno to 0 before calling
    • a libm function, and invoke this macro after, passing the function
    • result.
    • Caution:
    • This isn't reliable. C99 no longer requires libm to set errno under
    •    any exceptional condition, but does require +- HUGE_VAL return
      
    •    values on overflow.  A 754 box \*probably* maps HUGE_VAL to a
      
    •    double infinity, and we're cool if that's so, unless the input
      
    •    was an infinity and an infinity is the expected result.  A C89
      
    •    system sets errno to ERANGE, so we check for that too.  We're
      
    •    out of luck if a C99 754 box doesn't map HUGE_VAL to +Inf, or
      
    •    if the returned result is a NaN, or if a C89 box returns HUGE_VAL
      
    •    in non-overflow cases.
      
    • X is evaluated more than once.
    • Some platforms have better way to spell this, so expect some #ifdef'ery.
    • OpenBSD uses 'isinf()' because a compiler bug on that platform causes
    • the longer macro version to be mis-compiled. This isn't optimal, and
    • should be removed once a newer compiler is available on that platform.
    • The system that had the failure was running OpenBSD 3.2 on Intel, with
    • gcc 2.95.3.
    • According to Tim's checkin, the FreeBSD systems use isinf() to work
    • around a FPE bug on that platform.
      */

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 8, 2021

    Py_SET_ERRNO_ON_MATH_ERROR() documentation also says that it is not reliable:

    /* Py_SET_ERRNO_ON_MATH_ERROR(x)

    • If a libm function did not set errno, but it looks like the result
    • overflowed or not-a-number, set errno to ERANGE or EDOM. Set errno
    • to 0 before calling a libm function, and invoke this macro after,
    • passing the function result.
    • Caution:
    • This isn't reliable. See Py_OVERFLOWED comments.
    • X is evaluated more than once.
      */

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 8, 2021

    Py_ADJUST_ERANGE1() is used by float ** float.

    Py_ADJUST_ERANGE2() is used by complex ** complex.

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 8, 2021

    I add Mark Dickinson and Tim Peters who were involved in changes removing usage of these macros.

    @serhiy-storchaka
    Copy link
    Member

    And I have doubts about Py_ADJUST_ERANGE2(). I think that it is used incorrectly. See bpo-44970.

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 9, 2021

    And I have doubts about Py_ADJUST_ERANGE2(). I think that it is used incorrectly. See bpo-44970.

    The question here is if it should be exported in the C API. I propose to remove it.

    Fixing it is a different issue: bpo-44970 :-)

    @mdickinson
    Copy link
    Member

    +1 for the removals. (We should fix bpo-44970 too, but as you say that's a separate issue. And I suspect that the Py_ADJUST_ERANGE1() use for float pow should be replaced, too.)

    @vstinner
    Copy link
    Member Author

    New changeset 2f92e2a by Victor Stinner in branch 'main':
    bpo-45412: Remove Py_SET_ERRNO_ON_MATH_ERROR() macro (GH-28820)
    2f92e2a

    @vstinner
    Copy link
    Member Author

    +1 for the removals. (We should fix bpo-44970 too, but as you say that's a separate issue. And I suspect that the Py_ADJUST_ERANGE1() use for float pow should be replaced, too.)

    Well, it's scary of use functions which are documented as:

    • Caution:
    • This isn't reliable. See Py_OVERFLOWED comments.
    • X and Y may be evaluated more than once.

    @vstinner
    Copy link
    Member Author

    • This isn't reliable. See Py_OVERFLOWED comments.

    Oops, Py_OVERFLOWED() has been removed: I created PR 28884 to fix the comment.

    @vstinner
    Copy link
    Member Author

    New changeset a9fe1a8 by Victor Stinner in branch 'main':
    bpo-45412: Update _Py_ADJUST_ERANGE1() comment (GH-28884)
    a9fe1a8

    @vstinner
    Copy link
    Member Author

    New changeset 7103356 by Victor Stinner in branch 'main':
    bpo-45412: Move _Py_SET_53BIT_PRECISION_START to pycore_pymath.h (GH-28882)
    7103356

    @vstinner
    Copy link
    Member Author

    New changeset 61190e0 by Victor Stinner in branch 'main':
    bpo-45412: Move copysign() define to pycore_pymath.h (GH-28889)
    61190e0

    @vstinner
    Copy link
    Member Author

    Include/pymath.h is now better, I close the issue ;-)

    @vstinner
    Copy link
    Member Author

    New changeset 9bbdde2 by Victor Stinner in branch 'main':
    bpo-45412: Add _PY_SHORT_FLOAT_REPR macro (GH-31171)
    9bbdde2

    @vstinner
    Copy link
    Member Author

    See also bpo-46670: "Build Python with -Wundef: don't use undefined macros".

    @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.11 only security fixes topic-C-API
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants