Issue 14656: Add a macro for unreachable code (original) (raw)

Issue14656

Created on 2012-04-23 21:52 by benjamin.peterson, last changed 2022-04-11 14:57 by admin. 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
2022-04-11 14:57:29 admin set github: 58861
2019-05-29 21:38:40 pablogsal set status: open -> closedresolution: fixedstage: needs patch -> resolved
2019-05-29 15:45:57 ZackerySpytz set nosy: + ZackerySpytzmessages: +
2012-05-02 15:54:04 loewis set messages: +
2012-05-02 15:43:31 benjamin.peterson set messages: +
2012-05-02 15:39:50 loewis set messages: +
2012-05-02 15:33:48 georg.brandl set nosy: + georg.brandlmessages: +
2012-05-02 13:26:30 pitrou set messages: +
2012-05-02 11:50:06 vstinner set messages: +
2012-05-02 11:37:46 pitrou set nosy: + pitroumessages: +
2012-05-02 11:35:18 vstinner set messages: +
2012-05-02 06:56:37 loewis set messages: +
2012-04-30 13:08:09 benjamin.peterson set messages: +
2012-04-30 09:11:30 loewis set messages: +
2012-04-27 07:05:44 ezio.melotti set nosy: + ezio.melotti
2012-04-24 19:10:45 benjamin.peterson set files: + unreachable.patchkeywords: + patchmessages: +
2012-04-23 23:45:18 loewis set messages: +
2012-04-23 23:06:32 benjamin.peterson set messages: +
2012-04-23 22:45:07 loewis set messages: +
2012-04-23 22:19:35 benjamin.peterson set messages: +
2012-04-23 22:12:06 loewis set nosy: + loewismessages: +
2012-04-23 22:04:28 vstinner set nosy: + vstinner
2012-04-23 21:52:11 benjamin.peterson create