Message ID | 7390933.EvYhyI6sBW@fomalhaut |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 45058383867C for <patchwork@sourceware.org>; Fri, 10 Jun 2022 10:58:18 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 45058383867C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1654858698; bh=M541NP1JNZaDvwQaSJvamOBMGHX9O82DFDaD8SKPLZw=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=phCigavneI9CscAL98BE+LmZjsCph881+9TRBwAqxEKIO/CP7wtpSW1Q2XxNgr3L+ zfqfvS7TiydUky5Pwtm7qbX1jRht2jtb2lZAkHfqQFJ2gBQcwdvLnnIz8btgUIE2hM WN+AxP+Pz/1rumr7Ek5rXVghwHrZFSjFBVyADaxk= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-wm1-x32f.google.com (mail-wm1-x32f.google.com [IPv6:2a00:1450:4864:20::32f]) by sourceware.org (Postfix) with ESMTPS id C946C3838665 for <gcc-patches@gcc.gnu.org>; Fri, 10 Jun 2022 10:57:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C946C3838665 Received: by mail-wm1-x32f.google.com with SMTP id z9so6936201wmf.3 for <gcc-patches@gcc.gnu.org>; Fri, 10 Jun 2022 03:57:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:date:message-id:mime-version :content-transfer-encoding; bh=M541NP1JNZaDvwQaSJvamOBMGHX9O82DFDaD8SKPLZw=; b=zAILM4h2fKr75qiIXCWQW7kzLCHMYoSOsJJfrUA0BsxP3fsnWBjBfyDkU3hz4z/AzX mBk4LahN4g6y8fRcY8m7kuyOCG5HmaskZcBXOennSLrt17Da08wEQw188MPEHVSNY19F hGVVw03c3RoHD+e1+xYOewDYfWly8cXVCYS2pzdLpGlPmFO0skAKpnjssSrib/cXf5U9 BBNrJGqgUHb6EV4ipuNJYs4AVU6+4cbqEgzWVAUX6wE/T19F4OokEFE2bo23bSKMr3z3 IWgThpEDWO6xqfRA5e7I0SfK7QZKSpdxnGC09M1rcI41x4CDeheaRN96Fiegl/zEPB5G EEgA== X-Gm-Message-State: AOAM530VzuDGkoM9NFDvr4QF3wrkvZGKwPn309ueKHAysk+PYlkbm99/ 2i9sjONyNBKWNF1omwYckPqeQSmS+H0zew== X-Google-Smtp-Source: ABdhPJycdfmorebDVPvgiUWHF9UE7aqpHZHOZpYp4vhFX6PQp1pa+F0lbw+ra8VssXR1OHvKyZN/nw== X-Received: by 2002:a7b:c1d1:0:b0:39c:605c:1530 with SMTP id a17-20020a7bc1d1000000b0039c605c1530mr8149579wmj.80.1654858626137; Fri, 10 Jun 2022 03:57:06 -0700 (PDT) Received: from fomalhaut.localnet ([2a01:e0a:8d5:d990:bf38:f508:6f40:de1d]) by smtp.gmail.com with ESMTPSA id l15-20020a05600c47cf00b0039c5642e430sm2461758wmo.20.2022.06.10.03.57.04 for <gcc-patches@gcc.gnu.org> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Jun 2022 03:57:05 -0700 (PDT) X-Google-Original-From: Eric Botcazou <ebotcazou@adacore.com> To: gcc-patches@gcc.gnu.org Subject: [PATCH] Do not erase warning data in gimple_set_location Date: Fri, 10 Jun 2022 12:57:04 +0200 Message-ID: <7390933.EvYhyI6sBW@fomalhaut> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nextPart3672481.kQq0lBPeGt" Content-Transfer-Encoding: 7Bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Eric Botcazou via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Eric Botcazou <botcazou@adacore.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Do not erase warning data in gimple_set_location
|
|
Commit Message
Eric Botcazou
June 10, 2022, 10:57 a.m. UTC
Hi, gimple_set_location is mostly invoked on newly built GIMPLE statements, so their location is UNKNOWN_LOCATION and setting it will clobber the warning data of the passed location, if any. Tested on x86-64/Linux, OK for mainline and 12 branch? 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> * gimple.h (gimple_set_location): Do not copy warning data from the previous location when it is UNKNOWN_LOCATION. 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> testsuite/ * c-c++-common/nonnull-1.c: Remove XFAIL for C++.
Comments
On 6/10/2022 4:57 AM, Eric Botcazou via Gcc-patches wrote: > Hi, > > gimple_set_location is mostly invoked on newly built GIMPLE statements, so > their location is UNKNOWN_LOCATION and setting it will clobber the warning > data of the passed location, if any. > > Tested on x86-64/Linux, OK for mainline and 12 branch? > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > * gimple.h (gimple_set_location): Do not copy warning data from > the previous location when it is UNKNOWN_LOCATION. > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > testsuite/ > * c-c++-common/nonnull-1.c: Remove XFAIL for C++. OK for trunk and gcc-12. jeff
On Fri, Jun 10, 2022 at 12:58 PM Eric Botcazou via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi, > > gimple_set_location is mostly invoked on newly built GIMPLE statements, so > their location is UNKNOWN_LOCATION and setting it will clobber the warning > data of the passed location, if any. Hmm, I think instead of special-casing UNKNOWN_LOCATION what gimple_set_location should probably do is either not copy warnings at all or union them. Btw, gimple_set_location also removes a previously set BLOCK (but gimple_set_block preserves the location locus and diagnostic override). So I'd be tempted to axe the copy_warning () completely here. Martin, there were probably cases that warranted it - do you remember anything specific here? Thanks, Richard. > Tested on x86-64/Linux, OK for mainline and 12 branch? > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > * gimple.h (gimple_set_location): Do not copy warning data from > the previous location when it is UNKNOWN_LOCATION. > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > testsuite/ > * c-c++-common/nonnull-1.c: Remove XFAIL for C++. > > -- > Eric Botcazou
On 6/13/22 05:15, Richard Biener wrote: > On Fri, Jun 10, 2022 at 12:58 PM Eric Botcazou via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> Hi, >> >> gimple_set_location is mostly invoked on newly built GIMPLE statements, so >> their location is UNKNOWN_LOCATION and setting it will clobber the warning >> data of the passed location, if any. > > Hmm, I think instead of special-casing UNKNOWN_LOCATION > what gimple_set_location should probably do is either not copy > warnings at all or union them. Btw, gimple_set_location also > removes a previously set BLOCK (but gimple_set_block preserves > the location locus and diagnostic override). > > So I'd be tempted to axe the copy_warning () completely here. Martin, > there were > probably cases that warranted it - do you remember anything specific here? Nothing specific, just that the assumption behind the warning group design was that a location must exist in order to suppress a warning (a location is one of the first things that's set early on by the FE and it makes little sense to issue a warning without one). There was and in all likelihood still is code sets TREE_NO_WARNING or gimple_no_warning on new trees/statements before setting their location. That interferes with the design when the new tree or statement is meant to be a replacement of another. I fixed a few cases like that to set the location first but didn't have a way of finding all such instances. My expectation was to over time change GCC to make sure a location would always be set before the no-warning bit, and asserting that on every call to these routines. Adding tests like in the patch below goes in the opposite direction and effectively papers over the problem. I can't think of a way to make the suppression work completely reliably without ensuring that a location is always set before suppressing a warning. Martin > > Thanks, > Richard. > >> Tested on x86-64/Linux, OK for mainline and 12 branch? >> >> >> 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> >> >> * gimple.h (gimple_set_location): Do not copy warning data from >> the previous location when it is UNKNOWN_LOCATION. >> >> >> 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> >> >> testsuite/ >> * c-c++-common/nonnull-1.c: Remove XFAIL for C++. >> >> -- >> Eric Botcazou >
> Hmm, I think instead of special-casing UNKNOWN_LOCATION > what gimple_set_location should probably do is either not copy > warnings at all or union them. Btw, gimple_set_location also > removes a previously set BLOCK (but gimple_set_block preserves > the location locus and diagnostic override). > > So I'd be tempted to axe the copy_warning () completely here. The first thing I tried, but it regressed the original testcase IIRC. Even my minimal patch manages to break bootstrap on ARM: buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/libcpp/lex.cc: 1523:9: error: pointer used after ‘void operator delete(void*, std::size_t)’ [-Werror=use-after-free] # 00:31:04 make[3]: *** [Makefile:227: lex.o] Error 1 # 00:31:04 make[2]: *** [Makefile:9527: all-stage3-libcpp] Error 2 # 00:31:35 make[1]: *** [Makefile:25887: stage3-bubble] Error 2 # 00:31:35 make: *** [Makefile:1072: all] Error 2 /* Don't warn for cases like when a cdtor returns 'this' on ARM. */ else if (warning_suppressed_p (var, OPT_Wuse_after_free)) return; because warning-control.cc:copy_warning also clobbers the warning data of the destination. We have in cp/decl.cc:maybe_return_this the lines: /* Return the address of the object. */ tree val = DECL_ARGUMENTS (current_function_decl); suppress_warning (val, OPT_Wuse_after_free); -Wuse-after-free is suppressed for the location of VAL and the TREE_NO_WARNING bit set on it. But other expressions may have the same location as VAL and the TREE_NO_WARNING bit _not_ set, so when you call copy_warning (expr, expr) (we do that a lot after failed folding) for them, copy_warning erases the warning data of the location. I have installed the obvious fixlet after testing on x86-64/Linux, but the decoupling between TREE_NO_WARNING bit and location looks a bit problematic. * warning-control.cc (copy_warning) [generic version]: Do not erase the warning data of the destination location when the no-warning bit is not set on the source. (copy_warning) [tree version]: Return early if TO is equal to FROM. (copy_warning) [gimple version]: Likewise. testsuite/ * g++.dg/warn/Wuse-after-free5.C: New test.
On Tue, Jun 14, 2022 at 12:49 PM Eric Botcazou <botcazou@adacore.com> wrote: > > > Hmm, I think instead of special-casing UNKNOWN_LOCATION > > what gimple_set_location should probably do is either not copy > > warnings at all or union them. Btw, gimple_set_location also > > removes a previously set BLOCK (but gimple_set_block preserves > > the location locus and diagnostic override). > > > > So I'd be tempted to axe the copy_warning () completely here. > > The first thing I tried, but it regressed the original testcase IIRC. > > Even my minimal patch manages to break bootstrap on ARM: > > buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/libcpp/lex.cc: > 1523:9: error: pointer used after ‘void operator delete(void*, std::size_t)’ > [-Werror=use-after-free] > # 00:31:04 make[3]: *** [Makefile:227: lex.o] Error 1 > # 00:31:04 make[2]: *** [Makefile:9527: all-stage3-libcpp] Error 2 > # 00:31:35 make[1]: *** [Makefile:25887: stage3-bubble] Error 2 > # 00:31:35 make: *** [Makefile:1072: all] Error 2 > > /* Don't warn for cases like when a cdtor returns 'this' on ARM. */ > else if (warning_suppressed_p (var, OPT_Wuse_after_free)) > return; > > because warning-control.cc:copy_warning also clobbers the warning data of the > destination. We have in cp/decl.cc:maybe_return_this the lines: > > /* Return the address of the object. */ > tree val = DECL_ARGUMENTS (current_function_decl); > suppress_warning (val, OPT_Wuse_after_free); > > -Wuse-after-free is suppressed for the location of VAL and the TREE_NO_WARNING > bit set on it. But other expressions may have the same location as VAL and > the TREE_NO_WARNING bit _not_ set, so when you call copy_warning (expr, expr) > (we do that a lot after failed folding) for them, copy_warning erases the > warning data of the location. > > I have installed the obvious fixlet after testing on x86-64/Linux, but the > decoupling between TREE_NO_WARNING bit and location looks a bit problematic. Thanks - that makes more sense. > > * warning-control.cc (copy_warning) [generic version]: Do not erase > the warning data of the destination location when the no-warning > bit is not set on the source. > (copy_warning) [tree version]: Return early if TO is equal to FROM. > (copy_warning) [gimple version]: Likewise. > testsuite/ > * g++.dg/warn/Wuse-after-free5.C: New test. > > -- > Eric Botcazou
diff --git a/gcc/gimple.h b/gcc/gimple.h index 6b1e89ad74e..870629cd562 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -1913,7 +1913,8 @@ static inline void gimple_set_location (gimple *g, location_t location) { /* Copy the no-warning data to the statement location. */ - copy_warning (location, g->location); + if (g->location != UNKNOWN_LOCATION) + copy_warning (location, g->location); g->location = location; } diff --git a/gcc/testsuite/c-c++-common/nonnull-1.c b/gcc/testsuite/c-c++-common/nonnull-1.c index ea987365302..7be4e3479dd 100644 --- a/gcc/testsuite/c-c++-common/nonnull-1.c +++ b/gcc/testsuite/c-c++-common/nonnull-1.c @@ -30,5 +30,5 @@ func (char *cp1, char *cp2, char *cp3, char *cp4) __attribute__((nonnull (1))) int func2 (char *cp) { - return (cp != NULL) ? 1 : 0; /* { dg-warning "'nonnull' argument" "cp compared to NULL" { xfail c++ } } */ + return (cp != NULL) ? 1 : 0; /* { dg-warning "'nonnull' argument" "cp compared to NULL" } */ }