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

[Lewis Hyatt <lhyatt@gmail.com>]

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...

-Lewis