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

[Jakub Jelinek <jakub@redhat.com>]

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