Navigation Menu

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

Python can now use the C99 NAN constant or __builtin_nan() #90798

Closed
vstinner opened this issue Feb 4, 2022 · 17 comments
Closed

Python can now use the C99 NAN constant or __builtin_nan() #90798

vstinner opened this issue Feb 4, 2022 · 17 comments
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

vstinner commented Feb 4, 2022

BPO 46640
Nosy @tim-one, @rhettinger, @mdickinson, @vstinner, @bitdancer, @encukou
PRs
  • bpo-46640: Py_NAN now uses the C99 NAN constant #31134
  • Files
  • test_nan_bits.py
  • 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 2022-02-25.15:09:43.947>
    created_at = <Date 2022-02-04.20:24:09.387>
    labels = ['interpreter-core', '3.11']
    title = 'Python can now use the C99 NAN constant or __builtin_nan()'
    updated_at = <Date 2022-02-25.15:09:43.946>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2022-02-25.15:09:43.946>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-02-25.15:09:43.947>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2022-02-04.20:24:09.387>
    creator = 'vstinner'
    dependencies = []
    files = ['50603']
    hgrepos = []
    issue_num = 46640
    keywords = ['patch']
    message_count = 17.0
    messages = ['412531', '412533', '412536', '412538', '412539', '412540', '412542', '412576', '412581', '412582', '412624', '412626', '412728', '412743', '412747', '412748', '414002']
    nosy_count = 6.0
    nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'vstinner', 'r.david.murray', 'petr.viktorin']
    pr_nums = ['31134']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue46640'
    versions = ['Python 3.11']

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    While debugging a GCC regression (*) on "HUGE_VAL * 0" used by Py_NAN macro, I noticed that Python can now C99 "NAN" constant.

    (*) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104389

    In bpo-45440, I already removed legacy code for pre-C99 support and old platforms:

    "Building Python now requires a C99 <math.h> header file providing the following functions: copysign(), hypot(), isfinite(), isinf(), isnan(), round()."

    Attached patch modifies Py_NAN to simply reuse NAN.

    mathmodule.c and cmathmodule.c m_nan() still use _Py_dg_stdnan() by default (if PY_NO_SHORT_FLOAT_REPR is not defined).

    @vstinner vstinner added 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Feb 4, 2022
    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    The Py_NAN has a special implementation for the ICC compiler, if __INTEL_COMPILER and ICC_NAN_STRICT macros are defined, in bpo-21167:
    ---
    commit edbc28c
    Author: R David Murray <rdmurray@bitdance.com>
    Date: Thu Aug 13 09:58:07 2015 -0400

    bpo-21167: Fix definition of NAN when ICC used without -fp-model strict.
    
    Patch from Chris Hogan of Intel, reviewed by Mark Dickinson.
    

    ---

    I don't know if it should be kept if Py_NAN is modified to use the NAN constant. In case of doubt, I prefer to remove the ICC code since we have no ICC buildbot anymore and maybe ICC changed in the meanwhile.

    Also, I don't have acess to ICC.

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    mathmodule.c and cmathmodule.c m_nan() still use _Py_dg_stdnan() by default (if PY_NO_SHORT_FLOAT_REPR is not defined).

    These functions are only use to create the following constants:

    • math.nan
    • cmath.nan
    • cmath.nanj

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    Manual test to check if m_nan(), _Py_dg_stdnan(0) and Py_NAN are exactly the same number (same bits):

    $ ./python
    >>> import math, struct
    >>> m_nan=math.nan; Py_NAN=math.atan2(m_nan, 1.0)
    >>> Py_NAN is m_nan
    False
    >>> struct.pack('d', m_nan) == struct.pack('d', Py_NAN)
    True
    >>> struct.pack('d', Py_NAN)
    b'\x00\x00\x00\x00\x00\x00\xf8\x7f'

    => see attached script: test_nan_bits.py

    "struct.pack('d', m_nan) == struct.pack('d', Py_NAN)" is true with #75317 on Fedora 35 with gcc-11.2.1-7.fc35.x86_64. I tested with "gcc -O0" and "gcc -O3".

    GCC float.h defines NAN with:

        #define NAN (__builtin_nanf (""))

    GCC: "Built-in Function: double __builtin_nan (const char *str): This is an implementation of the ISO C99 function nan."

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    Python/dtoa.c uses:

    /* Standard NaN used by _Py_dg_stdnan. */

    #define NAN_WORD0 0x7ff80000
    #define NAN_WORD1 0

    /* Return a 'standard' NaN value.

    There are exactly two quiet NaNs that don't arise by 'quieting' signaling
    NaNs (see IEEE 754-2008, section 6.2.1). If sign == 0, return the one whose
    sign bit is cleared. Otherwise, return the one whose sign bit is set.
    */

    double
    _Py_dg_stdnan(int sign)
    {
        U rv;
        word0(&rv) = NAN_WORD0;
        word1(&rv) = NAN_WORD1;
        if (sign)
            word0(&rv) |= Sign_bit;
        return dval(&rv);
    }

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    see attached script: test_nan_bits.py

    test_nan_bits.py says "True" for Python built with "clang -O3" and with "clang -O0".

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 4, 2022

    Using clang -E, I see that clang also replaces NAN with: __builtin_nanf ("").

    @mdickinson
    Copy link
    Member

    The big blocker here is that a platform that fully supports C99 might not define the "NAN" macro. I don't think we can require that NAN be defined in order for Python to build (which is what the PR currently does, if I'm understanding it correctly).

    Python deliberately doesn't assume IEEE 754 floating-point. By requiring that the C "NAN" macro is present to be able to build Python, we'd be effectively requiring IEEE 754 by stealth. (No other common floating-point format has NaNs.)

    I'd be fully on board with a decision to require IEEE 754 floating-point for Python in future, but that decision would at least need a python-dev discussion - we shouldn't sneak it in by the back door.

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 5, 2022

    The big blocker here is that a platform that fully supports C99 might not define the "NAN" macro. I don't think we can require that NAN be defined in order for Python to build (which is what the PR currently does, if I'm understanding it correctly).

    If a platform doesn't implement NaN, it should define the Py_NO_NAN macro:

    /* Py_NAN
     * A value that evaluates to a NaN. On IEEE 754 platforms INF*0 or
     * INF/INF works. Define Py_NO_NAN in pyconfig.h if your platform
     * doesn't support NaNs.
     */
    #if !defined(Py_NAN) && !defined(Py_NO_NAN)
       // Use C99 "NAN" constant: quiet Not-A-Number (when supported)
    #  define Py_NAN NAN
    #endif

    Or do you mean that a platform can support NaN but don't define the <math.h> NAN macro?

    @mdickinson
    Copy link
    Member

    If a platform doesn't implement NaN, it should define the Py_NO_NAN macro

    Ah. In that case your PR description (and the PR news entry) is misleading:

    Building Python now requires a C99 <math.h> header file providing the
    NAN constant.

    Please could you update them?

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 6, 2022

    New changeset 54842e4 by Victor Stinner in branch 'main':
    bpo-46640: Py_NAN now uses the C99 NAN constant (GH-31134)
    54842e4

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 6, 2022

    I merged my change, thanks for the reviews.

    @vstinner vstinner closed this as completed Feb 6, 2022
    @vstinner vstinner changed the title Python can now use the C99 NAN constant Python can now use the C99 NAN constant or __builtin_nan() Feb 6, 2022
    @vstinner vstinner closed this as completed Feb 6, 2022
    @vstinner vstinner changed the title Python can now use the C99 NAN constant Python can now use the C99 NAN constant or __builtin_nan() Feb 6, 2022
    @encukou
    Copy link
    Member

    encukou commented Feb 7, 2022

    Adding new C99 features needs a change in PEP-7 (https://www.python.org/dev/peps/pep-0007/#c-dialect)

    @encukou encukou reopened this Feb 7, 2022
    @encukou encukou reopened this Feb 7, 2022
    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 7, 2022

    Adding new C99 features needs a change in PEP-7 (https://www.python.org/dev/peps/pep-0007/#c-dialect)

    IMO this PEP is outdated for a long time.

    C99 standard is wide. Do we have to explicitly list every single function, macro or constant used by Python? It doesn't sound reasonable to me. IMO saying that we use "C99 except of these few features: <...>" would be closer to the reality. I don't know which features are not used.

    Well, if you ask me, I would simply require a C99 compiler. That's all :-)

    Note: Python uses C11 <stdatomic.h>, but it remains an optional requirement.

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 7, 2022

    Well, if you ask me, I would simply require a C99 compiler. That's all :-)

    Done in python/peps#2309

    @encukou
    Copy link
    Member

    encukou commented Feb 7, 2022

    IMO this PEP is outdated for a long time.

    It is not. Even if it is, it should be marked as such, and that is not a decision that should be done in this issue.

    Please, don't break the rules because you think they're outdated.

    Well, if you ask me, I would simply require a C99 compiler. That's all :-)

    Please propose that change. Perhaps it would be a good change to make, but I don't even know how to determine that.
    Nor can I list the places where the change should be made -- at least there should be a What's New entry like https://docs.python.org/3.10/whatsnew/3.6.html#build-and-c-api-changes

    Note: Python uses C11 <stdatomic.h>, but it remains an optional requirement.

    That's fine. You can still build with an older compiler.

    @vstinner
    Copy link
    Member Author

    PEP-7 has been updated, I close the issue.

    @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 interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants