From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 275393858D28 for ; Wed, 18 Jan 2023 08:06:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 275393858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id B4C083E7B0; Wed, 18 Jan 2023 08:06:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1674029194; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Gd5NxUljfD7qYn0F8xYWl+0BLkR9V12ZxmfgCVqlvvQ=; b=EBTyFXrEpbJqT9cCnA0ST0QElEBbbarrS75aUmEahSoc5feL9vQyv7HgU7J637jTZJs2QT LbKw4Qmp6c1yGxz/3tglyoP65WhMts0HbZLMwfJqDvwx7NITpY2xj2ACMHnYkTsqw5o+en Lw3DwPUjG+1kSNOm4H/vGQGWxu6geYU= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1674029194; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Gd5NxUljfD7qYn0F8xYWl+0BLkR9V12ZxmfgCVqlvvQ=; b=UPxhIcUnMCZ3Z5Efw2zvAL6cHKQe76i+/qmiT3VLgwbOTurGf2eDaRGIUz9gaz7Nb445lm gbZTSPsJ52LwqwDw== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 97E232C171; Wed, 18 Jan 2023 08:06:34 +0000 (UTC) Date: Wed, 18 Jan 2023 08:06:34 +0000 (UTC) From: Richard Biener To: Jason Merrill cc: gcc-patches@gcc.gnu.org, Jakub Jelinek Subject: Re: [PATCH] tree-optimization/104475 - bogus -Wstringop-overflow In-Reply-To: Message-ID: References: <20221207112501.989ED136B4@imap1.suse-dmz.suse.de> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 = 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 SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)