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

Py_UNREACHABLE() no longer behaves as a function call #82386

Closed
vstinner opened this issue Sep 17, 2019 · 24 comments
Closed

Py_UNREACHABLE() no longer behaves as a function call #82386

vstinner opened this issue Sep 17, 2019 · 24 comments
Labels
3.9 only security fixes build The build process and cross-build interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

BPO 38205
Nosy @gpshead, @vstinner, @zware, @serhiy-storchaka, @animalize, @ammaraskar, @sir-sigurd
PRs
  • bpo-38205: replace get_small_int(ival) macro with a function version #16270
  • bpo-38205: Convert Py_UNREACHABLE() macro to a function #16280
  • bpo-38205: Py_UNREACHABLE() calls Py_FatalError() #16290
  • [3.8] bpo-38205: Py_UNREACHABLE() calls Py_FatalError() (GH-16290) #16306
  • 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 2019-10-01.10:28:14.960>
    created_at = <Date 2019-09-17.22:12:35.773>
    labels = ['interpreter-core', 'build', '3.9']
    title = 'Py_UNREACHABLE() no longer behaves as a function call'
    updated_at = <Date 2019-10-01.10:28:14.959>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2019-10-01.10:28:14.959>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-10-01.10:28:14.960>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2019-09-17.22:12:35.773>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38205
    keywords = ['patch']
    message_count = 24.0
    messages = ['352669', '352680', '352681', '352682', '352700', '352703', '352708', '352735', '352757', '352758', '352763', '352764', '352765', '352774', '352776', '352780', '352782', '352783', '352786', '352799', '352892', '352898', '353028', '353659']
    nosy_count = 7.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'zach.ware', 'serhiy.storchaka', 'malin', 'ammar2', 'sir-sigurd']
    pr_nums = ['16270', '16280', '16290', '16306']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue38205'
    versions = ['Python 3.9']

    @vstinner
    Copy link
    Member Author

    With the following change, Python no longer compiles. I'm sure that it compiled successfully 2 weeks ago.

    diff --git a/Objects/longobject.c b/Objects/longobject.c
    index 4cf2b0726e..0ad2609882 100644
    --- a/Objects/longobject.c
    +++ b/Objects/longobject.c
    @@ -16,10 +16,10 @@ class int "PyObject *" "&PyLong_Type"
     /*[clinic end generated code: output=da39a3ee5e6b4b0d input=ec0275e3422a36e3]*/
     
     #ifndef NSMALLPOSINTS
    -#define NSMALLPOSINTS           257
    +#define NSMALLPOSINTS           0
     #endif
     #ifndef NSMALLNEGINTS
    -#define NSMALLNEGINTS           5
    +#define NSMALLNEGINTS           0
     #endif
     
     _Py_IDENTIFIER(little);

    Main GCC error:

    Objects/longobject.c: In function '_PyLong_Copy':
    ./Include/pymacro.h:119:26: error: expected expression before 'do'
    119 | #define Py_UNREACHABLE() do { \
    | ^~
    Objects/longobject.c:83:30: note: in expansion of macro 'Py_UNREACHABLE'
    83 | #define get_small_int(ival) (Py_UNREACHABLE(), NULL)
    | ^~~~~~~~~~~~~~

    It seems to be a regression introduced by:

    commit 6b51998
    Author: animalize <animalize@users.noreply.github.com>
    Date: Fri Sep 6 14:00:56 2019 +0800

    replace inline function `is_small_int` with a macro version (GH-15710)
    

    @vstinner vstinner added 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) build The build process and cross-build labels Sep 17, 2019
    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Sep 18, 2019

    This commit changed Py_UNREACHABLE() five days ago:

    3ab6147

    If remove this change, it can be compiled successfully.

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Sep 18, 2019

    We can change Py_UNREACHABLE() to assert(0) in longobject.c
    Or remove the article in Py_UNREACHABLE()

    @vstinner
    Copy link
    Member Author

    @sir-sigurd
    Copy link
    Mannequin

    sir-sigurd mannequin commented Sep 18, 2019

    I believe that the problem is caused by the change in Py_UNREACHABLE() (3ab6147).

    Before the mentioned commit Py_UNREACHABLE() was an expression, now it's a block. Py_UNREACHABLE() macro is public (see https://docs.python.org/3/c-api/intro.html#c.Py_UNREACHABLE), so this change can cause similar problems outside of CPython (i.e. that change was breaking).

    @sir-sigurd
    Copy link
    Mannequin

    sir-sigurd mannequin commented Sep 18, 2019

    Also quote from Py_UNREACHABLE() doc:

    Use this in places where you might be tempted to put an assert(0) or abort() call.

    6b51998 does exactly that, it replaces assert(0) with Py_UNREACHABLE().

    @vstinner
    Copy link
    Member Author

    Before the mentioned commit Py_UNREACHABLE() was an expression, now it's a block.

    Right, it was changed by:

    commit 3ab6147
    Author: Zachary Ware <zachary.ware@gmail.com>
    Date: Thu Sep 12 13:35:48 2019 +0100

    Enhance Py_UNREACHABLE macro (GH-16032)
    

    I add Zachary Ware in the nosy list.

    @zware
    Copy link
    Member

    zware commented Sep 18, 2019

    Whoops :(. Fixing this is beyond my C skill. I'd rather not lose the easter egg, but if it can't be fixed properly then 3ab6147 should just be reverted.

    @gpshead
    Copy link
    Member

    gpshead commented Sep 18, 2019

    While I don't like how that get_small_int macro is defined...
    and I don't like that Py_UNREACHABLE() was usable as an expression in the past...

    it is probably best to just revert 3ab6147.

    The choice to use a macro for this when it was created and the way the macro was defined means it is an expression forever unless we want to bother forcing code cleanups for uses of it.

    @zware
    Copy link
    Member

    zware commented Sep 18, 2019

    I have poked at this a bit and have an implementation that defines a static inline void _Py_Unreachable() in pymacro.h, see zware@d07b425

    This works on at least macOS and Windows, and we also have prior art for defining static inline functions in headers in #54288. If we agree that that's an acceptable solution, I'll open a PR for that change.

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Sep 19, 2019

    If use static inline function, and Py_UNREACHABLE() inside an if-else branch that should return a value, compiler may emit warning:
    https://godbolt.org/z/YtcNSf

    MSVC v19.14:
    warning C4715: 'test': not all control paths return a value
    
    clang 8.0.0:
    warning: control may reach end of non-void function [-Wreturn-type]
    

    Other compilers (gcc, icc) don't emit this warning.

    This situation in real code:
    https://github.com/python/cpython/blob/v3.8.0b4/Include/object.h#L600
    https://github.com/python/cpython/blob/v3.8.0b4/Objects/longobject.c#L3088

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Sep 19, 2019

    PR 16270 use Py_UNREACHABLE() in a single line.
    It solves this particular issue.

    @ammaraskar
    Copy link
    Member

    For the control path warning, that can be fixed by just hinting to the compiler that the function doesn't return like so:

      # ifdef __GNUC__
      __attribute__ ((noreturn))
      # elif defined(_MSC_VER)
      __declspec(noreturn)
      # endif
      static inline void
      _Py_UNREACHABLE()
      {

    (https://godbolt.org/z/AG9tx_)

    @vstinner
    Copy link
    Member Author

    I like the idea of implementing Py_UNREACHABLE() as a function. Would it make sense to even declare it as a regular function rather than a static inline function? What is the benefit of inlining here? Inlining can make the code larger, rather than a function call at the machine code level is shorter.

    @vstinner
    Copy link
    Member Author

    I wrote PR 16280: "Convert Py_UNREACHABLE() macro to a function". This change fix this issue but also enhance Py_UNREACHABLE() which now dumps the Python traceback where the bug occurs. See my example:
    #16280 (comment)

    @vstinner vstinner changed the title Python no longer compiles without small integer singletons Py_UNREACHABLE() no longer behaves as a function call Sep 19, 2019
    @vstinner
    Copy link
    Member Author

    I propose to restrict this issue to Py_UNREACHABLE() macro/function.

    Please use bpo-37812 to discuss longobject.c.

    @serhiy-storchaka
    Copy link
    Member

    I prefer to keep it a macro. The compiler does not know that it is never executed, so it can generate a suboptimal code.

    While it is a macro, it can be made a no-op, or even with compiler-specific instructions like __builtin_unreachable. This can help the compiler to generate more optimal code. For example, the popular idiom:

        switch (kind) {
        case PyUnicode_1BYTE_KIND: {
            ...
            break;
        }
        case PyUnicode_2BYTE_KIND: {
            ...
            break;
        }
        case PyUnicode_4BYTE_KIND: {
            ...
            break;
        }
        default: Py_UNREACHABLE();
        }

    could be compiled to the code equivalent to:

        if (kind == PyUnicode_1BYTE_KIND) {
            ...
            break;
        }
        else if (kind == PyUnicode_2BYTE_KIND) {
            ...
            break;
        }
        else { // assuming (kind == PyUnicode_4BYTE_KIND)
            ...
            break;
        }

    @vstinner
    Copy link
    Member Author

    While it is a macro, it can be made a no-op, or even with compiler-specific instructions like __builtin_unreachable.

    My PR 16280 uses _Py_NO_RETURN which uses __attribute__((noreturn)) with GCC and clang.

    I'm not sure how __builtin_unreachable could be used with Py_UNREACHABLE() macro.

    While it is a macro, it can be made a no-op

    I understood that Py_UNREACHABLE() is used on purpose to prevent undefined behavior. For example, if a function accepts an enum, but is called with a value which is not part of the enum: what should happen? Should Python crash? Usually, we try to be nice and return an error. But sometimes, you cannot report an error and so Py_UNREACHABLE() is a good solution.

    I prefer to keep it a macro. The compiler does not know that it is never executed, so it can generate a suboptimal code.

    I don't see how PR 16280 could have an effect on that. I don't see how the compiler can guess that the code is never executed with the current macro.

    --

    Using a function allows to put a breakpoint on it.

    In fact, I can easily modify PR 16280 to keep the macro, since I only call Py_FatalError() with a string. The function body is simple.

    @sir-sigurd
    Copy link
    Mannequin

    sir-sigurd mannequin commented Sep 19, 2019

    FWIW I proposed to add Py_ASSUME() macro that uses __builtin_unreachable() in bpo-38147.

    @serhiy-storchaka
    Copy link
    Member

    _Py_NO_RETURN is a promise that the code past the function is unreachable, not that the function call is unreachable.

    I'm not sure how __builtin_unreachable could be used with Py_UNREACHABLE() macro.

    In the release mode Py_UNREACHABLE() can be replaced with __builtin_unreachable().

    For example, if a function accepts an enum, but is called with a value which is not part of the enum: what should happen? Should Python crash?

    If the accessibility of the code depends on the call from Python code, we should raise normal exception. If it is not possible to pass it out of the function, we should use PyErr_WriteUnraisable().

    If the accessibility of the code depends on the call from C code,
    we can use PyErr_BadInternalCall() or raise a SystemError directly.

    If we are in the situation where recovering from errors is not possible (for example if the memory manager is broken) we use Py_FatalError().

    If we are absolutely sure that the code never can be executed (unless memory was corrupted or something like), we use Py_UNREACHABLE(). It will silence compiler complains, fail loudly in debug mode if our assumption is wrong (this is our programming bug), and allow the compiler to optimize out the code if substituted by __builtin_unreachable() in the release mode.

    I don't see how the compiler can guess that the code is never executed with the current macro.

    But we can make the macro to expand to __builtin_unreachable in the release mode. Or just to no-op if there is other way to silence the compiler warning.

    Using a function allows to put a breakpoint on it.

    You can put a brackpoint on Py_FatalError().

    In fact, I can easily modify PR 16280 to keep the macro, since I only call Py_FatalError() with a string.

    It would be nice. We can consider using __builtin_unreachable() in different issue. I am also going to use Py_UNREACHABLE() more widely to write more uniform, but still optimal code. It could replace

        if (cond1) { ...
        } else if (cond2) { ...
        } else if (cond3) { ...
        } else {
            assert (!cond4);
            ...
        }

    with

        if (cond1) { ...
        } else if (cond2) { ...
        } else if (cond3) { ...
        } else if (cond4) { ...
        } else {
            Py_UNREACHABLE();
        }

    @vstinner
    Copy link
    Member Author

    New changeset b154258 by Victor Stinner in branch 'master':
    bpo-38205: Py_UNREACHABLE() calls Py_FatalError() (GH-16290)
    b154258

    @vstinner
    Copy link
    Member Author

    New changeset 245d439 by Victor Stinner in branch '3.8':
    bpo-38205: Py_UNREACHABLE() calls Py_FatalError() (GH-16290) (GH-16306)
    245d439

    @vstinner
    Copy link
    Member Author

    Serhiy created bpo-38249: "Optimize out Py_UNREACHABLE in the release mode".

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 1, 2019

    The initial issue has been fixed in 3.8 and master. I close the issue.

    @vstinner vstinner closed this as completed Oct 1, 2019
    @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.9 only security fixes build The build process and cross-build interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants