classification
Title: Use abort() for code we never expect to hit
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: barry Nosy List: barry, haypo, pitrou, rhettinger, serhiy.storchaka, skrah
Priority: normal Keywords:

Created on 2017-09-04 19:18 by barry, last changed 2017-09-15 01:13 by barry. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 3282 barry, 2017-09-05 16:23
PR 3374 merged barry, 2017-09-06 00:49
Messages (20)
msg301244 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-04 19:18
Over in bpo-31337 the observation was made that we often use the following pattern in situations we never expect to hit:

assert(0);
return NULL;

but this isn't strictly optimal.  First, the asserts can be compiled away.  Second, it's possible that our assumptions about a particular condition are incorrect.  Third, the intent of non-reachability isn't as clear as it could be.

As suggested in http://bugs.python.org/issue31337#msg301229 it would be better to use

abort() /* NOT REACHED */

instead (although @skrah says "The only drawback is that in the case of libraries, sometimes distribution package lint tools complain." so it would be useful to understand that in more detail.

@serhiy.storchaka says "I have counted 48 occurrences of assert(0), 11 assert(0 && "message") and 2 assert(!"message"). If fix one occurrence, why not fix all others?"  We should!  This issue tracks that.
msg301252 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-09-04 20:07
Regarding lint warnings, I may have confused abort() with exit().

Lintian has the shlib-calls-exit tag, somehow I thought there was
a similar one for abort(), but I can't find it now.
msg301304 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-05 10:43
The fact that assert() is compiled out in release build looks as an advantage to me. This allows the compiler to generate smaller code. I'm in favor of using assert(!"message"), but this form is rarely used in CPython sources.

I think it would be nice to ask on Python-Dev first. Maybe there are other concerns.
msg301314 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-05 14:49
I'm thinking there are two aspects to this.  One would involve updates to PEP 7 to include a section on "Unreachable code".  The other would be a PR that updates the current C code to the PEP 7 standard.

I'll work on a PEP update as a separate PR, then we can discuss further on python-dev.  If the PEP changes are accepted, then we can manage the code changes as a PR against this issue.
msg301323 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-05 16:11
@skrah - quick question.  Is /* NOT REACHED */ a common convention?  Do any compilers or IDEs recognize it?  Is it documented anywhere?  I like the idea of adding that comment on the abort(), but I'm trying to find some prior art or references for that.
msg301324 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-09-05 16:14
Can we have a Py_UNREACHABLE() macro for that, then?
First, it makes the intent extra clear without needing any additional comment.  Second, it can be defined however we need in order to get along with the various tools around.
msg301325 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-05 16:17
> 
> Can we have a Py_UNREACHABLE() macro for that, then?
> First, it makes the intent extra clear without needing any additional comment.  Second, it can be defined however we need in order to get along with the various tools around.

+1
msg301326 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-09-05 16:27
> Is /* NOT REACHED */ a common convention?

I think it's often used in BSD, I first saw it in OpenBSD.


A macro is fine of course.
msg301330 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-09-05 16:36
And Solaris lint recognizes it:

   https://docs.oracle.com/cd/E60778_01/pdf/E60745.pdf

Actually it could be that the convention comes right from K&R, but I
can't find my copy right now.
msg301334 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-05 16:51
Can we use compiler-specific code like GCC's __builtin_unreachable()? This would help the optimizer.
msg301335 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-05 17:10
> As suggested in http://bugs.python.org/issue31337#msg301229 it would be better to use
> abort() /* NOT REACHED */

Please don't use abort(), but add a new Py_UNREACHABLE() macro to allow to use a different code depending on the compiler/platform and compiler option (like release vs debug build).

> Can we use compiler-specific code like GCC's __builtin_unreachable()? This would help the optimizer.

That's a good example of better implementation for Py_UNREACHABLE().

The tricky part is to make compiler warnings quiet. For example, you cannot replace "abort(); return NULL;" with "abort()", because a function without return would emit a warning.

Maybe the default implementation of the macro should be:

#define Py_UNREACHABLE(stmt) abort(); stmt

I don't know if it would work.
msg301340 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-09-05 17:16
Le 05/09/2017 à 19:10, STINNER Victor a écrit :
> 
> Maybe the default implementation of the macro should be:
> 
> #define Py_UNREACHABLE(stmt) abort(); stmt

Or simply you would write:

    Py_UNREACHABLE();
    return NULL;
msg301341 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-05 17:18
> Or simply you would write:
>
>    Py_UNREACHABLE();
>     return NULL;

I recall that I had to fix warnings when using clang on:

Py_FatalError("...");
return NULL;

See bpo-28152.

I don't know if it's related to this issue.
msg301349 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-09-05 18:17
On Tue, Sep 05, 2017 at 05:10:24PM +0000, STINNER Victor wrote:
> That's a good example of better implementation for Py_UNREACHABLE().
> 
> The tricky part is to make compiler warnings quiet. For example, you cannot replace "abort(); return NULL;" with "abort()", because a function without return would emit a warning.

Which compiler needs more than "abort();" in a default statement? gcc and clang
don't.
msg301355 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-05 18:24
> Which compiler needs more than "abort();" in a default statement? gcc and clang don't.

Again, I'm not sure that bpo-28152 is directly related to this issue, but here an example:

$ git diff

diff --git a/Parser/grammar.c b/Parser/grammar.c
index 75fd5b9cde..2b7da68929 100644
--- a/Parser/grammar.c
+++ b/Parser/grammar.c
@@ -139,13 +139,6 @@ findlabel(labellist *ll, int type, const char *str)
     }
     fprintf(stderr, "Label %d/'%s' not found\n", type, str);
     Py_FatalError("grammar.c:findlabel()");
-
-    /* Py_FatalError() is declared with __attribute__((__noreturn__)).
-       GCC emits a warning without "return 0;" (compiler bug!), but Clang is
-       smarter and emits a warning on the return... */
-#ifndef __clang__
-    return 0; /* Make gcc -Wall happy */
-#endif
 }
 
 /* Forward */

$ make

Parser/grammar.c: In function '_Py_findlabel':
Parser/grammar.c:142:1: warning: control reaches end of non-void function [-Wreturn-type]
msg301393 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-05 23:02
So with this diff:

modified   Include/pymacro.h
@@ -95,4 +95,6 @@
 #define Py_UNUSED(name) _unused_ ## name
 #endif
 
+#define Py_UNREACHABLE() abort()
+
 #endif /* Py_PYMACRO_H */
modified   Python/compile.c
@@ -1350,8 +1350,7 @@ get_const_value(expr_ty e)
     case NameConstant_kind:
         return e->v.NameConstant.value;
     default:
-        assert(!is_const(e));
-        return NULL;
+        Py_UNREACHABLE();
     }
 }
 

Neither gcc (macOS, Ubuntu), nor clang (Ubuntu) complain.
msg301394 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-05 23:06
+#define Py_UNREACHABLE() abort()

Using such macro, I don't think that __builtin_unreachable() is useful.

https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

Another use for __builtin_unreachable is following a call a function that never returns but that is not declared __attribute__((noreturn)), as in this example: 

      (...)
      function_that_never_returns ();
      __builtin_unreachable ();

I expect abort() to be annotated with __attribute__((noreturn)) on the C library used GCC.
msg301395 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-05 23:07
> Neither gcc (macOS, Ubuntu), nor clang (Ubuntu) complain.

Ok, cool. In that case, go ahead.
msg301400 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-05 23:30
On Sep 5, 2017, at 16:07, STINNER Victor <report@bugs.python.org> wrote:
> 
> STINNER Victor added the comment:
> 
>> Neither gcc (macOS, Ubuntu), nor clang (Ubuntu) complain.
> 
> Ok, cool. In that case, go ahead.

I checked with @steve.dower and I think abort() will work on MSVC too.  He did have the idea to #define it to `Py_FatalError(“some message”); abort();` but since the former calls the latter we could get warnings that the second abort() isn’t reachable.

I say we start with abort() as the expansion and go from there.  Since it’s a macro it’s easy to redefine if you want to crank up debugging, add a breakpoint, add __LINE__ and __FILE__ or need something special for some particular compiler.
msg302227 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2017-09-15 01:13
New changeset b2e5794870eb4728ddfaafc0f79a40299576434f by Barry Warsaw in branch 'master':
bpo-31338 (#3374)
https://github.com/python/cpython/commit/b2e5794870eb4728ddfaafc0f79a40299576434f
History
Date User Action Args
2017-09-15 01:13:40barrysetstatus: open -> closed
resolution: fixed
stage: resolved
2017-09-15 01:13:18barrysetmessages: + msg302227
2017-09-06 00:49:32barrysetpull_requests: + pull_request3385
2017-09-05 23:30:45barrysetmessages: + msg301400
2017-09-05 23:07:05hayposetmessages: + msg301395
2017-09-05 23:06:48hayposetmessages: + msg301394
2017-09-05 23:02:19barrysetmessages: + msg301393
2017-09-05 18:24:29hayposetmessages: + msg301355
2017-09-05 18:17:40skrahsetmessages: + msg301349
2017-09-05 17:18:26hayposetmessages: + msg301341
2017-09-05 17:16:11pitrousetmessages: + msg301340
2017-09-05 17:10:24hayposetmessages: + msg301335
2017-09-05 16:51:39serhiy.storchakasetmessages: + msg301334
2017-09-05 16:36:01skrahsetmessages: + msg301330
2017-09-05 16:27:40skrahsetmessages: + msg301326
2017-09-05 16:23:42barrysetpull_requests: + pull_request3358
2017-09-05 16:17:18barrysetmessages: + msg301325
2017-09-05 16:14:48pitrousetnosy: + pitrou
messages: + msg301324
2017-09-05 16:11:15barrysetmessages: + msg301323
2017-09-05 14:49:59barrysetmessages: + msg301314
2017-09-05 10:43:53serhiy.storchakasetnosy: + rhettinger, haypo
messages: + msg301304
2017-09-04 20:07:17skrahsetmessages: + msg301252
2017-09-04 19:18:14barrycreate