From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id ED58D3858C53 for ; Thu, 19 Jan 2023 20:53:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org ED58D3858C53 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1674161625; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ra0CRV3vW/BvB7hzjVE/uTmQav8YmpLTvT8hvjQTU3Q=; b=fxEQGRTLf7Og2BHiluaZT/vboIdCSQeTf6OjHCeOdkRO9B1pig/opJHHQhoOywNTBeJMs1 d7aMkYhmlfo/RobBuQs2pFl0oK6tik61tbLQLgN0WA8kVEAvSuOl0087lwpoiXn5NXeOKd dt/vEzaqaLWbRhi+iS2MR3kXVSARYhs= Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-76-83ntHit1MKCUqzBldztzbQ-1; Thu, 19 Jan 2023 15:53:44 -0500 X-MC-Unique: 83ntHit1MKCUqzBldztzbQ-1 Received: by mail-qt1-f197.google.com with SMTP id br26-20020a05622a1e1a00b003b62dc86831so1491019qtb.8 for ; Thu, 19 Jan 2023 12:53:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Ra0CRV3vW/BvB7hzjVE/uTmQav8YmpLTvT8hvjQTU3Q=; b=FZIhLkfW6ByXvWU4URDWID22MNylf6V2oLiiNwPhub4+H0tYDdZ6hAN2lpDjsICkFi 7xPqb0n4EErv8mJgs/JOc+A/WlsqhuvVGHpq2kyKcj7irv17+b+LTdGlB2JCdKOXf3FY Xl2pPjknpQ8p25oybANFuJ1Gte1ZXqVS2QFMuoDTgpn2eSprYWAXtlFele/V3Ad48W2F QgqJxlQFwf2+wg46B/02cVecMLXmEZNShEb/DvkCqT2b3JaPgYprdSbOuCMo1VULRi5h EmitltDv9sDRvbSDc5u4wqyBXdGr3Ba1s+Fq7xcmBUEKMNBAADnWs8vGvVUFDgKhriPT /Tig== X-Gm-Message-State: AFqh2kpxnBiT8J56AHCLkBOGC2z/r0r2oKFqw4U61dz/GLys1ro1F9cO mnZHBQ8FmL2CgkaPgSkRAD4OD6zb8xZhd5FVsaFvLLOtI/MvttyTb2vePOSaSe3S9iIpXFF/TDf FA6aqqxKR3bOedE4j8A== X-Received: by 2002:ac8:58c3:0:b0:3b6:2e8b:3364 with SMTP id u3-20020ac858c3000000b003b62e8b3364mr21214117qta.38.1674161623517; Thu, 19 Jan 2023 12:53:43 -0800 (PST) X-Google-Smtp-Source: AMrXdXuzitonktZyWM9+lwgMk3seNdw9e4JFBXpbMmm/uAunBpJCgkHse4lFAa6rp8GFaZb8pOb+5w== X-Received: by 2002:ac8:58c3:0:b0:3b6:2e8b:3364 with SMTP id u3-20020ac858c3000000b003b62e8b3364mr21214087qta.38.1674161623106; Thu, 19 Jan 2023 12:53:43 -0800 (PST) Received: from [192.168.1.108] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id c23-20020a05620a269700b006fb11eee465sm25084905qkp.64.2023.01.19.12.53.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 19 Jan 2023 12:53:42 -0800 (PST) Message-ID: <6bd58c29-f344-1950-d483-3da765c6b7a1@redhat.com> Date: Thu, 19 Jan 2023 15:53:41 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH] tree-optimization/104475 - bogus -Wstringop-overflow To: Richard Biener Cc: gcc-patches@gcc.gnu.org, Jakub Jelinek References: <20221207112501.989ED136B4@imap1.suse-dmz.suse.de> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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 1/18/23 03:06, Richard Biener wrote: > 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 (accidentally) 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. Makes sense. > That said, we could re-define GENERIC/GIMPLE here to the extent > that ADDR_EXPR of a COMPONENT_REF (or all handled components?) Not ARRAY_REF, I think; in C++ &p[0] (i.e. p+0) seems well-formed for null p, though any other index is undefined. > 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. Any thoughts about where the -fsanitize=null check goes? > 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 >> >> >> >