classification
Title: Add a macro for unreachable code
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ZackerySpytz, benjamin.peterson, ezio.melotti, georg.brandl, loewis, pitrou, vstinner
Priority: normal Keywords: easy, patch

Created on 2012-04-23 21:52 by benjamin.peterson, last changed 2019-05-29 21:38 by pablogsal. This issue is now closed.

Files
File name Uploaded Description Edit
unreachable.patch benjamin.peterson, 2012-04-24 19:10 review
Messages (19)
msg159088 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-04-23 21:52
It would be nice to have a macro Py_UNREACHABLE to keep compiler warnings away in unreachable code. This is can call __builtin_unreachable on gcc and abort elsewhere.
msg159094 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-04-23 22:12
What's the specific warning that you want to eliminate?
msg159095 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-04-23 22:19
2012/4/23 Martin v. Löwis <report@bugs.python.org>:
>
> Martin v. Löwis <martin@v.loewis.de> added the comment:
>
> What's the specific warning that you want to eliminate?

"control reaches end of non-void function without return"

Basically, whenever you see "assert(0)" today.
msg159097 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-04-23 22:45
>> What's the specific warning that you want to eliminate?
>
> "control reaches end of non-void function without return"
>
> Basically, whenever you see "assert(0)" today.

Sorry, what I meant is: what specific line (in what specific
source file) is that warning on?

If there is an assert(0) somewhere, there should be no
subsequent code, so no such warning should be generated.
msg159101 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-04-23 23:06
2012/4/23 Martin v. Löwis <report@bugs.python.org>:
>
> Martin v. Löwis <martin@v.loewis.de> added the comment:
>
>>> What's the specific warning that you want to eliminate?
>>
>> "control reaches end of non-void function without return"
>>
>> Basically, whenever you see "assert(0)" today.
>
> Sorry, what I meant is: what specific line (in what specific
> source file) is that warning on?
>
> If there is an assert(0) somewhere, there should be no
> subsequent code, so no such warning should be generated.

Right, we currently avoid this problem by writing "assert(0)" for
example in lookdict_split in dictobject.c. What I'm saying is that
instead writing "assert(0)", we could use Py_UNREACHABLE.
msg159103 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-04-23 23:45
> Right, we currently avoid this problem by writing "assert(0)" for
> example in lookdict_split in dictobject.c. What I'm saying is that
> instead writing "assert(0)", we could use Py_UNREACHABLE.

I don't understand. The assert(0) is not there to silence any compiler
warnings, is it? Instead, ISTM that it is there to truly assert that
the code is not reached, which isn't actually obvious at all (IIUC, it's
not reached because there must be some empty slot eventually unless the
key is in there already).

Instead, ISTM that is actually the "return 0;" that should silence the
compiler warning about not having a return statement.

If that's all true, I fail to see what __builtin_unreachable would achieve:
we certainly have to preserve the return statement, for compilers not  
supporting
such a declaration.

Wouldn't this actually have the undesirable effect of complaining about
the return statement when compiling with -Wunreachable-code?
msg159194 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-04-24 19:10
Let me explain what I meant with code.
msg159682 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-04-30 09:11
Please, go ahead, explain :-)
msg159690 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-04-30 13:08
Did you see the sample patch I posted?
msg159780 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-05-02 06:56
Sorry, I missed the patch. I still fail to see the problem that this solves: what compiler produces "control reaches end of non-void function without return" for the current code? ISTM that your patch has the potential of *introducing* such a warning on some compilers, since you are removing the return statement (and the compiler may not be smart enough to determine that Py_FatalError does not return).
msg159786 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-05-02 11:35
I think you misuse __builtin_unreachable(): the code *is* reachable,
it is just very unlikely.

I really prefer to return something looking valid and continue the
execution of the program, instead of calling the evil Py_FatalError()
in release mode which stops immediatly the program in an insane
manner. For example, -1 is a valid result for findchar(), so the
program execution will continue, even if the kind is invalid.
msg159788 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-02 11:37
> I really prefer to return something looking valid and continue the
> execution of the program

How would that be better? Should Python become more like PHP?
msg159789 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-05-02 11:50
Oh, I read again the patch. There are two different cases:

 * The code is really unreachable. Ex: the loop of lookdict() does never stop, return is used in the loop. Py_UNREACHABLE can be used in this case *to avoid a compiler warning*. __builtin_unreachable() helps if you have GCC, but if you don't have GCC, using return with a dummy value would be better than a Py_FatalError(). I don't think that calling Py_FatalError() does make the warning quiet.

 * The code is reachable, but if it is reached, it's a bug. Ex: switch/case on the Unicode kinde of a string. I like the current solution: assert(0) + return a almost valid value, but Antoine and Benjamin don't like the "almost valid value" (and so prefer a fatal error?). We may issue a warning instead of a fatal error (e.g. write a message into stderr with fprintf ?) in release mode.

--

Using return, it gives something like:

+#ifdef NDEBUG
+#ifdef __GNUC__
+#define Py_UNREACHABLE(value) __builtin_unreachable()
+#else
+#define Py_UNREACHABLE(value) return (value);
+#endif
+#else
+#define Py_UNREACHABLE(value) do { assert(0); return (value); } while (0)
+#endif

It cannot be used if the function has no result value (void), but quite all Python functions have a result.
msg159794 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-02 13:26
> I like the current
> solution: assert(0) + return a almost valid value, but Antoine and
> Benjamin don't like the "almost valid value" (and so prefer a fatal
> error?). We may issue a warning instead of a fatal error (e.g. write a 
> message into stderr with fprintf ?) in release mode.

If there's a bug, either an exception should be raised, or a fatal error.
We should discourage warnings on stderr (the PHP approach).
msg159797 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-05-02 15:33
> If there's a bug, either an exception should be raised, or a fatal error.
> We should discourage warnings on stderr (the PHP approach).

Agreed.

That said, I'm with Martin in that don't see how the patch in this issue helps.
msg159801 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-05-02 15:39
> Py_UNREACHABLE can be used in this case *to avoid a compiler warning*.

What is the specific warning that you want to avoid, and what specific compiler version produces it currently on what specific code? 

It's not "control reaches end of non-void function without return", is it? (since if the compiler correctly determines that the code is unreachable, it shouldn't complain that control reaches the end of the function, as it doesn't reach the end of the function).
msg159802 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-05-02 15:43
It does not avoid any warning because we have the "assert(0); return X" pattern. I was just attempting to provide a standard way of marking unreachable code.
msg159805 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-05-02 15:54
> I was just attempting to provide a standard way of marking unreachable code.

I'm -1 for the proposed patch (and probably -0 on the general idea). I think the patch has the potential *introducing* new warnings, as compilers might warn that a return is lacking in these functions. 

I'm -0 on the general idea, as I think the status quo is just fine.

As for Victor's second use case (run-time checking that supposedly-unreachable code is indeed not reached, in release mode), I'm -0 also: we check that in debug mode; this looks sufficient to me.
msg343893 - (view) Author: Zackery Spytz (ZackerySpytz) * (Python triager) Date: 2019-05-29 15:45
The Py_UNREACHABLE() macro was implemented in bpo-31338, so this issue can be closed.
History
Date User Action Args
2019-05-29 21:38:40pablogsalsetstatus: open -> closed
resolution: fixed
stage: needs patch -> resolved
2019-05-29 15:45:57ZackerySpytzsetnosy: + ZackerySpytz
messages: + msg343893
2012-05-02 15:54:04loewissetmessages: + msg159805
2012-05-02 15:43:31benjamin.petersonsetmessages: + msg159802
2012-05-02 15:39:50loewissetmessages: + msg159801
2012-05-02 15:33:48georg.brandlsetnosy: + georg.brandl
messages: + msg159797
2012-05-02 13:26:30pitrousetmessages: + msg159794
2012-05-02 11:50:06vstinnersetmessages: + msg159789
2012-05-02 11:37:46pitrousetnosy: + pitrou
messages: + msg159788
2012-05-02 11:35:18vstinnersetmessages: + msg159786
2012-05-02 06:56:37loewissetmessages: + msg159780
2012-04-30 13:08:09benjamin.petersonsetmessages: + msg159690
2012-04-30 09:11:30loewissetmessages: + msg159682
2012-04-27 07:05:44ezio.melottisetnosy: + ezio.melotti
2012-04-24 19:10:45benjamin.petersonsetfiles: + unreachable.patch
keywords: + patch
messages: + msg159194
2012-04-23 23:45:18loewissetmessages: + msg159103
2012-04-23 23:06:32benjamin.petersonsetmessages: + msg159101
2012-04-23 22:45:07loewissetmessages: + msg159097
2012-04-23 22:19:35benjamin.petersonsetmessages: + msg159095
2012-04-23 22:12:06loewissetnosy: + loewis
messages: + msg159094
2012-04-23 22:04:28vstinnersetnosy: + vstinner
2012-04-23 21:52:11benjamin.petersoncreate