[Bug c++/104642] New: Add __builtin_trap() for missing return at -O0

[Bug c++/104642] New: Add __builtin_trap() for missing return at -O0

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

            Bug ID: 104642
           Summary: Add __builtin_trap() for missing return at -O0
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: redi at gcc dot gnu.org
  Target Milestone: ---

Again and again and again users are surprised (and often angry) about the
effects of a missing return in C++. It's undefined behaviour but that doesn't
stop bug reports like PR 104635 and questions like
https://gcc.gnu.org/pipermail/gcc/2022-January/238200.html

Users expect the undefined behaviour for a missing return to be "semi-defined".
We should just make it trap at -O0, then at least we can tell them that for
unoptimized code, the result is guaranteed to crash. With optimisation, it's
still undefined and can invalidate all their assumptions.

As I said in https://gcc.gnu.org/pipermail/gcc/2022-January/238204.html

What if we inserted the trap for -O0?

1. Not everybody uses ubsan even when they should use it.

2. The code can use unreachable annotations if it really needs to leave
some paths unhandled, but really can't live with the branch and trap
instructions. (The C++ standard is getting std::unreachable and std::assume
to do that in a portable way, so there is less excuse for not doing it).

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Not sure, people will still see the surprising behavior at -O1+ then and at -O0
we're not exploiting the __builtin_unreachable () in too surprising ways (we'll
just fall thru to the next function or so - heh.

I'd rather have -funreachable-traps or so, enabled by default at -O0, rather
than special-casing the return case btw.

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed,

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-03-03

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #1)
> Not sure, people will still see the surprising behavior at -O1+ then

Sure, but we can justify that as optimizing away the checks. But giving a
predictable crash at -O0 and confusing nonsense at -O1+ is easier to debug than
confusing nonsense at all levels.


> I'd rather have -funreachable-traps or so, enabled by default at -O0, rather
> than special-casing the return case btw.

That would be even better, yes.

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
PR 104884 is another "why is undefined behaviour so surprising?" case for
-funreachable-traps

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |jason at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

--- Comment #5 from Jason Merrill <jason at gcc dot gnu.org> ---
The suggested -funreachable-traps seems to have a lot of overlap with
-fsanitize-undefined-trap-on-error; I wonder about combining them, and having
it by itself imply -fsanitize=unreachable.

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:d68d366425369649cb4e25a07752e25a4fff52cf

commit r13-1204-gd68d366425369649cb4e25a07752e25a4fff52cf
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Jun 10 16:35:21 2022 -0400

    ubsan: default to trap on unreachable at -O0 and -Og [PR104642]

    When not optimizing, we can't do anything useful with unreachability in
    terms of code performance, so we might as well improve debugging by turning
    __builtin_unreachable into a trap.  I think it also makes sense to do this
    when we're explicitly optimizing for the debugging experience.

    In the PR richi suggested introducing an -funreachable-traps flag for this.
    This functionality is already implemented as -fsanitize=unreachable
    -fsanitize-trap=unreachable, and we want to share the implementation, but
it
    does seem useful to have a separate flag that isn't affected by the various
    sanitization controls.  -fsanitize=unreachable takes priority over
    -funreachable-traps if both are enabled.

    Jakub observed that this would slow down -O0 by default from running the
    sanopt pass, so this revision avoids the need for sanopt by rewriting calls
    introduced by the compiler immediately, and calls written by the user at
    fold time.  Many of the calls introduced by the compiler are also rewritten
    immediately to ubsan calls when not trapping, which fixes ubsan-8b.C;
    previously the call to f() was optimized away before sanopt.  But this
early
    rewriting isn't practical for uses of __builtin_unreachable in
    devirtualization and such, so sanopt rewriting is still done for
    non-trapping sanitize.

            PR c++/104642

    gcc/ChangeLog:

            * common.opt: Add -funreachable-traps.
            * doc/invoke.texi (-funreachable-traps): Document it.
            * opts.cc (finish_options): Enable at -O0 or -Og.
            * tree.cc (build_common_builtin_nodes): Add __builtin_trap.
            (builtin_decl_unreachable, build_builtin_unreachable): New.
            * tree.h: Declare them.
            * ubsan.cc (sanitize_unreachable_fn): Factor out.
            (ubsan_instrument_unreachable): Use
            gimple_build_builtin_unreachable.
            * ubsan.h (sanitize_unreachable_fn): Declare.
            * gimple.cc (gimple_build_builtin_unreachable): New.
            * gimple.h: Declare it.
            * builtins.cc (expand_builtin_unreachable): Add assert.
            (fold_builtin_0): Call build_builtin_unreachable.
            * sanopt.cc: Don't run for just SANITIZE_RETURN
            or SANITIZE_UNREACHABLE when trapping.
            * cgraphunit.cc (walk_polymorphic_call_targets): Use new
            unreachable functions.
            * gimple-fold.cc (gimple_fold_call)
            (gimple_get_virt_method_for_vtable)
            * ipa-fnsummary.cc (redirect_to_unreachable)
            * ipa-prop.cc (ipa_make_edge_direct_to_target)
            (ipa_impossible_devirt_target)
            * ipa.cc (walk_polymorphic_call_targets)
            * tree-cfg.cc (pass_warn_function_return::execute)
            (execute_fixup_cfg)
            * tree-ssa-loop-ivcanon.cc (remove_exits_and_undefined_stmts)
            (unloop_loops)
            * tree-ssa-sccvn.cc (eliminate_dom_walker::eliminate_stmt):
            Likewise.

    gcc/cp/ChangeLog:

            * constexpr.cc (cxx_eval_builtin_function_call): Handle
            unreachable/trap earlier.
            * cp-gimplify.cc (cp_maybe_instrument_return): Use
            build_builtin_unreachable.

    gcc/testsuite/ChangeLog:

            * g++.dg/ubsan/return-8a.C: New test.
            * g++.dg/ubsan/return-8b.C: New test.
            * g++.dg/ubsan/return-8d.C: New test.
            * g++.dg/ubsan/return-8e.C: New test.

[Bug c++/104642] Add __builtin_trap() for missing return at -O0

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104642

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |13.0

--- Comment #7 from Jason Merrill <jason at gcc dot gnu.org> ---
Implemented for GCC 13.