From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x130.google.com (mail-lf1-x130.google.com [IPv6:2a00:1450:4864:20::130]) by sourceware.org (Postfix) with ESMTPS id E81153858C53 for ; Sun, 17 Jul 2022 15:24:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E81153858C53 Received: by mail-lf1-x130.google.com with SMTP id e28so15557786lfj.4 for ; Sun, 17 Jul 2022 08:24:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=5yM1FKU3Q2GbMxJqbZdsA5eWsk+gcCKwt6pHqrw53ZA=; b=XuIP2fxYJOAIynawfO0fT+cVKXrhqCzS77PqozPeO1EHjzn4NxHPxl2tXwvwc15wsf ZSz9BRVTcCzwBYRe8MEdnSkVbzW8H2g2Xc6XpcVAIAjiCw/xo1nVyhyknDC1TR+K7c4/ 3qUg4J28b6eyvi3ZS0sAqINZ24XiXYCo+bJPAHr6Ke+m/SrSaP7KXTOy0ejvyCNe2n3r nJhXNNCkJeMkF4fDs2Q7PhxH+LSJ1VKe5KPX088XtmPRodu1l0/n3aLsGp9xbxAWFjok 9tuZlbgg0k7jfHrfz1gGrPS+7/WQW6tcO+AorzcJ0tLlgP/wffmKj++oKWrHz3bvGid6 zVPw== X-Gm-Message-State: AJIora/bLzKI2CjWDVd+mZcN0jr0Z+OnBdCiLhP6nylWaQlyhh26BAUf 3/VHzmbMXIznCqqXIuTWfd3s28ZpRCfXPg8Uc7iu8k8iQJI= X-Google-Smtp-Source: AGRyM1vWTs9a4OiETbraKybPJU821xLKQFezrpFOue7e5Fy4/gO8FejQAe+rjg1PNYx5KZyfEG40Y6x/ozIMFn5t09E= X-Received: by 2002:a05:6512:218c:b0:48a:1e1e:7b59 with SMTP id b12-20020a056512218c00b0048a1e1e7b59mr7731547lft.580.1658071452970; Sun, 17 Jul 2022 08:24:12 -0700 (PDT) MIME-Version: 1.0 References: <20220709205230.GA4991@ldh-imac.local> <377c7016-633c-f354-8c21-599ff2b56449@gmail.com> In-Reply-To: <377c7016-633c-f354-8c21-599ff2b56449@gmail.com> From: Lewis Hyatt Date: Sun, 17 Jul 2022 11:24:01 -0400 Message-ID: Subject: Re: [PATCH] c: Fix location for _Pragma tokens [PR97498] To: gcc-patches List Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3029.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, 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 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: Sun, 17 Jul 2022 15:24:20 -0000 On Sat, Jul 9, 2022 at 11:59 PM Jeff Law via Gcc-patches wrote: > > > > On 7/9/2022 2:52 PM, Lewis Hyatt via Gcc-patches wrote: > > Hello- > > > > PR97498 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97498) is another PR > > related to the fact that imprecise locations for _Pragma result in > > counterintuitive behavior for GCC diagnostic pragmas, which inhibit the > > ability to make convenient wrapper macros for enabling and disabling > > diagnostics in specific scopes. > > > > It looks like David did a lot of work a few years ago improving this > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69543 and > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69558), and in particular > > r233637 added a lot of new test coverage for cases that had regressed in the > > past. > > > > I think the main source of problems for all remaining issues is that we use > > the global input_location for deciding when/if a diagnostic should apply. I > > think it should be eventually doable to eliminate this, and rather properly > > resolve the token locations to the place they need to be > I've long wanted to see our dependency on input_location be diminished > with the goal of making it go away completely. > > so that _Pragma > > type wrapper macros just work the way people expect. > Certainly desirable since many projects have built wrapper macros which > use Pragmas to control warnings. One of the biggest QOI implementation > details we've had with the warnings has been problems with location data > leading to an inability to turn them off in specific locations. > > So I'm all for improvements, in terms of getting our location data more > correct. > > > > > > > That said, PR97498 can be solved easily with a 2-line fix without removing > > input_location, and I think the resulting change to input_location's value > > is an improvement that will benefit other areas, so I thought I'd see what > > you think about this patch please? > > > > Here is a typical testcase. Note the line continuations so it's all one > > logical line. > > > > === > > _Pragma("GCC diagnostic push") \ > > _Pragma("GCC diagnostic ignored \"-Wunused-function\"") \ > > static void f() {} \ > > _Pragma("GCC diagnostic pop") > > === > > > > What happens is that in C++ mode, input_location is always updated to the > > most recently-lexed token, so the above case works fine and does not warn > > when compiled with "g++ -Wunused-functions". However, in C mode, it does > > warn because input_location in C is almost always set to the start of the > > line, and is in this case. So the pop is deemed to take place prior to the > > definition of f(). > > > > Initially, I thought it best to change input_location for C mode to behave > > like C++, and always update to the most recently lexed token. Maybe that's > > still the right way to go, but there was a fair amount of testsuite fallout > > from that. Most of it, was just that we would need to change the tests to look > > for the new locations, and in many cases, the new locations seemed > > preferable to the old ones, but it seemed a bit much for now, so I took a > > more measured approach and just changed input_location in the specific case > > of processing a pragma, to be the location of the CPP_PRAGMA token. > > > > Unfortunately, it turns out that the CPP_PRAGMA token that libcpp provides > > to represent the _Pragma() expression doesn't have a valid location with > > which input_location could be overridden. Looking into that, in r232893 > > David added logic which sets the location of all tokens inside the > > _Pragma(...) to a reasonable place (namely it points to "_Pragma" at the > > expansion point). However, that patch didn't change the location of the > > CPP_PRAGMA token itself to similarly point there, so the 2nd line of this > > patch does that. > > > > The rest of it is just tweaking a couple tests which were sensitive to the > > location being output. In all these cases, the new locations seem more > > informative to me than the old ones. With those tweaks, bootstrap + regtest > > all languages looks good with no regressions. > > > > Please let me know what you think? Thanks! > > gcc/c/ChangeLog: > > > > PR preprocessor/97498 > > * c-parser.cc (c_parser_pragma): Set input_location to the > > location of the pragma, rather than the start of the line. > > > > libcpp/ChangeLog: > > > > PR preprocessor/97498 > > * directives.cc (destringize_and_run): Override the location of > > the CPP_PRAGMA token from a _Pragma directive to the location of > > the expansion point, as is done for the tokens lexed from it. > > > > gcc/testsuite/ChangeLog: > > > > PR preprocessor/97498 > > * c-c++-common/pr97498.c: New test. > > * c-c++-common/gomp/pragma-3.c: Adapt for improved warning locations. > > * c-c++-common/gomp/pragma-5.c: Likewise. > > * gcc.dg/pragma-message.c: Likewise. > > > > libgomp/ChangeLog: > > > > * testsuite/libgomp.oacc-c-c++-common/reduction-5.c: Adapt for > > improved warning locations. > > * testsuite/libgomp.oacc-c-c++-common/vred2d-128.c: Likewise. > OK for the trunk. Thanks for digging into this! > > jeff > There was a request to backport this (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97498#c7) since it is relevant to this one: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106267. Is that OK as well for any of the current release branches please? It will work fine as far back as 10. Thanks... -Lewis