Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use abort() for code we never expect to hit #75519

Closed
warsaw opened this issue Sep 4, 2017 · 22 comments
Closed

Use abort() for code we never expect to hit #75519

warsaw opened this issue Sep 4, 2017 · 22 comments
Assignees
Labels
3.7 interpreter-core

Comments

@warsaw
Copy link
Member

@warsaw warsaw commented Sep 4, 2017

BPO 31338
Nosy @warsaw, @rhettinger, @pitrou, @vstinner, @skrah, @serhiy-storchaka
PRs
  • #3282
  • #3374
  • #4337
  • #4339
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/warsaw'
    closed_at = <Date 2017-09-15.01:13:40.432>
    created_at = <Date 2017-09-04.19:18:14.110>
    labels = ['interpreter-core', '3.7']
    title = 'Use abort() for code we never expect to hit'
    updated_at = <Date 2017-11-08.14:06:26.211>
    user = 'https://github.com/warsaw'

    bugs.python.org fields:

    activity = <Date 2017-11-08.14:06:26.211>
    actor = 'vstinner'
    assignee = 'barry'
    closed = True
    closed_date = <Date 2017-09-15.01:13:40.432>
    closer = 'barry'
    components = ['Interpreter Core']
    creation = <Date 2017-09-04.19:18:14.110>
    creator = 'barry'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31338
    keywords = []
    message_count = 22.0
    messages = ['301244', '301252', '301304', '301314', '301323', '301324', '301325', '301326', '301330', '301334', '301335', '301340', '301341', '301349', '301355', '301393', '301394', '301395', '301400', '302227', '305833', '305845']
    nosy_count = 6.0
    nosy_names = ['barry', 'rhettinger', 'pitrou', 'vstinner', 'skrah', 'serhiy.storchaka']
    pr_nums = ['3282', '3374', '4337', '4339']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue31338'
    versions = ['Python 3.7']

    @warsaw
    Copy link
    Member Author

    @warsaw warsaw commented Sep 4, 2017

    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.

    @warsaw warsaw added the 3.7 label Sep 4, 2017
    @warsaw warsaw self-assigned this Sep 4, 2017
    @warsaw warsaw added the interpreter-core label Sep 4, 2017
    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Sep 4, 2017

    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.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Sep 5, 2017

    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.

    @warsaw
    Copy link
    Member Author

    @warsaw warsaw commented Sep 5, 2017

    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.

    @warsaw
    Copy link
    Member Author

    @warsaw warsaw commented Sep 5, 2017

    @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.

    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Sep 5, 2017

    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.

    @warsaw
    Copy link
    Member Author

    @warsaw warsaw commented Sep 5, 2017

    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

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Sep 5, 2017

    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.

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Sep 5, 2017

    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.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Sep 5, 2017

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

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Sep 5, 2017

    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.

    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Sep 5, 2017

    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;

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Sep 5, 2017

    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.

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Sep 5, 2017

    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.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Sep 5, 2017

    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]

    @warsaw
    Copy link
    Member Author

    @warsaw warsaw commented Sep 5, 2017

    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.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Sep 5, 2017

    +#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.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Sep 5, 2017

    Neither gcc (macOS, Ubuntu), nor clang (Ubuntu) complain.

    Ok, cool. In that case, go ahead.

    @warsaw
    Copy link
    Member Author

    @warsaw warsaw commented Sep 5, 2017

    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.

    @warsaw
    Copy link
    Member Author

    @warsaw warsaw commented Sep 15, 2017

    New changeset b2e5794 by Barry Warsaw in branch 'master':
    bpo-31338 (bpo-3374)
    b2e5794

    @warsaw warsaw closed this as completed Sep 15, 2017
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 8, 2017

    New changeset 8bf288e by Serhiy Storchaka (Petr Viktorin) in branch 'master':
    Docs: Mention that Py_UNREACHABLE was added in 3.7 (bpo-4337)
    8bf288e

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 8, 2017

    New changeset 54cc0c0 by Victor Stinner in branch 'master':
    bpo-31338: C API intro: add missing versionadded (bpo-4339)
    54cc0c0

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 interpreter-core
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants