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.