From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62e.google.com (mail-pl1-x62e.google.com [IPv6:2607:f8b0:4864:20::62e]) by sourceware.org (Postfix) with ESMTPS id 0C048385BAEC for ; Sun, 10 Jul 2022 03:58:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0C048385BAEC Received: by mail-pl1-x62e.google.com with SMTP id 5so1886613plk.9 for ; Sat, 09 Jul 2022 20:58:36 -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=fDqEXeOpD5n1JMJL59ERk4svl1Wk7ymlOVTOxuZxFcc=; b=6cvP9V2RsCT6BI1fGAL+wK6y3XsZuCjINCt578UCwJOi//PzQ8cylEepAhC8XzxROq hgmr7h4f4ApmlwxBxAk9MYryk1yMNNLel7Tnx+/qcfhQ/NYp+KPliYRBytKgfJeT6aLf +jMPuOJpsBUgaVRfI3f5BezSRoczPx/cpV30oTuUUWGj1764JBFy0kc2hWvNH4deVh5k Zxkcrg07n/iZTMxyADbYS1XNEPQ1nbNQVjYtV2vHI07HvhoYKc6wKhvDD391EbsOlGlg ns1WQSVE+iXQORJHFwnai4BKWVD8CWo5IlTxUd8VT98cg+woy9VRfADV0RotscCMA8lJ tUiQ== X-Gm-Message-State: AJIora8R4uF4maCcgoPniFVUT8CVZ9yXYN5HzNQV5XlNDRaWcDQffvtC nYLp+vAPuqY23YNA99SgZkE4Y33ClJw= X-Google-Smtp-Source: AGRyM1tvTJEBHtpTT53lmdNq77dd7m/clnVfto7JCBANpxK1RSPMRVB2yoUqRTi+MQ7E5HdNVr3pGA== X-Received: by 2002:a17:902:d509:b0:16c:1664:8201 with SMTP id b9-20020a170902d50900b0016c16648201mr11766781plg.31.1657425513669; Sat, 09 Jul 2022 20:58:33 -0700 (PDT) Received: from [172.31.0.204] (c-73-63-24-84.hsd1.ut.comcast.net. [73.63.24.84]) by smtp.gmail.com with ESMTPSA id g14-20020aa796ae000000b0052536c695c0sm2264606pfk.170.2022.07.09.20.58.33 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 09 Jul 2022 20:58:33 -0700 (PDT) Message-ID: <377c7016-633c-f354-8c21-599ff2b56449@gmail.com> Date: Sat, 9 Jul 2022 21:58:32 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH] c: Fix location for _Pragma tokens [PR97498] Content-Language: en-US To: gcc-patches@gcc.gnu.org References: <20220709205230.GA4991@ldh-imac.local> From: Jeff Law In-Reply-To: <20220709205230.GA4991@ldh-imac.local> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.5 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, 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 03:58:38 -0000 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