classification
Title: Py_UNREACHABLE() no longer behaves as a function call
Type: compile error Stage: resolved
Components: Interpreter Core Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Ma Lin, ammar2, gregory.p.smith, serhiy.storchaka, sir-sigurd, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2019-09-17 22:12 by vstinner, last changed 2019-10-01 10:28 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 16270 closed Ma Lin, 2019-09-19 01:08
PR 16280 closed vstinner, 2019-09-19 09:18
PR 16290 merged vstinner, 2019-09-19 16:25
PR 16306 merged vstinner, 2019-09-20 21:17
Messages (24)
msg352669 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-17 22:12
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 6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2
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)
msg352680 - (view) Author: Ma Lin (Ma Lin) * Date: 2019-09-18 00:03
This commit changed Py_UNREACHABLE() five days ago:

https://github.com/python/cpython/commit/3ab61473ba7f3dca32d779ec2766a4faa0657923

If remove this change, it can be compiled successfully.
msg352681 - (view) Author: Ma Lin (Ma Lin) * Date: 2019-09-18 00:21
We can change Py_UNREACHABLE() to assert(0) in longobject.c
Or remove the article in Py_UNREACHABLE()
msg352682 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-18 00:24
See https://bugs.python.org/issue37812#msg352670 and https://bugs.python.org/issue38015
msg352700 - (view) Author: Sergey Fedoseev (sir-sigurd) * Date: 2019-09-18 06:29
I believe that the problem is caused by the change in Py_UNREACHABLE() (https://github.com/python/cpython/commit/3ab61473ba7f3dca32d779ec2766a4faa0657923).

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).
msg352703 - (view) Author: Sergey Fedoseev (sir-sigurd) * Date: 2019-09-18 06:54
Also quote from Py_UNREACHABLE() doc:

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

https://github.com/python/cpython/commit/6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2 does exactly that, it replaces assert(0) with Py_UNREACHABLE().
msg352708 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-18 07:41
> Before the mentioned commit Py_UNREACHABLE() was an expression, now it's a block. 

Right, it was changed by:

commit 3ab61473ba7f3dca32d779ec2766a4faa0657923
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.
msg352735 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2019-09-18 15:56
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 3ab61473ba7f3dca32d779ec2766a4faa0657923 should just be reverted.
msg352757 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-09-18 21:41
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 3ab61473ba7f3dca32d779ec2766a4faa0657923.

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.
msg352758 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2019-09-18 21:48
I have poked at this a bit and have an implementation that defines a `static inline void _Py_Unreachable()` in pymacro.h, see https://github.com/zware/cpython/commit/d07b4255dc1170155e18db0fea99ec1cb29c2f0a

This works on at least macOS and Windows, and we also have prior art for defining static inline functions in headers in GH-10079.  If we agree that that's an acceptable solution, I'll open a PR for that change.
msg352763 - (view) Author: Ma Lin (Ma Lin) * Date: 2019-09-19 01:03
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
msg352764 - (view) Author: Ma Lin (Ma Lin) * Date: 2019-09-19 01:11
PR 16270 use Py_UNREACHABLE() in a single line.
It solves this particular issue.
msg352765 - (view) Author: Ammar Askar (ammar2) * (Python triager) Date: 2019-09-19 01:34
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_)
msg352774 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-19 08:54
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.
msg352776 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-19 09:34
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:
https://github.com/python/cpython/pull/16280#issuecomment-533049748
msg352780 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-19 09:48
I propose to restrict this issue to Py_UNREACHABLE() macro/function.

Please use bpo-37812 to discuss longobject.c.
msg352782 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-09-19 09:53
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;
    }
msg352783 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-19 10:00
> 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.
msg352786 - (view) Author: Sergey Fedoseev (sir-sigurd) * Date: 2019-09-19 10:36
FWIW I proposed to add Py_ASSUME() macro that uses __builtin_unreachable() in bpo-38147.
msg352799 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-09-19 14:59
_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();
    }
msg352892 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-20 21:10
New changeset b1542583bee204130934c2b90684041e29378250 by Victor Stinner in branch 'master':
bpo-38205: Py_UNREACHABLE() calls Py_FatalError() (GH-16290)
https://github.com/python/cpython/commit/b1542583bee204130934c2b90684041e29378250
msg352898 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-20 21:36
New changeset 245d439320e0921459502883fe809213e3b93e35 by Victor Stinner in branch '3.8':
bpo-38205: Py_UNREACHABLE() calls Py_FatalError() (GH-16290) (GH-16306)
https://github.com/python/cpython/commit/245d439320e0921459502883fe809213e3b93e35
msg353028 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-23 14:58
Serhiy created bpo-38249: "Optimize out Py_UNREACHABLE in the release mode".
msg353659 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 10:28
The initial issue has been fixed in 3.8 and master. I close the issue.
History
Date User Action Args
2019-10-01 10:28:14vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg353659

stage: patch review -> resolved
2019-09-23 14:58:27vstinnersetmessages: + msg353028
2019-09-20 21:36:35vstinnersetmessages: + msg352898
2019-09-20 21:17:37vstinnersetpull_requests: + pull_request15892
2019-09-20 21:10:19vstinnersetmessages: + msg352892
2019-09-19 16:25:52vstinnersetpull_requests: + pull_request15875
2019-09-19 14:59:53serhiy.storchakasetmessages: + msg352799
2019-09-19 10:36:41sir-sigurdsetmessages: + msg352786
2019-09-19 10:00:57vstinnersetmessages: + msg352783
2019-09-19 09:53:25serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg352782
2019-09-19 09:48:28vstinnersetmessages: + msg352780
2019-09-19 09:48:06vstinnersettitle: Python no longer compiles without small integer singletons -> Py_UNREACHABLE() no longer behaves as a function call
2019-09-19 09:34:59vstinnersetmessages: + msg352776
2019-09-19 09:18:35vstinnersetpull_requests: + pull_request15865
2019-09-19 08:54:51vstinnersetmessages: + msg352774
2019-09-19 01:34:35ammar2setnosy: + ammar2
messages: + msg352765
2019-09-19 01:11:08Ma Linsetmessages: + msg352764
2019-09-19 01:08:03Ma Linsetkeywords: + patch
stage: patch review
pull_requests: + pull_request15860
2019-09-19 01:03:41Ma Linsetmessages: + msg352763
2019-09-18 21:48:26zach.waresetmessages: + msg352758
2019-09-18 21:41:46gregory.p.smithsetmessages: + msg352757
2019-09-18 15:56:32zach.waresetnosy: + gregory.p.smith
messages: + msg352735
2019-09-18 07:41:13vstinnersetnosy: + zach.ware
messages: + msg352708
2019-09-18 06:54:17sir-sigurdsetmessages: + msg352703
2019-09-18 06:29:04sir-sigurdsetnosy: + sir-sigurd
messages: + msg352700
2019-09-18 00:24:01vstinnersetmessages: + msg352682
2019-09-18 00:21:30Ma Linsetmessages: + msg352681
2019-09-18 00:03:52Ma Linsetnosy: + Ma Lin
messages: + msg352680
2019-09-17 22:12:35vstinnercreate