Py_UNREACHABLE() no longer behaves as a function call

[vstinner]

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]

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)

[animalize]

This commit changed Py_UNREACHABLE() five days ago:

3ab6147

If remove this change, it can be compiled successfully.

[animalize]

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

[vstinner]

See https://bugs.python.org/issue37812#msg352670 and https://bugs.python.org/issue38015

[sir-sigurd]

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]

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]

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]

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]

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]

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]

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]

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

[ammaraskar]

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]

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]

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]

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

Please use bpo-37812 to discuss longobject.c.

[serhiy-storchaka]

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]

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]

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

[serhiy-storchaka]

_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]

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

[vstinner]

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

[vstinner]

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

[vstinner]

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