c++: Further fix for get_member_function_from_ptrfunc [PR117259]

  Hi!

The following testcase shows that the previous get_member_function_from_ptrfunc
changes weren't sufficient and we still have cases where
-fsanitize=undefined with pointers to member functions can cause wrong code
being generated and related false positive warnings.

The problem is that save_expr doesn't always create SAVE_EXPR, it can skip
some invariant arithmetics and in the end it could be really large
expressions which would be evaluated several times (and what is worse, with
-fsanitize=undefined those expressions then can have SAVE_EXPRs added to
their subparts for -fsanitize=bounds or -fsanitize=null or
-fsanitize=alignment instrumentation).  Tried to just build1 a SAVE_EXPR
+ add TREE_SIDE_EFFECTS instead of save_expr, but that doesn't work either,
because cp_fold happily optimizes those SAVE_EXPRs away when it sees
SAVE_EXPR operand is tree_invariant_p.

So, the following patch instead of using save_expr or building SAVE_EXPR
manually builds a TARGET_EXPR.  Both types are pointers, so it doesn't need
to be destroyed in any way, but TARGET_EXPR is what doesn't get optimized
away immediately.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-10-22  Jakub Jelinek  <jakub@redhat.com>

	PR c++/117259
	* typeck.cc (get_member_function_from_ptrfunc): Use force_target_expr
	rather than save_expr for instance_ptr and function.  Don't call it
	for TREE_CONSTANT.

	* g++.dg/ubsan/pr117259.C: New test.


	Jakub
  

On 10/22/24 2:17 PM, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase shows that the previous get_member_function_from_ptrfunc
> changes weren't sufficient and we still have cases where
> -fsanitize=undefined with pointers to member functions can cause wrong code
> being generated and related false positive warnings.
> 
> The problem is that save_expr doesn't always create SAVE_EXPR, it can skip
> some invariant arithmetics and in the end it could be really large
> expressions which would be evaluated several times (and what is worse, with
> -fsanitize=undefined those expressions then can have SAVE_EXPRs added to
> their subparts for -fsanitize=bounds or -fsanitize=null or
> -fsanitize=alignment instrumentation).  Tried to just build1 a SAVE_EXPR
> + add TREE_SIDE_EFFECTS instead of save_expr, but that doesn't work either,
> because cp_fold happily optimizes those SAVE_EXPRs away when it sees
> SAVE_EXPR operand is tree_invariant_p.

Hmm, when is that be a problem?  I wouldn't expect instance_ptr to be 
tree_invariant_p.

> So, the following patch instead of using save_expr or building SAVE_EXPR
> manually builds a TARGET_EXPR.  Both types are pointers, so it doesn't need
> to be destroyed in any way, but TARGET_EXPR is what doesn't get optimized
> away immediately.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

> 2024-10-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/117259
> 	* typeck.cc (get_member_function_from_ptrfunc): Use force_target_expr
> 	rather than save_expr for instance_ptr and function.  Don't call it
> 	for TREE_CONSTANT.
> 
> 	* g++.dg/ubsan/pr117259.C: New test.
> 
> --- gcc/cp/typeck.cc.jj	2024-10-16 14:42:58.835725318 +0200
> +++ gcc/cp/typeck.cc	2024-10-22 16:12:58.462731292 +0200
> @@ -4193,24 +4193,27 @@ get_member_function_from_ptrfunc (tree *
>         if (!nonvirtual && is_dummy_object (instance_ptr))
>   	nonvirtual = true;
>   
> -      /* Use save_expr even when instance_ptr doesn't have side-effects,
> -	 unless it is a simple decl (save_expr won't do anything on
> -	 constants), so that we don't ubsan instrument the expression
> -	 multiple times.  See PR116449.  */
> +      /* Use force_target_expr even when instance_ptr doesn't have
> +	 side-effects, unless it is a simple decl or constant, so
> +	 that we don't ubsan instrument the expression multiple times.
> +	 Don't use save_expr, as save_expr can avoid building a SAVE_EXPR
> +	 and building a SAVE_EXPR manually can be optimized away during
> +	 cp_fold.  See PR116449 and PR117259.  */
>         if (TREE_SIDE_EFFECTS (instance_ptr)
> -	  || (!nonvirtual && !DECL_P (instance_ptr)))
> -	{
> -	  instance_save_expr = save_expr (instance_ptr);
> -	  if (instance_save_expr == instance_ptr)
> -	    instance_save_expr = NULL_TREE;
> -	  else
> -	    instance_ptr = instance_save_expr;
> -	}
> +	  || (!nonvirtual
> +	      && !DECL_P (instance_ptr)
> +	      && !TREE_CONSTANT (instance_ptr)))
> +	instance_ptr = instance_save_expr
> +	  = force_target_expr (TREE_TYPE (instance_ptr), instance_ptr,
> +			       complain);
>   
>         /* See above comment.  */
>         if (TREE_SIDE_EFFECTS (function)
> -	  || (!nonvirtual && !DECL_P (function)))
> -	function = save_expr (function);
> +	  || (!nonvirtual
> +	      && !DECL_P (function)
> +	      && !TREE_CONSTANT (function)))
> +	function
> +	  = force_target_expr (TREE_TYPE (function), function, complain);
>   
>         /* Start by extracting all the information from the PMF itself.  */
>         e3 = pfn_from_ptrmemfunc (function);
> --- gcc/testsuite/g++.dg/ubsan/pr117259.C.jj	2024-10-22 17:00:52.156114344 +0200
> +++ gcc/testsuite/g++.dg/ubsan/pr117259.C	2024-10-22 17:05:20.470324367 +0200
> @@ -0,0 +1,13 @@
> +// PR c++/117259
> +// { dg-do compile }
> +// { dg-options "-Wuninitialized -fsanitize=undefined" }
> +
> +struct A { void foo () {} };
> +struct B { void (A::*b) (); B (void (A::*x) ()) : b(x) {}; };
> +const B c[1] = { &A::foo };
> +
> +void
> +foo (A *x, int y)
> +{
> +  (x->*c[y].b) ();
> +}
> 
> 	Jakub
>
  

On Wed, Oct 23, 2024 at 12:27:32PM -0400, Jason Merrill wrote:
> On 10/22/24 2:17 PM, Jakub Jelinek wrote:
> > The following testcase shows that the previous get_member_function_from_ptrfunc
> > changes weren't sufficient and we still have cases where
> > -fsanitize=undefined with pointers to member functions can cause wrong code
> > being generated and related false positive warnings.
> > 
> > The problem is that save_expr doesn't always create SAVE_EXPR, it can skip
> > some invariant arithmetics and in the end it could be really large
> > expressions which would be evaluated several times (and what is worse, with
> > -fsanitize=undefined those expressions then can have SAVE_EXPRs added to
> > their subparts for -fsanitize=bounds or -fsanitize=null or
> > -fsanitize=alignment instrumentation).  Tried to just build1 a SAVE_EXPR
> > + add TREE_SIDE_EFFECTS instead of save_expr, but that doesn't work either,
> > because cp_fold happily optimizes those SAVE_EXPRs away when it sees
> > SAVE_EXPR operand is tree_invariant_p.
> 
> Hmm, when is that be a problem?  I wouldn't expect instance_ptr to be
> tree_invariant_p.

E.g. TREE_READONLY !TREE_SIDE_EFFECTS ARRAY_REF (with some const array first
operand and some VAR_DECL etc. second operand).
That is tree_invariant_p, but when -fsanitize=bounds attempts to instrument
that, it sees the index is a VAR_DECL and so creates SAVE_EXPR for it.

	Jakub
  

On 10/23/24 12:33 PM, Jakub Jelinek wrote:
> On Wed, Oct 23, 2024 at 12:27:32PM -0400, Jason Merrill wrote:
>> On 10/22/24 2:17 PM, Jakub Jelinek wrote:
>>> The following testcase shows that the previous get_member_function_from_ptrfunc
>>> changes weren't sufficient and we still have cases where
>>> -fsanitize=undefined with pointers to member functions can cause wrong code
>>> being generated and related false positive warnings.
>>>
>>> The problem is that save_expr doesn't always create SAVE_EXPR, it can skip
>>> some invariant arithmetics and in the end it could be really large
>>> expressions which would be evaluated several times (and what is worse, with
>>> -fsanitize=undefined those expressions then can have SAVE_EXPRs added to
>>> their subparts for -fsanitize=bounds or -fsanitize=null or
>>> -fsanitize=alignment instrumentation).  Tried to just build1 a SAVE_EXPR
>>> + add TREE_SIDE_EFFECTS instead of save_expr, but that doesn't work either,
>>> because cp_fold happily optimizes those SAVE_EXPRs away when it sees
>>> SAVE_EXPR operand is tree_invariant_p.
>>
>> Hmm, when is that be a problem?  I wouldn't expect instance_ptr to be
>> tree_invariant_p.
> 
> E.g. TREE_READONLY !TREE_SIDE_EFFECTS ARRAY_REF (with some const array first
> operand and some VAR_DECL etc. second operand).

That seems like a bug in tree_invariant_p.

> That is tree_invariant_p, but when -fsanitize=bounds attempts to instrument
> that, it sees the index is a VAR_DECL and so creates SAVE_EXPR for it.
> 
> 	Jakub
>
  

On Wed, Oct 23, 2024 at 12:45:53PM -0400, Jason Merrill wrote:
> On 10/23/24 12:33 PM, Jakub Jelinek wrote:
> > On Wed, Oct 23, 2024 at 12:27:32PM -0400, Jason Merrill wrote:
> > > On 10/22/24 2:17 PM, Jakub Jelinek wrote:
> > > > The following testcase shows that the previous get_member_function_from_ptrfunc
> > > > changes weren't sufficient and we still have cases where
> > > > -fsanitize=undefined with pointers to member functions can cause wrong code
> > > > being generated and related false positive warnings.
> > > > 
> > > > The problem is that save_expr doesn't always create SAVE_EXPR, it can skip
> > > > some invariant arithmetics and in the end it could be really large
> > > > expressions which would be evaluated several times (and what is worse, with
> > > > -fsanitize=undefined those expressions then can have SAVE_EXPRs added to
> > > > their subparts for -fsanitize=bounds or -fsanitize=null or
> > > > -fsanitize=alignment instrumentation).  Tried to just build1 a SAVE_EXPR
> > > > + add TREE_SIDE_EFFECTS instead of save_expr, but that doesn't work either,
> > > > because cp_fold happily optimizes those SAVE_EXPRs away when it sees
> > > > SAVE_EXPR operand is tree_invariant_p.
> > > 
> > > Hmm, when is that be a problem?  I wouldn't expect instance_ptr to be
> > > tree_invariant_p.
> > 
> > E.g. TREE_READONLY !TREE_SIDE_EFFECTS ARRAY_REF (with some const array first
> > operand and some VAR_DECL etc. second operand).
> 
> That seems like a bug in tree_invariant_p.

save_expr has been doing that at least since 1992, likely before that.
Though, that
4073	      /* Array ref is const/volatile if the array elements are
4074		 or if the array is..  */
4075	      TREE_READONLY (rval)
4076		|= (CP_TYPE_CONST_P (type) | TREE_READONLY (array));
is done in the C++ FE also since 1994-ish.

I'm afraid what will break especially in Ada if we change it.
Though, unsure even to what.

Are the TREE_READONLY flags needed on ARRAY_REFs/COMPONENT_REFs with
ARRAY_REF bases etc.?
If yes, are ARRAY_REFs/ARRAY_RANGE_REFs with non-invariant index (or
possibly also non-invariant 3rd/4th argument or ARRAY_RANGE_REFs with
non-invariant type size) the only problematic cases?
Say TREE_READONLY COMPONENT_REF with VAR_DECL base should be invariant
I'd hope.
So, should we for the (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))
case walk the tree, looking for the ARRAY_REFs etc. and checking if that
is really invariant?

	Jakub
  

On Wed, Oct 23, 2024 at 08:53:36PM +0200, Jakub Jelinek wrote:
> save_expr has been doing that at least since 1992, likely before that.
> Though, that
> 4073	      /* Array ref is const/volatile if the array elements are
> 4074		 or if the array is..  */
> 4075	      TREE_READONLY (rval)
> 4076		|= (CP_TYPE_CONST_P (type) | TREE_READONLY (array));
> is done in the C++ FE also since 1994-ish.
> 
> I'm afraid what will break especially in Ada if we change it.
> Though, unsure even to what.
> 
> Are the TREE_READONLY flags needed on ARRAY_REFs/COMPONENT_REFs with
> ARRAY_REF bases etc.?
> If yes, are ARRAY_REFs/ARRAY_RANGE_REFs with non-invariant index (or
> possibly also non-invariant 3rd/4th argument or ARRAY_RANGE_REFs with
> non-invariant type size) the only problematic cases?
> Say TREE_READONLY COMPONENT_REF with VAR_DECL base should be invariant
> I'd hope.
> So, should we for the (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))
> case walk the tree, looking for the ARRAY_REFs etc. and checking if that
> is really invariant?

Perhaps INDIRECT_REF/MEM_REF are similarly a problem, one could have
TREE_READONLY INDIRECT_REF or say COMPONENT_REF with INDIRECT_REF first
operand etc. and if the pointer which is dereferenced isn't invariant,
then the INDIRECT_REF/MEM_REF isn't invariant either.

	Jakub
  

On 10/23/24 3:07 PM, Jakub Jelinek wrote:
> On Wed, Oct 23, 2024 at 08:53:36PM +0200, Jakub Jelinek wrote:
>> save_expr has been doing that at least since 1992, likely before that.
>> Though, that
>> 4073	      /* Array ref is const/volatile if the array elements are
>> 4074		 or if the array is..  */
>> 4075	      TREE_READONLY (rval)
>> 4076		|= (CP_TYPE_CONST_P (type) | TREE_READONLY (array));
>> is done in the C++ FE also since 1994-ish.

Setting TREE_READONLY is correct, according to tree.h that just means it 
means it isn't modifiable.

>> I'm afraid what will break especially in Ada if we change it.
>> Though, unsure even to what.
>>
>> Are the TREE_READONLY flags needed on ARRAY_REFs/COMPONENT_REFs with
>> ARRAY_REF bases etc.?
>> If yes, are ARRAY_REFs/ARRAY_RANGE_REFs with non-invariant index (or
>> possibly also non-invariant 3rd/4th argument or ARRAY_RANGE_REFs with
>> non-invariant type size) the only problematic cases?
>> Say TREE_READONLY COMPONENT_REF with VAR_DECL base should be invariant
>> I'd hope.
>> So, should we for the (TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t))
>> case walk the tree, looking for the ARRAY_REFs etc. and checking if that
>> is really invariant?
> 
> Perhaps INDIRECT_REF/MEM_REF are similarly a problem, one could have
> TREE_READONLY INDIRECT_REF or say COMPONENT_REF with INDIRECT_REF first
> operand etc. and if the pointer which is dereferenced isn't invariant,
> then the INDIRECT_REF/MEM_REF isn't invariant either.

Exactly.

Jason