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
