Re: [PATCH] gimple, internal-fn: Add IFN_TRAP and use it for __builtin_unreachable [PR106099]

[Richard Biener <rguenther@suse.de>]

On Wed, 27 Jul 2022, Jakub Jelinek wrote:

> Hi!
> 
> __builtin_unreachable and __ubsan_handle_builtin_unreachable don't
> use vops, they are marked const/leaf/noreturn/nothrow/cold.
> But __builtin_trap uses vops, isn't const, just leaf/noreturn/nothrow/cold.
> This is I believe so that when users explicitly use __builtin_trap in their
> sources they get stores visible at the trap side.
> -fsanitize=unreachable -fsanitize-undefined-trap-on-error used to transform
> __builtin_unreachable to __builtin_trap even in the past, but the sanopt pass
> has TODO_update_ssa, so it worked fine.
> 
> Now that gimple_build_builtin_unreachable can build a __builtin_trap call
> right away, we can run into problems that whenever we need it we would need
> to either manually or through TODO_update* ensure the vops being updated.
> 
> Though, as it is originally __builtin_unreachable which is just implemented
> as trap, I think for this case it is fine to avoid vops.  For this the
> patch introduces IFN_TRAP, which has ECF_* flags like __builtin_unreachable
> and is expanded as __builtin_trap.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I think for the sake of sanitizing unreachable as trap this is OK
but it seems this isn't actually what is done.

I still think the original change fiddling with the unreachable decl
is wrong.  Likewise unrolling shouldn't use 
gimple_build_builtin_unreachable - it isn't sanitizing anything but
telling the middle-end to DCE a path.  IMHO only few select places
where the middle-end builds unreachable () should be using this
function (which means it probably shouldn't exist), like path
isolation which IIRC uses a trap anyway.

> 2022-07-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/106099
> 	* internal-fn.def (TRAP): New internal fn.
> 	* internal-fn.h (expand_TRAP): Declare.
> 	* internal-fn.cc (expand_TRAP): Define.
> 	* gimple.cc (gimple_build_builtin_unreachable): For BUILT_IN_TRAP,
> 	use internal fn rather than builtin.
> 
> 	* gcc.dg/ubsan/pr106099.c: New test.
> 
> --- gcc/internal-fn.def.jj	2022-07-26 10:32:23.886269144 +0200
> +++ gcc/internal-fn.def	2022-07-26 11:40:41.799927048 +0200
> @@ -456,6 +456,10 @@ DEF_INTERNAL_FN (SHUFFLEVECTOR, ECF_CONS
>  /* <=> optimization.  */
>  DEF_INTERNAL_FN (SPACESHIP, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
>  
> +/* __builtin_trap created from/for __builtin_unreachable.  */
> +DEF_INTERNAL_FN (TRAP, ECF_CONST | ECF_LEAF | ECF_NORETURN
> +		       | ECF_NOTHROW | ECF_COLD, NULL)
> +
>  #undef DEF_INTERNAL_INT_FN
>  #undef DEF_INTERNAL_FLT_FN
>  #undef DEF_INTERNAL_FLT_FLOATN_FN
> --- gcc/internal-fn.h.jj	2022-06-16 10:56:28.945385251 +0200
> +++ gcc/internal-fn.h	2022-07-26 11:45:50.483837472 +0200
> @@ -242,6 +242,7 @@ extern void expand_internal_call (intern
>  extern void expand_PHI (internal_fn, gcall *);
>  extern void expand_SHUFFLEVECTOR (internal_fn, gcall *);
>  extern void expand_SPACESHIP (internal_fn, gcall *);
> +extern void expand_TRAP (internal_fn, gcall *);
>  
>  extern bool vectorized_internal_fn_supported_p (internal_fn, tree);
>  
> --- gcc/internal-fn.cc.jj	2022-07-26 10:32:23.885269157 +0200
> +++ gcc/internal-fn.cc	2022-07-26 11:42:02.611856420 +0200
> @@ -4494,3 +4494,9 @@ expand_SPACESHIP (internal_fn, gcall *st
>    if (!rtx_equal_p (target, ops[0].value))
>      emit_move_insn (target, ops[0].value);
>  }
> +
> +void
> +expand_TRAP (internal_fn, gcall *)
> +{
> +  expand_builtin_trap ();
> +}
> --- gcc/gimple.cc.jj	2022-06-27 11:18:02.680058429 +0200
> +++ gcc/gimple.cc	2022-07-26 11:57:17.049760135 +0200
> @@ -430,7 +430,16 @@ gimple_build_builtin_unreachable (locati
>  {
>    tree data = NULL_TREE;
>    tree fn = sanitize_unreachable_fn (&data, loc);
> -  gcall *g = gimple_build_call (fn, data != NULL_TREE, data);
> +  gcall *g;
> +  if (DECL_FUNCTION_CODE (fn) != BUILT_IN_TRAP)
> +    g = gimple_build_call (fn, data != NULL_TREE, data);
> +  else
> +    {
> +      /* Instead of __builtin_trap use .TRAP, so that it doesn't
> +	 need vops.  */
> +      gcc_checking_assert (data == NULL_TREE);
> +      g = gimple_build_call_internal (IFN_TRAP, 0);
> +    }
>    gimple_set_location (g, loc);
>    return g;
>  }
> --- gcc/testsuite/gcc.dg/ubsan/pr106099.c.jj	2022-07-26 12:22:26.248156163 +0200
> +++ gcc/testsuite/gcc.dg/ubsan/pr106099.c	2022-07-26 11:34:25.660909186 +0200
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/106099 */
> +/* { dg-do compile } */
> +/* { dg-options "-O -fsanitize=unreachable -fsanitize-undefined-trap-on-error -fno-tree-ccp -fno-tree-dominator-opts" } */
> +
> +void
> +foo (void)
> +{
> +  for (unsigned i = 0; i == 0; i++)
> +    ;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)