c++, v3: Further address_compare fixes [PR89074]

  On Thu, Feb 03, 2022 at 04:34:17PM -0500, Jason Merrill wrote:
> On 2/3/22 16:18, Jakub Jelinek wrote:
> > On Thu, Feb 03, 2022 at 04:04:57PM -0500, Jason Merrill wrote:
> > > > > I think it would be clearer to leave the !DECL_P case alone and add
> > > > > 
> > > > > /* In C++ it is unspecified, and so non-constant, whether two
> > > > >      equivalent strings have the same address.  */
> > > > > else if (folding_cxx_constexpr
> > > > >            && (TREE_CODE (base0) == STRING_CST
> > > > >                || TREE_CODE (base1) == STRING_CST)
> > > > 
> > > > The point was to let the first if handle for
> > > > !folding_cxx_constexpr the cases with STRING_CST
> > > > as one or both operands and if that falls through, return 2.
> > > 
> > > Ah, I see.  And then for folding_cxx_constexpr you have your new code toward
> > > the bottom of the function that can say they're unequal in some cases.  Can
> > > you combine the STRING_CST handling for both values of folding_cxx_constexpr
> > > instead of having them so far apart?
> > 
> > Not easily, because for the folding_cxx_constexpr case it primarily reuses
> > the code from the last else if - computing sizes of objects and checking
> > if one is at a start of one and another at the end of the other.
> 
> And the !folding_cxx_constexpr case shouldn't also use that code?
> 
> > One further option would be to compute early flags like
> >    enum { OFF_POS_START, OFF_POS_MIDDLE, OFF_POS_END } pos0, pos1;
> > and then just use them or ignore them in the decisions later.
> 
> If that helps to refactor a bit, sure.

Here it is, hopefully it makes the code more readable and understandable.

Bootstrapped/regtested on powerpc64le-linux, ok for trunk?

2022-02-04  Jakub Jelinek  <jakub@redhat.com>

	PR c++/89074
	PR c++/104033
	* fold-const.h (folding_initializer): Adjust comment.
	(folding_cxx_constexpr): Declare.
	* fold-const.cc (folding_initializer): Adjust comment.
	(folding_cxx_constexpr): New variable.
	(address_compare): Restrict the decl vs. STRING_CST
	or vice versa or STRING_CST vs. STRING_CST or
	is_global_var != is_global_var optimizations to !folding_cxx_constexpr.
	Punt for FUNCTION_DECLs with non-zero offsets.  If folding_initializer,
	assume non-aliased functions have non-zero size and have different
	addresses.  For folding_cxx_constexpr, punt on comparisons of start
	of some object and end of another one, regardless whether it is a decl
	or string literal.  Also punt for folding_cxx_constexpr on
	STRING_CST vs. STRING_CST comparisons if the two literals could be
	overlapping.

	* constexpr.cc (cxx_eval_binary_expression): Temporarily set
	folding_cxx_constexpr.

	* g++.dg/cpp1y/constexpr-89074-3.C: New test.



	Jakub
  

On 2/4/22 08:41, Jakub Jelinek wrote:
> On Thu, Feb 03, 2022 at 04:34:17PM -0500, Jason Merrill wrote:
>> On 2/3/22 16:18, Jakub Jelinek wrote:
>>> On Thu, Feb 03, 2022 at 04:04:57PM -0500, Jason Merrill wrote:
>>>>>> I think it would be clearer to leave the !DECL_P case alone and add
>>>>>>
>>>>>> /* In C++ it is unspecified, and so non-constant, whether two
>>>>>>       equivalent strings have the same address.  */
>>>>>> else if (folding_cxx_constexpr
>>>>>>             && (TREE_CODE (base0) == STRING_CST
>>>>>>                 || TREE_CODE (base1) == STRING_CST)
>>>>>
>>>>> The point was to let the first if handle for
>>>>> !folding_cxx_constexpr the cases with STRING_CST
>>>>> as one or both operands and if that falls through, return 2.
>>>>
>>>> Ah, I see.  And then for folding_cxx_constexpr you have your new code toward
>>>> the bottom of the function that can say they're unequal in some cases.  Can
>>>> you combine the STRING_CST handling for both values of folding_cxx_constexpr
>>>> instead of having them so far apart?
>>>
>>> Not easily, because for the folding_cxx_constexpr case it primarily reuses
>>> the code from the last else if - computing sizes of objects and checking
>>> if one is at a start of one and another at the end of the other.
>>
>> And the !folding_cxx_constexpr case shouldn't also use that code?
>>
>>> One further option would be to compute early flags like
>>>     enum { OFF_POS_START, OFF_POS_MIDDLE, OFF_POS_END } pos0, pos1;
>>> and then just use them or ignore them in the decisions later.
>>
>> If that helps to refactor a bit, sure.
> 
> Here it is, hopefully it makes the code more readable and understandable.

Much more readable, thanks!

> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
> 
> 2022-02-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/89074
> 	PR c++/104033
> 	* fold-const.h (folding_initializer): Adjust comment.
> 	(folding_cxx_constexpr): Declare.
> 	* fold-const.cc (folding_initializer): Adjust comment.
> 	(folding_cxx_constexpr): New variable.
> 	(address_compare): Restrict the decl vs. STRING_CST
> 	or vice versa or STRING_CST vs. STRING_CST or
> 	is_global_var != is_global_var optimizations to !folding_cxx_constexpr.
> 	Punt for FUNCTION_DECLs with non-zero offsets.  If folding_initializer,
> 	assume non-aliased functions have non-zero size and have different
> 	addresses.  For folding_cxx_constexpr, punt on comparisons of start
> 	of some object and end of another one, regardless whether it is a decl
> 	or string literal.  Also punt for folding_cxx_constexpr on
> 	STRING_CST vs. STRING_CST comparisons if the two literals could be
> 	overlapping.
> 
> 	* constexpr.cc (cxx_eval_binary_expression): Temporarily set
> 	folding_cxx_constexpr.
> 
> 	* g++.dg/cpp1y/constexpr-89074-3.C: New test.
> 
> --- gcc/fold-const.h.jj	2022-02-01 20:10:51.235856007 +0100
> +++ gcc/fold-const.h	2022-02-03 15:02:02.700228631 +0100
> @@ -20,9 +20,16 @@ along with GCC; see the file COPYING3.
>   #ifndef GCC_FOLD_CONST_H
>   #define GCC_FOLD_CONST_H
>   
> -/* Non-zero if we are folding constants inside an initializer; zero
> -   otherwise.  */
> +/* Nonzero if we are folding constants inside an initializer or a C++
> +   manifestly-constant-evaluated context; zero otherwise.
> +   Should be used when folding in initializer enables additional
> +   optimizations.  */
>   extern int folding_initializer;
> +/* Nonzer of we are folding C++ manifestly-constant-evaluated context; zero

Still need to fix this typo.

> +   otherwise.
> +   Should be used when certain constructs shouldn't be optimized
> +   during folding in that context.  */
> +extern bool folding_cxx_constexpr;
>   
>   /* Convert between trees and native memory representation.  */
>   extern int native_encode_expr (const_tree, unsigned char *, int, int off = -1);
> --- gcc/fold-const.cc.jj	2022-02-03 14:31:32.243129408 +0100
> +++ gcc/fold-const.cc	2022-02-04 10:19:13.812784763 +0100
> @@ -86,9 +86,17 @@ along with GCC; see the file COPYING3.
>   #include "gimple-range.h"
>   
>   /* Nonzero if we are folding constants inside an initializer or a C++
> -   manifestly-constant-evaluated context; zero otherwise.  */
> +   manifestly-constant-evaluated context; zero otherwise.
> +   Should be used when folding in initializer enables additional
> +   optimizations.  */
>   int folding_initializer = 0;
>   
> +/* Nonzer of we are folding C++ manifestly-constant-evaluated context; zero

And here.

> +   otherwise.
> +   Should be used when certain constructs shouldn't be optimized
> +   during folding in that context.  */
> +bool folding_cxx_constexpr = false;
> +
>   /* The following constants represent a bit based encoding of GCC's
>      comparison operators.  This encoding simplifies transformations
>      on relational comparison operators, such as AND and OR.  */
> @@ -16628,41 +16636,55 @@ address_compare (tree_code code, tree ty

Incidentally, the function comment needs to document TYPE.

>     HOST_WIDE_INT ioff0 = -1, ioff1 = -1;
>     off0.is_constant (&ioff0);
>     off1.is_constant (&ioff1);
> -  if ((DECL_P (base0) && TREE_CODE (base1) == STRING_CST)
> -       || (TREE_CODE (base0) == STRING_CST && DECL_P (base1))
> -       || (TREE_CODE (base0) == STRING_CST
> -	   && TREE_CODE (base1) == STRING_CST
> -	   && ioff0 >= 0 && ioff1 >= 0
> -	   && ioff0 < TREE_STRING_LENGTH (base0)
> -	   && ioff1 < TREE_STRING_LENGTH (base1)
> -	  /* This is a too conservative test that the STRING_CSTs
> -	     will not end up being string-merged.  */
> -	   && strncmp (TREE_STRING_POINTER (base0) + ioff0,
> -		       TREE_STRING_POINTER (base1) + ioff1,
> -		       MIN (TREE_STRING_LENGTH (base0) - ioff0,
> -			    TREE_STRING_LENGTH (base1) - ioff1)) != 0))
> -    ;
> -  else if (!DECL_P (base0) || !DECL_P (base1))
> +  /* Punt on non-zero offsets from functions.  */
> +  if ((TREE_CODE (base0) == FUNCTION_DECL && ioff0)
> +      || (TREE_CODE (base1) == FUNCTION_DECL && ioff1))
>       return 2;
> -  /* If this is a pointer comparison, ignore for now even
> -     valid equalities where one pointer is the offset zero
> -     of one object and the other to one past end of another one.  */
> -  else if (!folding_initializer && !INTEGRAL_TYPE_P (type))
> -    ;
> -  /* Assume that automatic variables can't be adjacent to global
> -     variables.  */
> -  else if (is_global_var (base0) != is_global_var (base1))
> -    ;
> +  /* Or if the bases are neither decls nor string literals.  */
> +  if (!DECL_P (base0) && TREE_CODE (base0) != STRING_CST)
> +    return 2;
> +  if (!DECL_P (base1) && TREE_CODE (base1) != STRING_CST)
> +    return 2;
> +
> +  /* Compute whether one address points to the start of one
> +     object and another one to the end of another one.  */
> +  poly_int64 size0 = 0, size1 = 0;
> +  if (TREE_CODE (base0) == STRING_CST)
> +    {
> +      if (ioff0 < 0 || ioff0 > TREE_STRING_LENGTH (base0))
> +	equal = 2;
> +      else
> +	size0 = TREE_STRING_LENGTH (base0);
> +    }
> +  else if (TREE_CODE (base0) == FUNCTION_DECL)
> +    size0 = 1;
>     else
>       {
>         tree sz0 = DECL_SIZE_UNIT (base0);
> +      if (!tree_fits_poly_int64_p (sz0))
> +	equal = 2;
> +      else
> +	size0 = tree_to_poly_int64 (sz0);
> +    }
> +  if (TREE_CODE (base1) == STRING_CST)
> +    {
> +      if (ioff1 < 0 || ioff1 > TREE_STRING_LENGTH (base1))
> +	equal = 2;
> +      else
> +	size1 = TREE_STRING_LENGTH (base1);
> +    }
> +  else if (TREE_CODE (base1) == FUNCTION_DECL)
> +    size1 = 1;
> +  else
> +    {
>         tree sz1 = DECL_SIZE_UNIT (base1);
> -      /* If sizes are unknown, e.g. VLA or not representable, punt.  */
> -      if (!tree_fits_poly_int64_p (sz0) || !tree_fits_poly_int64_p (sz1))
> -	return 2;
> -
> -      poly_int64 size0 = tree_to_poly_int64 (sz0);
> -      poly_int64 size1 = tree_to_poly_int64 (sz1);
> +      if (!tree_fits_poly_int64_p (sz1))
> +	equal = 2;
> +      else
> +	size1 = tree_to_poly_int64 (sz1);
> +    }
> +  if (equal == 0)
> +    {
>         /* If one offset is pointing (or could be) to the beginning of one
>   	 object and the other is pointing to one past the last byte of the
>   	 other object, punt.  */
> @@ -16678,7 +16700,60 @@ address_compare (tree_code code, tree ty
>   	  && (known_ne (off0, 0)
>   	      || (known_ne (size0, 0) && known_ne (size1, 0))))
>   	equal = 0;
> -     }
> +    }
> +

So at this point equal can be 0 or 2, the latter because either the 
offset is out of bounds, or because we're comparing offset 0 to 
one-past-the-end.

In the out-of-bounds case, we're into undefined behavior, so I'd be 
inclined to return 2 immediately rather than continue, so the code below 
only needs to worry about possibly overlapping/contiguous objects.

In the code below, in !constexpr mode we decide to return 0 even though 
equal == 2 in three cases which need more commentary, either together or 
separately:

1) One is a string and the other a decl.  Do we know that we can't 
layout a string and a global variable next to each other?  This overlaps 
a lot with...

2) We're comparing as pointers (rather than integers), so we return 
unequal even if they could be equal in practice if the objects are 
contiguous.  The comment says this but still needs a rationale; it 
doesn't seem useful to me for the limited cases that could reach here 
with equal == 2.

3) We're comparing a local variable and a global, so they really can't 
be equal unless the offset is far out of bounds.  This is currently 
last; we might move it first and treat strings like globals?

I know the above is existing behavior, but I'd like to at least improve 
the comments.

> +  if (TREE_CODE (base0) == STRING_CST || TREE_CODE (base1) == STRING_CST)
> +    {
> +      if (TREE_CODE (base0) != TREE_CODE (base1))
> +	return folding_cxx_constexpr ? equal : 0;
> +
> +      /* STRING_CST vs. STRING_CST.  */
> +      if (folding_cxx_constexpr && equal)
> +	return equal;
> +
> +      if (ioff0 < 0
> +	  || ioff1 < 0
> +	  || ioff0 > TREE_STRING_LENGTH (base0)
> +	  || ioff1 > TREE_STRING_LENGTH (base1))
> +	return 2;
> +
> +      /* If the bytes in the string literals starting at the pointers
> +	 differ, the pointers need to be different.  */
> +      if (memcmp (TREE_STRING_POINTER (base0) + ioff0,
> +		  TREE_STRING_POINTER (base1) + ioff1,
> +		  MIN (TREE_STRING_LENGTH (base0) - ioff0,
> +		       TREE_STRING_LENGTH (base1) - ioff1)) == 0)
> +	{
> +	  HOST_WIDE_INT ioffmin = MIN (ioff0, ioff1);
> +	  if (memcmp (TREE_STRING_POINTER (base0) + ioff0 - ioffmin,
> +		      TREE_STRING_POINTER (base1) + ioff1 - ioffmin,
> +		      ioffmin) == 0)
> +	    /* If even the bytes in the string literal before the
> +	       pointers are the same, the string literals could be
> +	       tail merged.  */
> +	    return 2;
> +	}
> +      return 0;
> +    }
> +
> +  /* If this is a pointer comparison, ignore for now even
> +     valid equalities where one pointer is the offset zero
> +     of one object and the other to one past end of another one.  */
> +  if (!folding_cxx_constexpr && !INTEGRAL_TYPE_P (type))
> +    return 0;
> +
> +  /* Assume that automatic variables can't be adjacent to global
> +     variables.  */
> +  if (!folding_cxx_constexpr && is_global_var (base0) != is_global_var (base1))
> +    return 0;
> +
> +  /* For initializers, assume addresses of different functions are
> +     different.  */
> +  else if (folding_initializer
> +	   && TREE_CODE (base0) == FUNCTION_DECL
> +	   && TREE_CODE (base1) == FUNCTION_DECL)
> +    return 0;
> +
>     return equal;
>   }
>   
> --- gcc/cp/constexpr.cc.jj	2022-01-26 19:40:22.957796808 +0100
> +++ gcc/cp/constexpr.cc	2022-02-03 15:04:00.760558570 +0100
> @@ -3413,7 +3413,10 @@ cxx_eval_binary_expression (const conste
>         if (ctx->manifestly_const_eval
>   	  && (flag_constexpr_fp_except
>   	      || TREE_CODE (type) != REAL_TYPE))
> -	r = fold_binary_initializer_loc (loc, code, type, lhs, rhs);
> +	{
> +	  auto ofcc = make_temp_override (folding_cxx_constexpr, true);
> +	  r = fold_binary_initializer_loc (loc, code, type, lhs, rhs);
> +	}
>         else
>   	r = fold_binary_loc (loc, code, type, lhs, rhs);
>       }
> --- gcc/testsuite/g++.dg/cpp1y/constexpr-89074-3.C.jj	2022-02-03 14:58:53.734901694 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/constexpr-89074-3.C	2022-02-03 14:58:53.734901694 +0100
> @@ -0,0 +1,132 @@
> +// PR c++/89074
> +// { dg-do compile { target c++14 } }
> +
> +int fn1 (void) { return 0; }
> +int fn2 (void) { return 1; }
> +
> +constexpr bool
> +f1 ()
> +{
> +  char a[] = { 1, 2, 3, 4 };
> +
> +  if (&a[1] == "foo")
> +    return false;
> +
> +  if (&a[1] == &"foo"[4])
> +    return false;
> +
> +  if (&"foo"[1] == &a[0])
> +    return false;
> +
> +  if (&"foo"[3] == &a[4])
> +    return false;
> +
> +  if (&a[0] == "foo")
> +    return false;
> +
> +  // Pointer to start of one object (var) and end of another one (literal)
> +  if (&a[0] == &"foo"[4])	// { dg-error "is not a constant expression" }
> +    return false;
> +
> +  return true;
> +}
> +
> +constexpr bool
> +f2 ()
> +{
> +  char a[] = { 1, 2, 3, 4 };
> +
> +  // Pointer to end of one object (var) and start of another one (literal)
> +  if (&a[4] == "foo")		// { dg-error "is not a constant expression" }
> +    return false;
> +
> +  return true;
> +}
> +
> +char v[] = { 1, 2, 3, 4 };
> +
> +constexpr bool
> +f3 ()
> +{
> +  char a[] = { 1, 2, 3, 4 };
> +
> +  if (&a[1] == &v[1])
> +    return false;
> +
> +  if (&a[0] == &v[3])
> +    return false;
> +
> +  if (&a[2] == &v[4])
> +    return false;
> +
> +  // Pointer to start of one object (automatic var) and end of another one (non-automagic var)
> +  if (&a[0] == &v[4])		// { dg-error "is not a constant expression" }
> +    return false;
> +
> +  return true;
> +}
> +
> +constexpr bool
> +f4 ()
> +{
> +  char a[] = { 1, 2, 3, 4, 5 };
> +
> +  // Pointer to end of one object (automatic var) and start of another one (non-automagic var)
> +  if (&a[5] == &v[0])		// { dg-error "is not a constant expression" }
> +    return false;
> +
> +  return true;
> +}
> +
> +constexpr bool
> +f5 ()
> +{
> +  if (fn1 != fn1)
> +    return false;
> +
> +  if (fn1 == fn2)
> +    return false;
> +
> +  if (&"abcde"[0] == &"edcba"[1])
> +    return false;
> +
> +  if (&"abcde"[1] == &"edcba"[6])
> +    return false;
> +
> +  // Pointer to start of one object (literal) and end of another one (literal)
> +  if (&"abcde"[0] == &"edcba"[6])	// { dg-error "is not a constant expression" }
> +    return false;
> +
> +  return true;
> +}
> +
> +constexpr bool
> +f6 ()
> +{
> +  // Pointer to start of one object (literal) and end of another one (literal)
> +  if (&"abcde"[6] == &"edcba"[0])	// { dg-error "is not a constant expression" }
> +    return false;
> +
> +  return true;
> +}
> +
> +constexpr bool
> +f7 ()
> +{
> +  if (&"abcde"[3] == &"fabcde"[3])
> +    return false;
> +
> +  // These could be suffix merged, with &"abcde"[0] == &"fabcde"[1].
> +  if (&"abcde"[3] == &"fabcde"[4])	// { dg-error "is not a constant expression" }
> +    return false;
> +
> +  return true;
> +}
> +
> +constexpr bool a = f1 ();
> +constexpr bool b = f2 ();
> +constexpr bool c = f3 ();
> +constexpr bool d = f4 ();
> +constexpr bool e = f5 ();
> +constexpr bool f = f6 ();
> +constexpr bool g = f7 ();
> 
> 
> 	Jakub
>
  

On Fri, Feb 04, 2022 at 04:42:41PM -0500, Jason Merrill wrote:
> > @@ -20,9 +20,16 @@ along with GCC; see the file COPYING3.
> >   #ifndef GCC_FOLD_CONST_H
> >   #define GCC_FOLD_CONST_H
> > -/* Non-zero if we are folding constants inside an initializer; zero
> > -   otherwise.  */
> > +/* Nonzero if we are folding constants inside an initializer or a C++
> > +   manifestly-constant-evaluated context; zero otherwise.
> > +   Should be used when folding in initializer enables additional
> > +   optimizations.  */
> >   extern int folding_initializer;
> > +/* Nonzer of we are folding C++ manifestly-constant-evaluated context; zero
> 
> Still need to fix this typo.

Sorry, finally now fixed in my copy.

> > +   otherwise.
> > +   Should be used when certain constructs shouldn't be optimized
> > +   during folding in that context.  */
> > +bool folding_cxx_constexpr = false;
> > +
> >   /* The following constants represent a bit based encoding of GCC's
> >      comparison operators.  This encoding simplifies transformations
> >      on relational comparison operators, such as AND and OR.  */
> > @@ -16628,41 +16636,55 @@ address_compare (tree_code code, tree ty
> 
> Incidentally, the function comment needs to document TYPE.

Will do.

> So at this point equal can be 0 or 2, the latter because either the offset
> is out of bounds, or because we're comparing offset 0 to one-past-the-end.

Yes.

> In the out-of-bounds case, we're into undefined behavior, so I'd be inclined
> to return 2 immediately rather than continue, so the code below only needs
> to worry about possibly overlapping/contiguous objects.

You mean for folding_cxx_constexpr ?  The code does that basically, with one
exception, the folding_initializer FUNCTION_DECL cmp FUNCTION_DECL case.
We don't track sizes of functions, so the size of 1 is just a hack to
pretend functions don't have zero size.  Some functions can have zero size
if they contain just __builtin_unreachable, but it is very rare.
But I guess I could move that
  if (folding_initializer
      && TREE_CODE (base0) == FUNCTION_DECL
      && TREE_CODE (base1) == FUNCTION_DECL)
    return 0;
above the size checking block and then indeed right after that do
  if (folding_cxx_constexpr && equal)
    return equal;
with a comment.

> In the code below, in !constexpr mode we decide to return 0 even though
> equal == 2 in three cases which need more commentary, either together or
> separately:
> 
> 1) One is a string and the other a decl.  Do we know that we can't layout a
> string and a global variable next to each other?  This overlaps a lot
> with...
> 
> 2) We're comparing as pointers (rather than integers), so we return unequal
> even if they could be equal in practice if the objects are contiguous.  The
> comment says this but still needs a rationale; it doesn't seem useful to me
> for the limited cases that could reach here with equal == 2.

For the pointer comparisons we just exploit the undefined behavior and
pretend they can't be adjacent even if they actually sometimes can be,
and we've been doing that intentionally for years.
If one does (uintptr_t) &x == (uintptr_t) &y, we try to be more
conservative.

> 3) We're comparing a local variable and a global, so they really can't be
> equal unless the offset is far out of bounds.  This is currently last; we
> might move it first and treat strings like globals?

For the automatic vs. global or strings, it is very unlikely they'd be
adjacent, with typical memory layouts there couldn't be any heap and stack
would need to grow until it reaches end of data or bss section on a page
boundary.  Strings perhaps could be adjacent in .rodata, but again it is
fairly rare.
And sure, I can try to improve comments.

	Jakub