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

[Richard Biener]

On Tue, 17 Jan 2023, Jason Merrill wrote:

> On 12/7/22 06:25, Richard Biener wrote:
> > The following avoids a bogus -Wstringop-overflow diagnostic by
> > properly recognizing that &d->m_mutex cannot be nullptr in C++
> > even if m_mutex is at offset zero.  The frontend already diagnoses
> > a &d->m_mutex != nullptr comparison and the following transfers
> > this knowledge to the middle-end which sees &d->m_mutex as
> > simple pointer arithmetic.  The new ADDR_NONZERO flag on an
> > ADDR_EXPR is used to carry this information and it's checked in
> > the tree_expr_nonzero_p API which causes this to be folded early.
> > 
> > To avoid the bogus diagnostic this avoids separating the nullptr
> > path via jump-threading by eliminating the nullptr check.
> > 
> > I'd appreciate C++ folks picking this up and put the flag on
> > the appropriate ADDR_EXPRs - I've tried avoiding to put it on
> > all of them and didn't try hard to mimick what -Waddress warns
> > on (the code is big, maybe some refactoring would help but also
> > not sure what exactly the C++ standard constraints are here).
> 
> This is allowed by the standard, at least after CWG2535, but we need to check
> -fsanitize=null before asserting that the address is non-null. With that
> elaboration, a flag on the ADDR_EXPR may not be a convenient way to express
> the property?

Adding a flag on the ADDR_EXPR was mostly out of caution for other
languages that do not have this guarantee (it seems C has a similar
guarantee at least) and for the middle-end (accidentially) producing
such expressions.  That is, I intended to set the flag on ADDR_EXPRs
written by the user as opposed to those created artificially.

I noticed the &* contraction rule and wondered how to conservatively
enforce that - I suppose we'd rely on the frontend to never actually
produce the ADDR_EXPR here.

That said, we could re-define GENERIC/GIMPLE here to the extent
that ADDR_EXPR of a COMPONENT_REF (or all handled components?)
is never nullptr when the target specifies nullptr is not a valid
object address.  We currently already assert there's a valid
object for &p->x if x lives at non-zero offset, so the case we
fail to handle is specifically _only_ the one the component is
at offset zero.  Note &p->x != (void *)4 isn't currently optimized
when x is at offset 4 even though *p would be at address zero
and -Waddress also doesn't diagnose this case - we could
canonicalize this to to p != (void *)0 but then we cannot
treat this as false anymore because of the address-taking of a component.

Richard.

> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > 
> > Thanks,
> > Richard.
> > 
> > 	PR tree-optimization/104475
> > gcc/
> >  * tree-core.h: Document use of nothrow_flag on ADDR_EXPR.
> >  * tree.h (ADDR_NONZERO): New.
> >  * fold-const.cc (tree_single_nonzero_warnv_p): Check
> >  ADDR_NONZERO.
> > 
> > gcc/cp/
> >  * typeck.cc (cp_build_addr_expr_1): Set ADDR_NONZERO
> >  on the built address if it is of a COMPONENT_REF.
> > 
> > 	* g++.dg/opt/pr104475.C: New testcase.
> > ---
> >   gcc/cp/typeck.cc                    |  3 +++
> >   gcc/fold-const.cc                   |  4 +++-
> >   gcc/testsuite/g++.dg/opt/pr104475.C | 12 ++++++++++++
> >   gcc/tree-core.h                     |  3 +++
> >   gcc/tree.h                          |  4 ++++
> >   5 files changed, 25 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/opt/pr104475.C
> > 
> > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> > index 7dfe5acc67e..3563750803e 100644
> > --- a/gcc/cp/typeck.cc
> > +++ b/gcc/cp/typeck.cc
> > @@ -7232,6 +7232,9 @@ cp_build_addr_expr_1 (tree arg, bool strict_lvalue,
> > tsubst_flags_t complain)
> >         gcc_assert (same_type_ignoring_top_level_qualifiers_p
> >         		  (TREE_TYPE (object), decl_type_context (field)));
> >         val = build_address (arg);
> > +      if (TREE_CODE (val) == ADDR_EXPR
> > +	  && TREE_CODE (TREE_OPERAND (val, 0)) == COMPONENT_REF)
> > +	ADDR_NONZERO (val) = 1;
> >       }
> >   
> >     if (TYPE_PTR_P (argtype)
> > diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> > index e80be8049e1..cdfe3f50ae3 100644
> > --- a/gcc/fold-const.cc
> > +++ b/gcc/fold-const.cc
> > @@ -15308,8 +15308,10 @@ tree_single_nonzero_warnv_p (tree t, bool
> > *strict_overflow_p)
> >   
> >       case ADDR_EXPR:
> >         {
> > -	tree base = TREE_OPERAND (t, 0);
> > +	if (ADDR_NONZERO (t))
> > +	  return true;
> >   +	tree base = TREE_OPERAND (t, 0);
> >    if (!DECL_P (base))
> >      base = get_base_address (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" } } */
> > diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> > index e146b133dbd..303e25b5df6 100644
> > --- a/gcc/tree-core.h
> > +++ b/gcc/tree-core.h
> > @@ -1376,6 +1376,9 @@ struct GTY(()) tree_base {
> >          TREE_THIS_NOTRAP in
> >             INDIRECT_REF, MEM_REF, TARGET_MEM_REF, ARRAY_REF,
> >             ARRAY_RANGE_REF
> >   +       ADDR_NONZERO in
> > +	  ADDR_EXPR
> > +
> >          SSA_NAME_IN_FREE_LIST in
> >             SSA_NAME
> >   diff --git a/gcc/tree.h b/gcc/tree.h
> > index 23223ca0c87..1c810c0b21b 100644
> > --- a/gcc/tree.h
> > +++ b/gcc/tree.h
> > @@ -876,6 +876,10 @@ extern void omp_clause_range_check_failed (const_tree,
> > const char *, int,
> >     (TREE_CHECK5 (NODE, INDIRECT_REF, MEM_REF, TARGET_MEM_REF, ARRAY_REF,
> >     \
> >     ARRAY_RANGE_REF)->base.nothrow_flag)
> >   +/* Nozero means this ADDR_EXPR is not equal to NULL.  */
> > +#define ADDR_NONZERO(NODE) \
> > +  (TREE_CHECK (NODE, ADDR_EXPR)->base.nothrow_flag)
> > +
> >   /* In a VAR_DECL, PARM_DECL or FIELD_DECL, or any kind of ..._REF node,
> >      nonzero means it may not be the lhs of an assignment.
> >      Nonzero in a FUNCTION_DECL means this function should be treated
> 
> 
> 

-- 
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)