From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x635.google.com (mail-pl1-x635.google.com [IPv6:2607:f8b0:4864:20::635]) by sourceware.org (Postfix) with ESMTPS id F2DF0385736D for ; Sun, 31 Jul 2022 02:43:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F2DF0385736D Received: by mail-pl1-x635.google.com with SMTP id w7so7661599ply.12 for ; Sat, 30 Jul 2022 19:43:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=EnRGa9IXJr1htAZg/2y3Hnys3ka/9mspXKTDlbLIdXU=; b=GfUIJIoHsOdIc+jbsavwydPiBdUXVNhy4ZoPb+hG0icdQAnRXH9PtMb7YWrJbCmJrJ 4n0mMAGlz01zOCIxN73GH7VU4N/JtNBJmYLLKQr/OhhhM0b8+YDMzj3dK8E9n2OQRk2H mimS23nDUwhw/cvJzCfBFMgO8Q7UL7E/FOLn6BZmofWic+VyzkVoLbYyEtO23J0IpMSp zbuxDJh5wdPwInWe9sr+xHeLIDIjMlWc6yeMA7THc1f5HDOsi5s6zM1MYIl0SqHI8tJ1 ZlxttTc8V4p+9/+pyxMNNs4QqYjuU235KDgDwqPJChgTvPH5JEFXjcKletM5AvJw5MvY G0Yw== X-Gm-Message-State: ACgBeo2qzMEEIH3L+ztUjU/FkZFvZhiVgBZltTBcfNjI/N8EQxFsBoxT NM1YNSmA+g5druUOzawUaYY= X-Google-Smtp-Source: AA6agR4FgU9/9jHsB68v41iuddU7fn87A3BlQJvTbr8LoVmPIs21w59zuDwqvoEICbMt+9uWqurB7w== X-Received: by 2002:a17:902:ecd0:b0:16d:5001:48f with SMTP id a16-20020a170902ecd000b0016d5001048fmr10753525plh.90.1659235418728; Sat, 30 Jul 2022 19:43:38 -0700 (PDT) Received: from [172.31.0.204] (c-73-98-188-51.hsd1.ut.comcast.net. [73.98.188.51]) by smtp.gmail.com with ESMTPSA id u18-20020a170902e21200b0016d5e4d29f8sm5907869plb.9.2022.07.30.19.43.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 30 Jul 2022 19:43:38 -0700 (PDT) Message-ID: Date: Sat, 30 Jul 2022 20:43:36 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] c: Fix location for _Pragma tokens [PR97498] Content-Language: en-US To: Lewis Hyatt , gcc-patches List References: <20220709205230.GA4991@ldh-imac.local> <377c7016-633c-f354-8c21-599ff2b56449@gmail.com> From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, NICE_REPLY_A, 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, 31 Jul 2022 02:43:42 -0000 On 7/17/2022 9:24 AM, Lewis Hyatt wrote: > 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... Generally we try to focus mostly on codegen issues and regressions on the release branches, but it's not a strict rule.  Given this has been on the trunk for nearly a couple weeks without issues, feel free to go ahead and backport per Martin's request. jeff