Re: [PATCH] c: Fix location for _Pragma tokens [PR97498]

[Jeff Law <jeffreyalaw@gmail.com>]

On 7/17/2022 9:24 AM, Lewis Hyatt wrote:
> On Sat, Jul 9, 2022 at 11:59 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> 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