[RFC] tree-optimization/104475 - bogus -Wstringop-overflow

  I've previously sent 
https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608077.html
adding ADDR_EXPR_NONZERO and there were comments from Jason
where I just realized I ignored ARRAY_REF for the following.
Anyway, here's a more aggressive variant not going for an extra
flag set by the frontend but instead have the middle-end treat
all &*.component as non-NULL (all handled_component_p).

This passes bootstrap for all languages, testing there isn't
complete but it already shows for example
gcc.c-torture/execute/pr44555.c explicitely testing that
we keep &p->z NULL when p is NULL and z is at offset zero.

There's also execute FAILs for gfortran.dg/class_optional_2.f90
and some optimization dump scan fails I did not yet investigate.

Nevertheless I'd like to hear opinions on whether a middle-end
implementation without frontend help is the way to go and
what the reasonable restrictions should be there?  Is
gcc.c-torture/execute/pr44555.c sanctioned by the C standard?
If so I think we have a lost cause without some help from
the frontend?

Thanks,
Richard.


--

The following avoids a bogus -Wstringop-overflow diagnostic by
properly recognizing that &d->m_mutex cannot be nullptr
even if m_mutex is at offset zero.  The C++ frontend already diagnoses
a &d->m_mutex != nullptr comparison and the following transfers
this knowledge to the middle-end in the most general way.

To avoid the bogus diagnostic this avoids separating the nullptr
path via jump-threading by eliminating the nullptr check.

	PR tree-optimization/104475
	* fold-const.cc (tree_single_nonzero_warnv_p): An ADDR_EXPR
	of a component reference can never be null.

	* g++.dg/opt/pr104475.C: New testcase.
---
 gcc/fold-const.cc                   | 11 ++++++++++-
 gcc/testsuite/g++.dg/opt/pr104475.C | 12 ++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/opt/pr104475.C
  

On 5/4/23 09:59, Richard Biener wrote:
> 
> I've previously sent
> https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608077.html
> adding ADDR_EXPR_NONZERO and there were comments from Jason
> where I just realized I ignored ARRAY_REF for the following.
> Anyway, here's a more aggressive variant not going for an extra
> flag set by the frontend but instead have the middle-end treat
> all &*.component as non-NULL (all handled_component_p).
> 
> This passes bootstrap for all languages, testing there isn't
> complete but it already shows for example
> gcc.c-torture/execute/pr44555.c explicitely testing that
> we keep &p->z NULL when p is NULL and z is at offset zero.
> 
> There's also execute FAILs for gfortran.dg/class_optional_2.f90
> and some optimization dump scan fails I did not yet investigate.
> 
> Nevertheless I'd like to hear opinions on whether a middle-end
> implementation without frontend help is the way to go and
> what the reasonable restrictions should be there?  Is
> gcc.c-torture/execute/pr44555.c sanctioned by the C standard?
> If so I think we have a lost cause without some help from
> the frontend?

The relevant C++ rule is https://eel.is/c++draft/expr.ref#8

The corresponding C clause doesn't have as explicit a rule that I can 
see, I don't know what the sense of the C committee is about this.  The 
special allowance for the common initial sequence suggests such that it 
is an exception to such a rule, but I'm not sure where that rule is, 
exactly.

I imagine that not all languages are as strict about this, so an 
unconditional rule like this may not be what we want.

And as I think I commented before, this kind of assumption based on 
undefined behavior ought to have a -fsanitize=undefined check.

> Thanks,
> Richard.
> 
> 
> --
> 
> The following avoids a bogus -Wstringop-overflow diagnostic by
> properly recognizing that &d->m_mutex cannot be nullptr
> even if m_mutex is at offset zero.  The C++ frontend already diagnoses
> a &d->m_mutex != nullptr comparison and the following transfers
> this knowledge to the middle-end in the most general way.
> 
> To avoid the bogus diagnostic this avoids separating the nullptr
> path via jump-threading by eliminating the nullptr check.
> 
> 	PR tree-optimization/104475
> 	* fold-const.cc (tree_single_nonzero_warnv_p): An ADDR_EXPR
> 	of a component reference can never be null.
> 
> 	* g++.dg/opt/pr104475.C: New testcase.
> ---
>   gcc/fold-const.cc                   | 11 ++++++++++-
>   gcc/testsuite/g++.dg/opt/pr104475.C | 12 ++++++++++++
>   2 files changed, 22 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/opt/pr104475.C
> 
> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> index db54bfc5662..c5c923e059d 100644
> --- a/gcc/fold-const.cc
> +++ b/gcc/fold-const.cc
> @@ -15368,7 +15368,16 @@ tree_single_nonzero_warnv_p (tree t, bool *strict_overflow_p)
>   	tree base = TREE_OPERAND (t, 0);
>   
>   	if (!DECL_P (base))
> -	  base = get_base_address (base);
> +	  {
> +	    gcc_checking_assert (TREE_CODE (base) != WITH_SIZE_EXPR);
> +	    /* Any component reference, even if at offset zero, requires
> +	       a non-null base.  */
> +	    if (handled_component_p (base)
> +		&& !targetm.addr_space.zero_address_valid
> +		      (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (t)))))
> +	      return true;
> +	    base = get_base_address (base);
> +	  }
>   
>   	if (base && TREE_CODE (base) == TARGET_EXPR)
>   	  base = TARGET_EXPR_SLOT (base);
> diff --git a/gcc/testsuite/g++.dg/opt/pr104475.C b/gcc/testsuite/g++.dg/opt/pr104475.C
> new file mode 100644
> index 00000000000..013c70302c6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/opt/pr104475.C
> @@ -0,0 +1,12 @@
> +// { dg-do compile }
> +// { dg-require-effective-target c++11 }
> +// { dg-options "-O -Waddress -fdump-tree-original" }
> +
> +struct X { int i; };
> +
> +bool foo (struct X *p)
> +{
> +  return &p->i != nullptr; /* { dg-warning "never be NULL" } */
> +}
> +
> +/* { dg-final { scan-tree-dump "return <retval> = 1;" "original" } } */
  

On Thu, 4 May 2023, Jason Merrill wrote:

> On 5/4/23 09:59, Richard Biener wrote:
> > 
> > I've previously sent
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608077.html
> > adding ADDR_EXPR_NONZERO and there were comments from Jason
> > where I just realized I ignored ARRAY_REF for the following.
> > Anyway, here's a more aggressive variant not going for an extra
> > flag set by the frontend but instead have the middle-end treat
> > all &*.component as non-NULL (all handled_component_p).
> > 
> > This passes bootstrap for all languages, testing there isn't
> > complete but it already shows for example
> > gcc.c-torture/execute/pr44555.c explicitely testing that
> > we keep &p->z NULL when p is NULL and z is at offset zero.
> > 
> > There's also execute FAILs for gfortran.dg/class_optional_2.f90
> > and some optimization dump scan fails I did not yet investigate.
> > 
> > Nevertheless I'd like to hear opinions on whether a middle-end
> > implementation without frontend help is the way to go and
> > what the reasonable restrictions should be there?  Is
> > gcc.c-torture/execute/pr44555.c sanctioned by the C standard?
> > If so I think we have a lost cause without some help from
> > the frontend?
> 
> The relevant C++ rule is https://eel.is/c++draft/expr.ref#8

OK, I can second-guess that the nullptr object doesn't have the
type specified by the pointer type but is more or less void
which would make subsetting invoke undefined behavior.

> The corresponding C clause doesn't have as explicit a rule that I can see, I
> don't know what the sense of the C committee is about this.  The special
> allowance for the common initial sequence suggests such that it is an
> exception to such a rule, but I'm not sure where that rule is, exactly.
> 
> I imagine that not all languages are as strict about this, so an unconditional
> rule like this may not be what we want.

Looks like that then.  The original proposal would make that
explicit on the ADDR_EXPR though I could imagine we could alternatively
put a flag on the COMPONENT_REF as well.

> And as I think I commented before, this kind of assumption based on undefined
> behavior ought to have a -fsanitize=undefined check.

Agreed.

I don't feel like poking on the C++ too much so I hope that eventually
somebody else will pick this up.

Thanks,
Richard.