[RFA] 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.  In the PR richi suggested introducing an
-funreachable-traps flag for this, but this functionality is already
implemented as -fsanitize=unreachable -fsanitize-undefined-trap-on-error, we
just need to set those flags by default.

I think it also makes sense to do this when we're explicitly optimizing for
the debugging experience.

I then needed to make options-save handle -fsanitize and
-fsanitize-undefined-trap-on-error; since -fsanitize is has custom parsing,
that meant handling it explicitly in the awk scripts.  I also noticed we
weren't setting it in opts_set.

Do we still want -funreachable-traps as an alias (or separate flag) for this
behavior that doesn't mention the sanitizer?

Tested x86_64-pc-linux-gnu, OK for trunk?

	PR c++/104642

gcc/ChangeLog:

	* doc/invoke.texi (-fsanitize-undefined-trap-on-error):
	On by default at -O0, implying -fsanitize=unreachable,return
	* opts.cc (finish_options): At -O0 trap on unreachable code.
	(common_handle_option): Set opts_set->x_flag_sanitize.
	* common.opt (fsanitize-undefined-trap-on-error): Add
	Optimization tag.
	* optc-save-gen.awk, opth-gen.awk: Include flag_sanitize.

gcc/testsuite/ChangeLog:

	* g++.dg/ubsan/return-8a.C: New test.
---
 gcc/doc/invoke.texi                    |  4 ++++
 gcc/common.opt                         |  2 +-
 gcc/opts.cc                            | 10 ++++++++++
 gcc/testsuite/g++.dg/ubsan/return-8a.C | 17 +++++++++++++++++
 gcc/optc-save-gen.awk                  |  8 ++++++--
 gcc/opth-gen.awk                       |  3 ++-
 6 files changed, 40 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ubsan/return-8a.C


base-commit: 13ea4a6e830da1f245136601e636dec62e74d1a7
prerequisite-patch-id: f75da3aa4e66d8b85562d5dd9ae35c5429c1ea74

On Mon, Jun 13, 2022 at 03:53:13PM -0400, Jason Merrill via Gcc-patches wrote:
> 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.  In the PR richi suggested introducing an
> -funreachable-traps flag for this, but this functionality is already
> implemented as -fsanitize=unreachable -fsanitize-undefined-trap-on-error, we
> just need to set those flags by default.
> 
> I think it also makes sense to do this when we're explicitly optimizing for
> the debugging experience.
> 
> I then needed to make options-save handle -fsanitize and
> -fsanitize-undefined-trap-on-error; since -fsanitize is has custom parsing,
> that meant handling it explicitly in the awk scripts.  I also noticed we
> weren't setting it in opts_set.
> 
> Do we still want -funreachable-traps as an alias (or separate flag) for this
> behavior that doesn't mention the sanitizer?

I do not like doing it this way, -fsanitize-undefined-trap-on-error is
(unfortunately for compatibility with llvm misdesign, users should have
ways to control which of the enabled sanitizers should be handled which way,
where the 3 ways are runtime diagnostics without abort, runtime diagnostics
with abort and __builtin_trap ()) an all or nothing option which affects all
the sanitizers.  Your patch will e.g. silently enable the sanitization
whenever just -fsanitize-undefined-trap-on-error is passed, but that can be
passed even when users don't really expect any sanitization, just making
sure that if it is enabled (say for selected TUs or functions), it doesn't
need a runtime library to report it.
Furthermore, handling it the UBSan way means we slow down the compiler
(enable a bunch of extra passes, like sanopt, ubsan), which is undesirable
e.g. for -O0 compilation speed.

So, I think -funreachable-traps should be a separate flag and not an alias,
enabled by default for -O0 and -Og, which would be handled elsewhere
(I'd say e.g. in fold_builtin_0 and perhaps gimple_fold_builtin too to
avoid allocating trees unnecessary) and would be done if
flag_unreachable_traps && !sanitize_flag_p (SANITIZE_UNREACHABLE),
just replacing that __builtin_unreachable call with __builtin_trap.
For the function ends in fact under those conditions we could emit
__builtin_trap right away instead of emitting __builtin_unreachable
and waiting on folding it later to __builtin_trap.

	Jakub

On 6/14/22 07:44, Jakub Jelinek wrote:
> On Mon, Jun 13, 2022 at 03:53:13PM -0400, Jason Merrill via Gcc-patches wrote:
>> 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.  In the PR richi suggested introducing an
>> -funreachable-traps flag for this, but this functionality is already
>> implemented as -fsanitize=unreachable -fsanitize-undefined-trap-on-error, we
>> just need to set those flags by default.
>>
>> I think it also makes sense to do this when we're explicitly optimizing for
>> the debugging experience.
>>
>> I then needed to make options-save handle -fsanitize and
>> -fsanitize-undefined-trap-on-error; since -fsanitize is has custom parsing,
>> that meant handling it explicitly in the awk scripts.  I also noticed we
>> weren't setting it in opts_set.
>>
>> Do we still want -funreachable-traps as an alias (or separate flag) for this
>> behavior that doesn't mention the sanitizer?
> 
> I do not like doing it this way, -fsanitize-undefined-trap-on-error is
> (unfortunately for compatibility with llvm misdesign, users should have
> ways to control which of the enabled sanitizers should be handled which way,
> where the 3 ways are runtime diagnostics without abort, runtime diagnostics
> with abort and __builtin_trap ()) an all or nothing option which affects all
> the sanitizers.

Makes sense.  The first is -fsanitize-recover=, I think, the second is 
the default, and perhaps the third could be -fsanitize-trap=?

> Your patch will e.g. silently enable the sanitization
> whenever just -fsanitize-undefined-trap-on-error is passed, but that can be
> passed even when users don't really expect any sanitization, just making
> sure that if it is enabled (say for selected TUs or functions), it doesn't
> need a runtime library to report it.

I'm happy to drop that part.

> Furthermore, handling it the UBSan way means we slow down the compiler
> (enable a bunch of extra passes, like sanopt, ubsan), which is undesirable
> e.g. for -O0 compilation speed.

The ubsan pass is not enabled for unreachable|return.  sanopt does a 
single pass over the function to rewrite __builtin_unreachable, but that 
doesn't seem like much overhead.

> So, I think -funreachable-traps should be a separate flag and not an alias,
> enabled by default for -O0 and -Og, which would be handled elsewhere
> (I'd say e.g. in fold_builtin_0 and perhaps gimple_fold_builtin too to
> avoid allocating trees unnecessary)

I tried this approach, but it misses some __builtin_unreachable calls 
added by e.g. execute_fixup_cfg; it seems they never get folded by any 
subsequent pass.

It seems to me that we want -funreachable-traps (or whatever spelling) 
to have exactly the same effect as the current 
-fsanitize=unreachable,return -fsanitize-undefined-trap-on-error, so 
using an entirely novel implementation strategy seems wrong.

OTOH, maybe the sanitizer should also hook into fold_builtin_0 to 
rewrite many of the calls before any of the optimizers run, so we don't 
need to explicitly check SANITIZE_UNREACHABLE in e.g. 
optimize_unreachable.  And I notice that we currently optimize away the 
call to f() in

bool b;
int f() {
   if (b) return 42;
   __builtin_unreachable ();
} 

int main() { f(); }

with -fsanitize=unreachable -O, so the test exits normally.

> and would be done if
> flag_unreachable_traps && !sanitize_flag_p (SANITIZE_UNREACHABLE),
> just replacing that __builtin_unreachable call with __builtin_trap.
> For the function ends in fact under those conditions we could emit
> __builtin_trap right away instead of emitting __builtin_unreachable
> and waiting on folding it later to __builtin_trap.

Sure, but I generally prefer to change fewer places.

Jason

On Wed, Jun 15, 2022 at 04:38:49PM -0400, Jason Merrill wrote:
> > I do not like doing it this way, -fsanitize-undefined-trap-on-error is
> > (unfortunately for compatibility with llvm misdesign, users should have
> > ways to control which of the enabled sanitizers should be handled which way,
> > where the 3 ways are runtime diagnostics without abort, runtime diagnostics
> > with abort and __builtin_trap ()) an all or nothing option which affects all
> > the sanitizers.
> 
> Makes sense.  The first is -fsanitize-recover=, I think, the second is the
> default, and perhaps the third could be -fsanitize-trap=?

-f{,no-}sanitize-recover= is a bitmask, whether some sanitizer recovers or
not.  The default is that ubsan sanitizers except for unreachable and return
default to recover on and similarly kasan and hwkasan, other sanitizers
default to no recover.
If we add -f{,no-}sanitize-trap= similar way, we'd need to figure out
what it means if a both bits are set (i.e. we are asked to recover and trap
at the same time).  We could error, or silently prefer trap over recover,
etc.
And we'd need to define interaction with -fsanitize-undefined-trap-on-error,
would that be effectively an alias for -fsanitize-trap=all (but if so,
we'd need the silent trap takes priority over recover way)?

> > Furthermore, handling it the UBSan way means we slow down the compiler
> > (enable a bunch of extra passes, like sanopt, ubsan), which is undesirable
> > e.g. for -O0 compilation speed.
> 
> The ubsan pass is not enabled for unreachable|return.  sanopt does a single

You're right.

> pass over the function to rewrite __builtin_unreachable, but that doesn't
> seem like much overhead.

But I think we are trying to avoid hard any kind of unnecessary whole IL
extra walks, especially for -O0.

> > So, I think -funreachable-traps should be a separate flag and not an alias,
> > enabled by default for -O0 and -Og, which would be handled elsewhere
> > (I'd say e.g. in fold_builtin_0 and perhaps gimple_fold_builtin too to
> > avoid allocating trees unnecessary)
> 
> I tried this approach, but it misses some __builtin_unreachable calls added
> by e.g. execute_fixup_cfg; it seems they never get folded by any subsequent
> pass.

We could also expand BUILT_IN_UNREACHABLE as BUILT_IN_TRAP during expansion
to catch whatever isn't caught by folding.

> > and would be done if
> > flag_unreachable_traps && !sanitize_flag_p (SANITIZE_UNREACHABLE),
> > just replacing that __builtin_unreachable call with __builtin_trap.
> > For the function ends in fact under those conditions we could emit
> > __builtin_trap right away instead of emitting __builtin_unreachable
> > and waiting on folding it later to __builtin_trap.
> 
> Sure, but I generally prefer to change fewer places.

I'd say this would be very small change and the fastest + most reliable.
Simply replace all builtin_decl_implicit (BUILT_IN_UNREACHABLE) calls
with builtin_decl_unreachable () (12 of them) and define
tree
builtin_decl_unreachable ()
{
  enum built_in_function fncode = BUILT_IN_UNREACHABLE;

  if (sanitize_flag_p (SANITIZE_UNREACHABLE))
    {
      if (flag_sanitize_undefined_trap_on_error)
	fncode = BUILT_IN_TRAP;
      /* Otherwise we want __builtin_unreachable () later folded into
	 __ubsan_handle_builtin_unreachable with extra args.  */
    }
  else if (flag_unreachable_traps)
    fncode = BUILT_IN_TRAP;
  return builtin_decl_implicit (fncode);
}
and that's it (well, also in build_common_builtin_nodes
declare __builtin_trap for FEs that don't do that - like it is done
for __builtin_unreachable).

	Jakub

On Tue, 14 Jun 2022, 12:44 Jakub Jelinek, <jakub@redhat.com> wrote:

> On Mon, Jun 13, 2022 at 03:53:13PM -0400, Jason Merrill via Gcc-patches
> wrote:
> > 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.  In the PR richi suggested
> introducing an
> > -funreachable-traps flag for this, but this functionality is already
> > implemented as -fsanitize=unreachable
> -fsanitize-undefined-trap-on-error, we
> > just need to set those flags by default.
> >
> > I think it also makes sense to do this when we're explicitly optimizing
> for
> > the debugging experience.
> >
> > I then needed to make options-save handle -fsanitize and
> > -fsanitize-undefined-trap-on-error; since -fsanitize is has custom
> parsing,
> > that meant handling it explicitly in the awk scripts.  I also noticed we
> > weren't setting it in opts_set.
> >
> > Do we still want -funreachable-traps as an alias (or separate flag) for
> this
> > behavior that doesn't mention the sanitizer?
>
> I do not like doing it this way, -fsanitize-undefined-trap-on-error is
> (unfortunately for compatibility with llvm misdesign, users should have
> ways to control which of the enabled sanitizers should be handled which
> way,
> where the 3 ways are runtime diagnostics without abort, runtime diagnostics
> with abort and __builtin_trap ()) an all or nothing option which affects
> all
> the sanitizers.


It looks like clang has addressed this deficiency now:

https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#usage

You can choose a different outcome for different checks.

They also have a smaller, intended-for-production runtime now:

https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#minimal-runtime

On Thu, Jun 16, 2022 at 09:32:02PM +0100, Jonathan Wakely wrote:
> It looks like clang has addressed this deficiency now:
> 
> https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#usage

Thanks, will study how it works tomorrow.

	Jakub

On 6/16/22 09:14, Jakub Jelinek wrote:
> On Wed, Jun 15, 2022 at 04:38:49PM -0400, Jason Merrill wrote:
>>> Furthermore, handling it the UBSan way means we slow down the compiler
>>> (enable a bunch of extra passes, like sanopt, ubsan), which is undesirable
>>> e.g. for -O0 compilation speed.
>>
>> The ubsan pass is not enabled for unreachable|return.  sanopt does a single
> 
> You're right.
> 
>> pass over the function to rewrite __builtin_unreachable, but that doesn't
>> seem like much overhead.
> 
> But I think we are trying to avoid hard any kind of unnecessary whole IL
> extra walks, especially for -O0.

OK.

>>> So, I think -funreachable-traps should be a separate flag and not an alias,
>>> enabled by default for -O0 and -Og, which would be handled elsewhere
>>> (I'd say e.g. in fold_builtin_0 and perhaps gimple_fold_builtin too to
>>> avoid allocating trees unnecessary)
>>
>> I tried this approach, but it misses some __builtin_unreachable calls added
>> by e.g. execute_fixup_cfg; it seems they never get folded by any subsequent
>> pass.
> 
> We could also expand BUILT_IN_UNREACHABLE as BUILT_IN_TRAP during expansion
> to catch whatever isn't caught by folding.

That was an early thing I tried, but that's too late to prevent it from 
being used for optimization.  More recently I've put an assert in 
expand_builtin_unreachable to catch ones that slip past.

>>> and would be done if
>>> flag_unreachable_traps && !sanitize_flag_p (SANITIZE_UNREACHABLE),
>>> just replacing that __builtin_unreachable call with __builtin_trap.
>>> For the function ends in fact under those conditions we could emit
>>> __builtin_trap right away instead of emitting __builtin_unreachable
>>> and waiting on folding it later to __builtin_trap.
>>
>> Sure, but I generally prefer to change fewer places.
> 
> I'd say this would be very small change and the fastest + most reliable.
> Simply replace all builtin_decl_implicit (BUILT_IN_UNREACHABLE) calls
> with builtin_decl_unreachable () (12 of them) and define
> tree
> builtin_decl_unreachable ()
> {
>    enum built_in_function fncode = BUILT_IN_UNREACHABLE;
> 
>    if (sanitize_flag_p (SANITIZE_UNREACHABLE))
>      {
>        if (flag_sanitize_undefined_trap_on_error)
> 	fncode = BUILT_IN_TRAP;
>        /* Otherwise we want __builtin_unreachable () later folded into
> 	 __ubsan_handle_builtin_unreachable with extra args.  */
>      }
>    else if (flag_unreachable_traps)
>      fncode = BUILT_IN_TRAP;
>    return builtin_decl_implicit (fncode);
> }
> and that's it (well, also in build_common_builtin_nodes
> declare __builtin_trap for FEs that don't do that - like it is done
> for __builtin_unreachable).

OK, here's another version of the patch using that approach.

On Mon, Jun 20, 2022 at 04:30:51PM -0400, Jason Merrill wrote:
I'd still prefer to see a separate -funreachable-traps.
The thing is that -fsanitize{,-recover,-trap}= are global options, not per
function (and only tweaked by no_sanitize attribute), while something
that needs to depend on the per-function -O0/-Og setting is necessarily per
function.  The *.awk changes I understand make -fsanitize= kind of per
function but -fsanitize-{recover,trap}= remain global, that is going to be a
nightmare especially with LTO which saves/restores the per function flags
and for the global ones merges them across TUs.
By separating sanitizers (which would remain global with no_sanitize
overrides) from -funreachable-traps which would be Optimization option
(with default set if unset in default_options_optimization or so)
saved/restored upon function changes that issue is gone.

> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -5858,6 +5858,11 @@ builtin_decl_implicit (enum built_in_function fncode)
>    return builtin_info[uns_fncode].decl;
>  }
>  
> +/* For BUILTIN_UNREACHABLE, use one of these instead of one of the above.  */
> +extern tree builtin_decl_unreachable ();
> +extern gcall *gimple_build_builtin_unreachable (location_t);
> +extern tree build_builtin_unreachable (location_t);

I think we generally try to declare functions in the header with same
basename as the source file in which they are defined.
So, the question is if builtin_decl_unreachable and build_builtin_unreachable
shouldn't be defined in tree.cc and declared in tree.h and
gimple_build_builtin_unreachable in gimple.cc and declared in gimple.h,
using a helper defined in ubsan.cc and declared in ubsan.h (your current
unreachable_1).

> +
>  /* Set explicit builtin function nodes and whether it is an implicit
>     function.  */
>  
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> --- a/gcc/cgraphunit.cc
> +++ b/gcc/cgraphunit.cc
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> --- a/gcc/cp/cp-gimplify.cc
> +++ b/gcc/cp/cp-gimplify.cc
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> --- a/gcc/ipa.cc
> +++ b/gcc/ipa.cc

The above changes LGTM.
>  	  if (dump_enabled_p ())
>  	    {
> diff --git a/gcc/opts.cc b/gcc/opts.cc
> index 959d48d173f..d92699a1bc9 100644
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -1122,6 +1122,17 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
>        opts->x_flag_no_inline = 1;
>      }
>  
> +  /* At -O0 or -Og, turn __builtin_unreachable into a trap.  */
> +  if (!opts_set->x_flag_sanitize)
> +    {
> +      if (!opts->x_optimize || opts->x_optimize_debug)
> +	opts->x_flag_sanitize = SANITIZE_UNREACHABLE|SANITIZE_RETURN;
> +
> +      /* Change this without regard to optimization level so we don't need to
> +	 deal with it in optc-save-gen.awk.  */
> +      opts->x_flag_sanitize_trap = SANITIZE_UNREACHABLE|SANITIZE_RETURN;
> +    }
> +
>    /* Pipelining of outer loops is only possible when general pipelining
>       capabilities are requested.  */
>    if (!opts->x_flag_sel_sched_pipelining)

See above.

> --- a/gcc/sanopt.cc
> +++ b/gcc/sanopt.cc
> @@ -942,7 +942,15 @@ public:
>    {}
>  
>    /* opt_pass methods: */
> -  virtual bool gate (function *) { return flag_sanitize; }
> +  virtual bool gate (function *)
> +  {
> +    /* SANITIZE_RETURN is handled in the front-end.  When trapping,
> +       SANITIZE_UNREACHABLE is handled by builtin_decl_unreachable.  */
> +    unsigned int mask = SANITIZE_RETURN;

There are other sanitizers purely handled in the FEs, guess as a follow-up
we should look at which of them don't really need any sanopt handling.

> +    if (flag_sanitize_trap & SANITIZE_UNREACHABLE)
> +      mask |= SANITIZE_UNREACHABLE;
> +    return flag_sanitize & ~mask;
> +  }
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> --- a/gcc/tree-ssa-loop-ivcanon.cc
> +++ b/gcc/tree-ssa-loop-ivcanon.cc
> --- a/gcc/tree-ssa-sccvn.cc
> +++ b/gcc/tree-ssa-sccvn.cc
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc

LGTM.

> --- a/gcc/ubsan.cc
> +++ b/gcc/ubsan.cc
> @@ -638,27 +638,84 @@ ubsan_create_data (const char *name, int loccnt, const location_t *ploc, ...)
>    return var;
>  }
>  
> -/* Instrument the __builtin_unreachable call.  We just call the libubsan
> -   routine instead.  */
> +/* The built-in decl to use to mark code points believed to be unreachable.
> +   Typically __builtin_unreachable, but __builtin_trap if
> +   -fsanitize=unreachable -fsanitize-trap=unreachable.  If only
> +   -fsanitize=unreachable, we rely on sanopt to replace any calls with the
> +   appropriate ubsan function.  When building a call directly, use
> +   {gimple_},build_builtin_unreachable instead.  */
> +
> +tree
> +builtin_decl_unreachable ()
> +{
> +  enum built_in_function fncode = BUILT_IN_UNREACHABLE;
> +
> +  if (sanitize_flags_p (SANITIZE_UNREACHABLE))
> +    {
> +      if (flag_sanitize_trap & SANITIZE_UNREACHABLE)
> +	fncode = BUILT_IN_TRAP;
> +      /* Otherwise we want __builtin_unreachable () later folded into
> +	 __ubsan_handle_builtin_unreachable with extra args.  */
> +    }

I'd add the flag_unreachable_traps stuff here as else

> +/* Shared between *build_builtin_unreachable.  */
> +
> +static void
> +unreachable_1 (tree &fn, tree &data, location_t loc)

Besides the potential moving, I think the coding guidelines don't recommend
using references that way.  But even if it is used, wouldn't it be better
to return fn instead of void and just set data (either using reference
or taking address of data)?

	Jakub

On 6/21/22 07:17, Jakub Jelinek wrote:
> On Mon, Jun 20, 2022 at 04:30:51PM -0400, Jason Merrill wrote:
> I'd still prefer to see a separate -funreachable-traps.
> The thing is that -fsanitize{,-recover,-trap}= are global options, not per
> function (and only tweaked by no_sanitize attribute), while something
> that needs to depend on the per-function -O0/-Og setting is necessarily per
> function.  The *.awk changes I understand make -fsanitize= kind of per
> function but -fsanitize-{recover,trap}= remain global, that is going to be a
> nightmare especially with LTO which saves/restores the per function flags
> and for the global ones merges them across TUs.
> By separating sanitizers (which would remain global with no_sanitize
> overrides) from -funreachable-traps which would be Optimization option
> (with default set if unset in default_options_optimization or so)
> saved/restored upon function changes that issue is gone.

Done.

>> --- a/gcc/tree.h
>> +++ b/gcc/tree.h
>> @@ -5858,6 +5858,11 @@ builtin_decl_implicit (enum built_in_function fncode)
>>     return builtin_info[uns_fncode].decl;
>>   }
>>   
>> +/* For BUILTIN_UNREACHABLE, use one of these instead of one of the above.  */
>> +extern tree builtin_decl_unreachable ();
>> +extern gcall *gimple_build_builtin_unreachable (location_t);
>> +extern tree build_builtin_unreachable (location_t);
> 
> I think we generally try to declare functions in the header with same
> basename as the source file in which they are defined.
> So, the question is if builtin_decl_unreachable and build_builtin_unreachable
> shouldn't be defined in tree.cc and declared in tree.h and
> gimple_build_builtin_unreachable in gimple.cc and declared in gimple.h,
> using a helper defined in ubsan.cc and declared in ubsan.h (your current
> unreachable_1).

Done.

>> +
>>   /* Set explicit builtin function nodes and whether it is an implicit
>>      function.  */
>>   
>> --- a/gcc/builtins.cc
>> +++ b/gcc/builtins.cc
>> --- a/gcc/cgraphunit.cc
>> +++ b/gcc/cgraphunit.cc
>> --- a/gcc/cp/constexpr.cc
>> +++ b/gcc/cp/constexpr.cc
>> --- a/gcc/cp/cp-gimplify.cc
>> +++ b/gcc/cp/cp-gimplify.cc
>> --- a/gcc/gimple-fold.cc
>> +++ b/gcc/gimple-fold.cc
>> --- a/gcc/ipa-fnsummary.cc
>> +++ b/gcc/ipa-fnsummary.cc
>> --- a/gcc/ipa-prop.cc
>> +++ b/gcc/ipa-prop.cc
>> --- a/gcc/ipa.cc
>> +++ b/gcc/ipa.cc
> 
> The above changes LGTM.
>>   	  if (dump_enabled_p ())
>>   	    {
>> diff --git a/gcc/opts.cc b/gcc/opts.cc
>> index 959d48d173f..d92699a1bc9 100644
>> --- a/gcc/opts.cc
>> +++ b/gcc/opts.cc
>> @@ -1122,6 +1122,17 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
>>         opts->x_flag_no_inline = 1;
>>       }
>>   
>> +  /* At -O0 or -Og, turn __builtin_unreachable into a trap.  */
>> +  if (!opts_set->x_flag_sanitize)
>> +    {
>> +      if (!opts->x_optimize || opts->x_optimize_debug)
>> +	opts->x_flag_sanitize = SANITIZE_UNREACHABLE|SANITIZE_RETURN;
>> +
>> +      /* Change this without regard to optimization level so we don't need to
>> +	 deal with it in optc-save-gen.awk.  */
>> +      opts->x_flag_sanitize_trap = SANITIZE_UNREACHABLE|SANITIZE_RETURN;
>> +    }
>> +
>>     /* Pipelining of outer loops is only possible when general pipelining
>>        capabilities are requested.  */
>>     if (!opts->x_flag_sel_sched_pipelining)
> 
> See above.
> 
>> --- a/gcc/sanopt.cc
>> +++ b/gcc/sanopt.cc
>> @@ -942,7 +942,15 @@ public:
>>     {}
>>   
>>     /* opt_pass methods: */
>> -  virtual bool gate (function *) { return flag_sanitize; }
>> +  virtual bool gate (function *)
>> +  {
>> +    /* SANITIZE_RETURN is handled in the front-end.  When trapping,
>> +       SANITIZE_UNREACHABLE is handled by builtin_decl_unreachable.  */
>> +    unsigned int mask = SANITIZE_RETURN;
> 
> There are other sanitizers purely handled in the FEs, guess as a follow-up
> we should look at which of them don't really need any sanopt handling.
> 
>> +    if (flag_sanitize_trap & SANITIZE_UNREACHABLE)
>> +      mask |= SANITIZE_UNREACHABLE;
>> +    return flag_sanitize & ~mask;
>> +  }
>> --- a/gcc/tree-cfg.cc
>> +++ b/gcc/tree-cfg.cc
>> --- a/gcc/tree-ssa-loop-ivcanon.cc
>> +++ b/gcc/tree-ssa-loop-ivcanon.cc
>> --- a/gcc/tree-ssa-sccvn.cc
>> +++ b/gcc/tree-ssa-sccvn.cc
>> --- a/gcc/tree.cc
>> +++ b/gcc/tree.cc
> 
> LGTM.
> 
>> --- a/gcc/ubsan.cc
>> +++ b/gcc/ubsan.cc
>> @@ -638,27 +638,84 @@ ubsan_create_data (const char *name, int loccnt, const location_t *ploc, ...)
>>     return var;
>>   }
>>   
>> -/* Instrument the __builtin_unreachable call.  We just call the libubsan
>> -   routine instead.  */
>> +/* The built-in decl to use to mark code points believed to be unreachable.
>> +   Typically __builtin_unreachable, but __builtin_trap if
>> +   -fsanitize=unreachable -fsanitize-trap=unreachable.  If only
>> +   -fsanitize=unreachable, we rely on sanopt to replace any calls with the
>> +   appropriate ubsan function.  When building a call directly, use
>> +   {gimple_},build_builtin_unreachable instead.  */
>> +
>> +tree
>> +builtin_decl_unreachable ()
>> +{
>> +  enum built_in_function fncode = BUILT_IN_UNREACHABLE;
>> +
>> +  if (sanitize_flags_p (SANITIZE_UNREACHABLE))
>> +    {
>> +      if (flag_sanitize_trap & SANITIZE_UNREACHABLE)
>> +	fncode = BUILT_IN_TRAP;
>> +      /* Otherwise we want __builtin_unreachable () later folded into
>> +	 __ubsan_handle_builtin_unreachable with extra args.  */
>> +    }
> 
> I'd add the flag_unreachable_traps stuff here as else
> 
>> +/* Shared between *build_builtin_unreachable.  */
>> +
>> +static void
>> +unreachable_1 (tree &fn, tree &data, location_t loc)
> 
> Besides the potential moving, I think the coding guidelines don't recommend
> using references that way.  But even if it is used, wouldn't it be better
> to return fn instead of void and just set data (either using reference
> or taking address of data)?

Done.

On Tue, Jun 21, 2022 at 11:59:56PM -0400, Jason Merrill wrote:
> 	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.

LGTM, thanks.

	Jakub