| msg301244 - (view) |
Author: Barry A. Warsaw (barry) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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 (vstinner) * |
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) * |
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) * |
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 |
|
|
| msg305833 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-11-08 13:11 |
| New changeset 8bf288e2c5330148e4bd07d9c2f1ccd05ced5a86 by Serhiy Storchaka (Petr Viktorin) in branch 'master': Docs: Mention that Py_UNREACHABLE was added in 3.7 (#4337) https://github.com/python/cpython/commit/8bf288e2c5330148e4bd07d9c2f1ccd05ced5a86 |
|
|
| msg305845 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-11-08 14:06 |
| New changeset 54cc0c0789af8ff2396cb19095b7ab269f2bc06c by Victor Stinner in branch 'master': bpo-31338: C API intro: add missing versionadded (#4339) https://github.com/python/cpython/commit/54cc0c0789af8ff2396cb19095b7ab269f2bc06c |
|
|