From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf42.google.com (mail-qv1-xf42.google.com [IPv6:2607:f8b0:4864:20::f42]) by sourceware.org (Postfix) with ESMTPS id 36CA4389244B for ; Thu, 23 Jul 2020 19:08:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 36CA4389244B Received: by mail-qv1-xf42.google.com with SMTP id u8so3048228qvj.12 for ; Thu, 23 Jul 2020 12:08:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=hE1nweBmWnGxhrIGsx8YTXyUd71V7suRJxGZ6PUXbV8=; b=l3y/tGNh6WvcURZ7WmJn4tD8MPFQf+eZ9agxq2c8vJdpxRcDbQO5kGip51HRuqABWL 5pE/Lk3v52rztuKadXXEUwu1ATD8uBg2ae+WDMRiEm99J5lentlxDKqARWeVOsjxJhMp 27r02DMHswmJ9VKJOSZBMFG1KeZ/G/QIJca5R0iY3cn37aCF9HX6BseY6uHAE7RvBD0g ULtwvbXqNN0GMxaP22UAz/uGsWBJM5wDyS092cVzVYoxynYsKYx3sjiFuqp882w4hA3z 9fQbtWy3Pk/kifi11IGIKU3SKH8WjCcgtoAVoMLpgNJeYYYX6nr2v1aXQgkapMl5d2Iq 6cjg== X-Gm-Message-State: AOAM530NlnFmkOeKa8zUpSpvhD97qRbF52G96rySefUSVbTM4zUM6fRB KD1xyxs49VJ6KDugcDp7xaU= X-Google-Smtp-Source: ABdhPJxF6Yd/KCUbfLYevQRFIhhe7Kaboxzk2DA6nz0Cye5d7HQGOnJMudJym3RKjFI+yGczF2z/Gw== X-Received: by 2002:a05:6214:370:: with SMTP id t16mr6262963qvu.206.1595531319745; Thu, 23 Jul 2020 12:08:39 -0700 (PDT) Received: from [192.168.0.41] (174-16-106-56.hlrn.qwest.net. [174.16.106.56]) by smtp.gmail.com with ESMTPSA id w44sm3434099qtj.86.2020.07.23.12.08.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 23 Jul 2020 12:08:38 -0700 (PDT) Subject: Re: [PATCH] avoid -Wnonnull on synthesized condition in static_cast (PR 96003) From: Martin Sebor To: gcc-patches , Jason Merrill References: Message-ID: Date: Thu, 23 Jul 2020 13:08:37 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------3493186E9B0B2226C524704F" Content-Language: en-US X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Jul 2020 19:08:41 -0000 This is a multi-part message in MIME format. --------------3493186E9B0B2226C524704F Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Another test case added to the bug since I posted the patch shows that the no-warning bit set by the C++ front end is easily lost during early folding, triggering the -Wnonull again despite the suppression. The attached revision tweaks the folder in just this one case to let the suppression take effect in this situation. In light of how pervasive this potential for stripping looks (none of the other folders preserves the bit) the tweak feels like a band- aid rather than a general solution. But implementing something better (and mainly developing tests to verify that it works in general) would probably be quite the project. So I leave it for some other time. Martin On 7/17/20 1:00 PM, Martin Sebor wrote: > The recent enhancement to treat the implicit this pointer argument > as nonnull in member functions triggers spurious -Wnonnull for > the synthesized conditional expression the C++ front end replaces > the pointer with in some static_cast expressions.  The front end > already sets the no-warning bit for the test but not for the whole > conditional expression, so the attached fix extends the same solution > to it. > > The consequence of this fix is that user-written code like this: > >   static_cast(p ? p : 0)->f (); > or >   static_cast(p ? p : nullptr)->f (); > > don't trigger the warning because they are both transformed into > the same expression as: > >   static_cast(p)->f (); > > What still does trigger it is this: > >   static_cast(p ? p : (T*)0)->f (); > > because here it's the inner COND_EXPR's no-warning bit that's set > (the outer one is clear), whereas in the former expressions it's > the other way around.  It would be nice if this worked consistently > but I didn't see an easy way to do that and more than a quick fix > seems outside the scope for this bug. > > Another case reported by someone else in the same bug involves > a dynamic_cast.  A simplified test case goes something like this: > >   if (dynamic_cast(p)) >     dynamic_cast(p)->f (); > > The root cause is the same: the front end emitting the COND_EXPR > >   ((p != 0) ? ((T*)__dynamic_cast(p, (& _ZTI1B), (& _ZTI1C), 0)) : 0) > > I decided not to suppress the warning in this case because doing > so would also suppress it in unconditional calls with the result > of the cast: > >   dynamic_cast(p)->f (); > > and that doesn't seem helpful.  Instead, I'd suggest to make > the second cast in the if statement to reference to T&: > >   if (dynamic_cast(p)) >     dynamic_cast(*p).f (); > > Martin --------------3493186E9B0B2226C524704F Content-Type: text/x-patch; name="gcc-96003.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-96003.diff" PR c++/96003 spurious -Wnonnull calling a member on the result of static_cast gcc/c-family/ChangeLog: PR c++/96003 * c-common.c (check_function_arguments_recurse): Return early when no-warning bit is set. gcc/cp/ChangeLog: PR c++/96003 * class.c (build_base_path): Set no-warning bit on the synthesized conditional expression in static_cast. gcc/ChangeLog: fold-const.c (fold_unary_loc): Set no-warning on a rebuilt COND_EXPR if the original had it set. gcc/testsuite/ChangeLog: PR c++/96003 * g++.dg/warn/Wnonnull7.C: New test. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 51ecde69f2d..8a2d641f17c 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5821,6 +5821,9 @@ check_function_arguments_recurse (void (*callback) void *ctx, tree param, unsigned HOST_WIDE_INT param_num) { + if (TREE_NO_WARNING (param)) + return; + if (CONVERT_EXPR_P (param) && (TYPE_PRECISION (TREE_TYPE (param)) == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (param, 0))))) diff --git a/gcc/cp/class.c b/gcc/cp/class.c index a3913f4ce0b..e024e8a4a74 100644 --- a/gcc/cp/class.c +++ b/gcc/cp/class.c @@ -516,8 +516,14 @@ build_base_path (enum tree_code code, out: if (null_test) - expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test, expr, - build_zero_cst (target_type)); + { + expr = fold_build3_loc (input_location, COND_EXPR, target_type, null_test, + expr, build_zero_cst (target_type)); + /* Avoid warning for the whole conditional expression (in addition + to NULL_TEST itself -- see above) in case the result is used in + a nonnull context that the front end -Wnonnull checks. */ + TREE_NO_WARNING (expr) = 1; + } return expr; } diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 300d959278b..67153e3f484 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -8754,6 +8754,7 @@ fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) arg02 = fold_build1_loc (loc, code, type, fold_convert_loc (loc, TREE_TYPE (op0), arg02)); + bool nowarn = TREE_NO_WARNING (op0); tem = fold_build3_loc (loc, COND_EXPR, type, TREE_OPERAND (arg0, 0), arg01, arg02); @@ -8788,6 +8789,8 @@ fold_unary_loc (location_t loc, enum tree_code code, tree type, tree op0) TREE_OPERAND (TREE_OPERAND (tem, 1), 0), TREE_OPERAND (TREE_OPERAND (tem, 2), 0))); + if (nowarn) + TREE_NO_WARNING (tem) = true; return tem; } } diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull7.C b/gcc/testsuite/g++.dg/warn/Wnonnull7.C new file mode 100644 index 00000000000..7611c18d948 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wnonnull7.C @@ -0,0 +1,36 @@ +/* PR c++/96003 - spurious -Wnonnull calling a member on the result + of static_cast + { dg-do compile } + { dg-options "-Wall" } */ + +struct D; +struct B +{ + B* next; + D* Next (); +}; + +struct D: B +{ + virtual ~D (); +}; + +struct Iterator +{ + D* p; + void advance () + { + p = static_cast(p)->Next (); // { dg-bogus "\\\[-Wnonnull" } + } +}; + +// Test case from comment #11. + +struct S1 { virtual ~S1 (); }; +struct S2 { virtual ~S2 (); }; +struct S3: S1, S2 { void f (); }; + +void f (S2 *p) +{ + static_cast(p)->f (); // { dg-bogus "\\\[-Wnonnull" } +} --------------3493186E9B0B2226C524704F--