From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com [IPv6:2a00:1450:4864:20::22e]) by sourceware.org (Postfix) with ESMTPS id E0C7F385AE56 for ; Sun, 10 Jul 2022 20:51:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E0C7F385AE56 Received: by mail-lj1-x22e.google.com with SMTP id w2so4159780ljj.7 for ; Sun, 10 Jul 2022 13:51:23 -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=yvSXh+/IB6E1Fy2mWIhCNnQ5fonY4DDFEdDCKM9+aB4=; b=WLDeqk77QUMaO89GlJkPLPG5LYrf1olOkM5M/GmI69mPxz62ZY97JEEUOfzJL5GbLs iDxoqotNZesS1c0wqho61Ax6s3lJBWVVqZFJ+eluNOSKau57kc77MaTWAqqmspp/wTDJ S06ShPh/sP+7ZOQk5sSRfFjiWHro+ChQq7GJj0KitqcfLLqZLf++3cYJJo5OgqSnadoK 02VOSoinbkixr+EkKqzQxR8xehQRpuIf9q3yACZPqBB4k2afx4GyO9RkDegpnLY8KH6r JidYr0/2k0e0Fn9QIyXM9xoaB8jJ/vkic4a76C77sC8H9tl4LCzhBy0IcL8v7X6v1+X/ Hs6g== X-Gm-Message-State: AJIora9U4mTx0LnRj7rzvMWOaNs9pkKmqavoLTkNPJuSeeCFARsz4N6v 0DLMu3FIyBnKBvK03kRKpHKHn+mOXQcU27C2V4g= X-Google-Smtp-Source: AGRyM1vQ0LnhA83G4i0JukPCC09B+NgeR8EUfJwWezNg9sEsbsdHdX1wkoTW99O6AMoa/OBLPj9mQTsin85yTX3h7ow= X-Received: by 2002:a2e:9849:0:b0:25d:63f7:ce3b with SMTP id e9-20020a2e9849000000b0025d63f7ce3bmr3831352ljj.427.1657486282141; Sun, 10 Jul 2022 13:51:22 -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, 10 Jul 2022 16:51:11 -0400 Message-ID: Subject: Re: [PATCH] c: Fix location for _Pragma tokens [PR97498] To: Jeff Law Cc: gcc-patches List Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3030.2 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, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Jul 2022 20:51:26 -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! Thanks very much for the feedback, this has been committed. I realized that I need a similar 1-line patch for gcc -E, to similarly set input_location to something reasonable (there it's worse actually, input_location is always the first byte of the file currently). I'll email that later after testing completes. Then I will plan to work on eliminating input_location from c-pragma.cc as a longer term goal. -Lewis