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

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

[Lewis Hyatt]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]

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 so that _Pragma
type wrapper macros just work the way people expect.

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!

-Lewis

[-- Attachment #2: pr97498.txt --]
[-- Type: text/plain, Size: 11327 bytes --]

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

The handling of #pragma GCC diagnostic uses input_location, which is not always
as precise as needed; in particular the relative location of some tokens and a
_Pragma directive will crucially determine whether a given diagnostic is enabled
or suppressed in the desired way. PR97498 shows how the C frontend ends up with
input_location pointing to the beginning of the line containing a _Pragma()
directive, resulting in the wrong behavior if the diagnostic to be modified
pertains to some tokens found earlier on the same line. This patch fixes that by
addressing two issues:

    a) libcpp was not assigning a valid location to the CPP_PRAGMA token
    generated by the _Pragma directive.
    b) C frontend was not setting input_location to something reasonable.

With this change, the C frontend is able to change input_location to point to
the _Pragma token as needed.

This is just a two-line fix (one for each of a) and b)), the testsuite changes
were needed only because the location on the tested warnings has been somewhat
improved, so the tests need to look for the new locations.

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.

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 9c02141e2c6..92049d1a101 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -12397,6 +12397,7 @@ c_parser_pragma (c_parser *parser, enum pragma_context context, bool *if_p)
   unsigned int id;
   const char *construct = NULL;
 
+  input_location = c_parser_peek_token (parser)->location;
   id = c_parser_peek_token (parser)->pragma_kind;
   gcc_assert (id != PRAGMA_NONE);
 
diff --git a/gcc/testsuite/c-c++-common/gomp/pragma-3.c b/gcc/testsuite/c-c++-common/gomp/pragma-3.c
index c1dee1bcc62..ae18e9b8886 100644
--- a/gcc/testsuite/c-c++-common/gomp/pragma-3.c
+++ b/gcc/testsuite/c-c++-common/gomp/pragma-3.c
@@ -1,13 +1,14 @@
 /* { dg-additional-options "-fdump-tree-original" }  */
 /* PR preprocessor/103165  */
 
-#define inner(...) #__VA_ARGS__ ; _Pragma("omp error severity(warning) message (\"Test\") at(compilation)")
+#define inner(...) #__VA_ARGS__ ; _Pragma("omp error severity(warning) message (\"Test\") at(compilation)") /* { dg-line inner_location } */
 #define outer(...) inner(__VA_ARGS__)
 
 void
 f (void)
 {
-  const char *str = outer(inner(1,2));  /* { dg-warning "'pragma omp error' encountered: Test" } */
+  const char *str = outer(inner(1,2));
+  /* { dg-warning "'pragma omp error' encountered: Test" "inner expansion" { target *-*-* } inner_location } */
 }
 
 #if 0
diff --git a/gcc/testsuite/c-c++-common/gomp/pragma-5.c b/gcc/testsuite/c-c++-common/gomp/pragma-5.c
index af54b682789..8124f701502 100644
--- a/gcc/testsuite/c-c++-common/gomp/pragma-5.c
+++ b/gcc/testsuite/c-c++-common/gomp/pragma-5.c
@@ -1,13 +1,14 @@
 /* { dg-additional-options "-fdump-tree-original" }  */
 /* PR preprocessor/103165  */
 
-#define inner(...) #__VA_ARGS__ ; _Pragma   (	"   omp		error severity   (warning)	message (\"Test\") at(compilation)" )
+#define inner(...) #__VA_ARGS__ ; _Pragma   (	"   omp		error severity   (warning)	message (\"Test\") at(compilation)" ) /* { dg-line inner_location } */
 #define outer(...) inner(__VA_ARGS__)
 
 void
 f (void)
 {
-  const char *str = outer(inner(1,2));  /* { dg-warning "'pragma omp error' encountered: Test" } */
+  const char *str = outer(inner(1,2));
+  /* { dg-warning "'pragma omp error' encountered: Test" "inner expansion" { target *-*-* } inner_location } */
 }
 
 #if 0
diff --git a/gcc/testsuite/c-c++-common/pr97498.c b/gcc/testsuite/c-c++-common/pr97498.c
new file mode 100644
index 00000000000..f5fa420415b
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr97498.c
@@ -0,0 +1,4 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-Wunused-function" } */
+#pragma GCC diagnostic ignored "-Wunused-function"
+static void f() {} _Pragma("GCC diagnostic error \"-Wunused-function\"") /* { dg-bogus "-Wunused-function" } */
diff --git a/gcc/testsuite/gcc.dg/pragma-message.c b/gcc/testsuite/gcc.dg/pragma-message.c
index 2f44b617710..1b7cf09de0a 100644
--- a/gcc/testsuite/gcc.dg/pragma-message.c
+++ b/gcc/testsuite/gcc.dg/pragma-message.c
@@ -42,9 +42,11 @@
 #pragma message ("Okay " THREE)  /* { dg-message "Okay 3" } */
 
 /* Create a TODO() that prints a message on compilation.  */
-#define DO_PRAGMA(x) _Pragma (#x)
-#define TODO(x) DO_PRAGMA(message ("TODO - " #x))
-TODO(Okay 4)                     /* { dg-message "TODO - Okay 4" } */
+#define DO_PRAGMA(x) _Pragma (#x) /* { dg-line pragma_loc1 } */
+#define TODO(x) DO_PRAGMA(message ("TODO - " #x)) /* { dg-line pragma_loc2 } */
+TODO(Okay 4) /* { dg-message "in expansion of macro 'TODO'" } */
+/* { dg-message "TODO - Okay 4" "test4.1" { target *-*-* } pragma_loc1 } */
+/* { dg-message "in expansion of macro 'DO_PRAGMA'" "test4.2" { target *-*-* } pragma_loc2 } */
 
 #if 0
 #pragma message ("Not printed")
diff --git a/libcpp/directives.cc b/libcpp/directives.cc
index f804a441f39..4104d5166e2 100644
--- a/libcpp/directives.cc
+++ b/libcpp/directives.cc
@@ -1930,6 +1930,7 @@ destringize_and_run (cpp_reader *pfile, const cpp_string *in,
       maxcount = 50;
       toks = XNEWVEC (cpp_token, maxcount);
       toks[0] = pfile->directive_result;
+      toks[0].src_loc = expansion_loc;
 
       do
 	{
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
index bae1dee6ad2..16aa0dd4ac1 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
@@ -12,7 +12,7 @@
 
 const int n = 100;
 
-#define DO_PRAGMA(x) _Pragma (#x)
+#define DO_PRAGMA(x) _Pragma (#x) /* { dg-line pragma_loc } */
 
 #define check_reduction(gwv_par, gwv_loop)		\
   {							\
@@ -46,7 +46,7 @@ main (void)
   /* Nvptx targets require a vector_length or 32 in to allow spinlocks with
      gangs.  */
   check_reduction (num_workers (nw) vector_length (vl), worker);
-  /* { dg-warning "region is vector partitioned but does not contain vector partitioned code" "" { target *-*-* } .-1 } */
+  /* { dg-warning "region is vector partitioned but does not contain vector partitioned code" "test1" { target *-*-* } pragma_loc } */
   check_reduction (vector_length (vl), vector);
   check_reduction (num_gangs (ng) num_workers (nw) vector_length (vl), gang
 		   worker vector);
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c
index 9c182d90a0d..84e6d51670b 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/vred2d-128.c
@@ -17,7 +17,7 @@ int a1[n], a2[n];
   void name ()					\
   {						\
   long i, j, t1, t2, t3; /* { dg-line vars } */	\
-  _Pragma(outer)				\
+  _Pragma(outer) /* { dg-line outer } */	\
   for (i = 0; i < n; i++)			\
     {						\
       t1 = 0;					\
@@ -40,44 +40,44 @@ int a1[n], a2[n];
 
 gentest (test1, "acc parallel loop gang vector_length (128) firstprivate (t1, t2)",
 	 "acc loop vector reduction(+:t1) reduction(-:t2)")
-/* { dg-warning {'t1' is used uninitialized} {} { target *-*-* } .-1 }
+/* { dg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
    { dg-note {'t1' was declared here} {} { target *-*-* } vars }
    { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
      TODO See PR101551 for 'offloading_enabled' differences.  */
-/* { dg-warning {'t2' is used uninitialized} {} { target *-*-* } .-5 }
+/* { dg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
    { dg-note {'t2' was declared here} {} { target *-*-* } vars }
    { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
      TODO See PR101551 for 'offloading_enabled' differences.  */
 
 gentest (test2, "acc parallel loop gang vector_length (128) firstprivate (t1, t2)",
 	 "acc loop worker vector reduction(+:t1) reduction(-:t2)")
-/* { dg-warning {'t1' is used uninitialized} {} { target *-*-* } .-1 }
+/* { DUPdg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
    { DUP_dg-note {'t1' was declared here} {} { target *-*-* } vars }
    { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
      TODO See PR101551 for 'offloading_enabled' differences.  */
-/* { dg-warning {'t2' is used uninitialized} {} { target *-*-* } .-5 }
+/* { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
    { DUP_dg-note {'t2' was declared here} {} { target *-*-* } vars }
    { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
      TODO See PR101551 for 'offloading_enabled' differences.  */
 
 gentest (test3, "acc parallel loop gang worker vector_length (128) firstprivate (t1, t2)",
 	 "acc loop vector reduction(+:t1) reduction(-:t2)")
-/* { dg-warning {'t1' is used uninitialized} {} { target *-*-* } .-1 }
+/* { DUPdg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
    { DUP_dg-note {'t1' was declared here} {} { target *-*-* } vars }
    { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
      TODO See PR101551 for 'offloading_enabled' differences.  */
-/* { dg-warning {'t2' is used uninitialized} {} { target *-*-* } .-5 }
+/* { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
    { DUP_dg-note {'t2' was declared here} {} { target *-*-* } vars }
    { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
      TODO See PR101551 for 'offloading_enabled' differences.  */
 
 gentest (test4, "acc parallel loop firstprivate (t1, t2)",
 	 "acc loop reduction(+:t1) reduction(-:t2)")
-/* { dg-warning {'t1' is used uninitialized} {} { target *-*-* } .-1 }
+/* { DUPdg-warning {'t1' is used uninitialized} {} { target *-*-* } outer }
    { DUP_dg-note {'t1' was declared here} {} { target *-*-* } vars }
    { dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-4 }
      TODO See PR101551 for 'offloading_enabled' differences.  */
-/* { dg-warning {'t2' is used uninitialized} {} { target *-*-* } .-5 }
+/* { DUPdg-warning {'t2' is used uninitialized} {} { target *-*-* } outer }
    { DUP_dg-note {'t2' was declared here} {} { target *-*-* } vars }
    { DUP_dg-note {in expansion of macro 'gentest'} {} { target { ! offloading_enabled } } .-8 }
      TODO See PR101551 for 'offloading_enabled' differences.  */

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

[Jeff Law]

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

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

[Lewis Hyatt]

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!


Thanks very much for the feedback, this has been committed. I realized
that I need a similar 1-line patch for gcc -E, to similarly set
input_location to something reasonable (there it's worse actually,
input_location is always the first byte of the file currently). I'll
email that later after testing completes. Then I will plan to work on
eliminating input_location from c-pragma.cc as a longer term goal.

-Lewis

Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases (was: [PATCH] c: Fix location for _Pragma tokens [PR97498])

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

Hi!

On 2022-07-10T16:51:11-0400, Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> 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:
>> > 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

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

> Then I will plan to work on
> eliminating input_location from c-pragma.cc as a longer term goal.

Great; I too am looking forward to that.  There, and then elsewhere,
everywhere.  :-)

>> > The rest of [patch] 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.

ACK, thanks.

On top of that, I've just pushed to master branch
commit 06b2a2abe26554c6f9365676683d67368cbba206
"Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases", see
attached.


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Enhance-_Pragma-diagnostics-verification-in-OMP-C-C-.patch --]
[-- Type: text/x-diff, Size: 4609 bytes --]

From 06b2a2abe26554c6f9365676683d67368cbba206 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Mon, 11 Jul 2022 09:33:19 +0200
Subject: [PATCH] Enhance '_Pragma' diagnostics verification in OMP C/C++ test
 cases

Follow-up to recent commit 0587cef3d7962a8b0f44779589ba2920dd3d71e5
"c: Fix location for _Pragma tokens [PR97498]".

	gcc/testsuite/
	* c-c++-common/gomp/pragma-3.c: Enhance '_Pragma' diagnostics
	verification.
	* c-c++-common/gomp/pragma-5.c: Likewise.
	libgomp/
	* testsuite/libgomp.oacc-c-c++-common/reduction-5.c: Enhance
	'_Pragma' diagnostics verification.
---
 gcc/testsuite/c-c++-common/gomp/pragma-3.c                | 8 +++++---
 gcc/testsuite/c-c++-common/gomp/pragma-5.c                | 8 +++++---
 libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c | 8 +++++---
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/gcc/testsuite/c-c++-common/gomp/pragma-3.c b/gcc/testsuite/c-c++-common/gomp/pragma-3.c
index ae18e9b8886..3e1b2111c3d 100644
--- a/gcc/testsuite/c-c++-common/gomp/pragma-3.c
+++ b/gcc/testsuite/c-c++-common/gomp/pragma-3.c
@@ -2,13 +2,15 @@
 /* PR preprocessor/103165  */
 
 #define inner(...) #__VA_ARGS__ ; _Pragma("omp error severity(warning) message (\"Test\") at(compilation)") /* { dg-line inner_location } */
-#define outer(...) inner(__VA_ARGS__)
+#define outer(...) inner(__VA_ARGS__) /* { dg-line outer_location } */
 
 void
 f (void)
 {
-  const char *str = outer(inner(1,2));
-  /* { dg-warning "'pragma omp error' encountered: Test" "inner expansion" { target *-*-* } inner_location } */
+  const char *str = outer(inner(1,2)); /* { dg-line str_location } */
+  /* { dg-warning "35:'pragma omp error' encountered: Test" "" { target *-*-* } inner_location }
+     { dg-note "20:in expansion of macro 'inner'" "" { target *-*-* } outer_location }
+     { dg-note "21:in expansion of macro 'outer'" "" { target *-*-* } str_location } */
 }
 
 #if 0
diff --git a/gcc/testsuite/c-c++-common/gomp/pragma-5.c b/gcc/testsuite/c-c++-common/gomp/pragma-5.c
index 8124f701502..173c25e803a 100644
--- a/gcc/testsuite/c-c++-common/gomp/pragma-5.c
+++ b/gcc/testsuite/c-c++-common/gomp/pragma-5.c
@@ -2,13 +2,15 @@
 /* PR preprocessor/103165  */
 
 #define inner(...) #__VA_ARGS__ ; _Pragma   (	"   omp		error severity   (warning)	message (\"Test\") at(compilation)" ) /* { dg-line inner_location } */
-#define outer(...) inner(__VA_ARGS__)
+#define outer(...) inner(__VA_ARGS__) /* { dg-line outer_location } */
 
 void
 f (void)
 {
-  const char *str = outer(inner(1,2));
-  /* { dg-warning "'pragma omp error' encountered: Test" "inner expansion" { target *-*-* } inner_location } */
+  const char *str = outer(inner(1,2)); /* { dg-line str_location } */
+  /* { dg-warning "35:'pragma omp error' encountered: Test" "" { target *-*-* } inner_location }
+     { dg-note "20:in expansion of macro 'inner'" "" { target *-*-* } outer_location }
+     { dg-note "21:in expansion of macro 'outer'" "" { target *-*-* } str_location } */
 }
 
 #if 0
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
index 16aa0dd4ac1..72094609f0f 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
@@ -17,7 +17,7 @@ const int n = 100;
 #define check_reduction(gwv_par, gwv_loop)		\
   {							\
   s1 = 2; s2 = 5;					\
-DO_PRAGMA (acc parallel gwv_par copy (s1, s2))		\
+DO_PRAGMA (acc parallel gwv_par copy (s1, s2)) /* { dg-line DO_PRAGMA_loc } */ \
 DO_PRAGMA (acc loop gwv_loop reduction (+:s1, s2))	\
     for (i = 0; i < n; i++)				\
       {							\
@@ -45,8 +45,10 @@ main (void)
 
   /* Nvptx targets require a vector_length or 32 in to allow spinlocks with
      gangs.  */
-  check_reduction (num_workers (nw) vector_length (vl), worker);
-  /* { dg-warning "region is vector partitioned but does not contain vector partitioned code" "test1" { target *-*-* } pragma_loc } */
+  check_reduction (num_workers (nw) vector_length (vl), worker); /* { dg-line check_reduction_loc }
+  /* { dg-warning "22:region is vector partitioned but does not contain vector partitioned code" "" { target *-*-* } pragma_loc }
+     { dg-note "1:in expansion of macro 'DO_PRAGMA'" "" { target *-*-* } DO_PRAGMA_loc }
+     { dg-note "3:in expansion of macro 'check_reduction'" "" { target *-*-* } check_reduction_loc } */
   check_reduction (vector_length (vl), vector);
   check_reduction (num_gangs (ng) num_workers (nw) vector_length (vl), gang
 		   worker vector);
-- 
2.35.1

XFAIL 'offloading_enabled' diagnostics issue in 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551] (was: Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases)

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

Hi!

On 2022-07-11T11:27:12+0200, I wrote:
> [...], I've just pushed to master branch
> commit 06b2a2abe26554c6f9365676683d67368cbba206
> "Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases"

> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
> @@ -17,7 +17,7 @@ const int n = 100;
>  #define check_reduction(gwv_par, gwv_loop)           \
>    {                                                  \
>    s1 = 2; s2 = 5;                                    \
> -DO_PRAGMA (acc parallel gwv_par copy (s1, s2))               \
> +DO_PRAGMA (acc parallel gwv_par copy (s1, s2)) /* { dg-line DO_PRAGMA_loc } */ \
>  DO_PRAGMA (acc loop gwv_loop reduction (+:s1, s2))   \
>      for (i = 0; i < n; i++)                          \
>        {                                                      \
> @@ -45,8 +45,10 @@ main (void)
>
>    /* Nvptx targets require a vector_length or 32 in to allow spinlocks with
>       gangs.  */
> -  check_reduction (num_workers (nw) vector_length (vl), worker);
> -  /* { dg-warning "region is vector partitioned but does not contain vector partitioned code" "test1" { target *-*-* } pragma_loc } */
> +  check_reduction (num_workers (nw) vector_length (vl), worker); /* { dg-line check_reduction_loc }
> +  /* { dg-warning "22:region is vector partitioned but does not contain vector partitioned code" "" { target *-*-* } pragma_loc }
> +     { dg-note "1:in expansion of macro 'DO_PRAGMA'" "" { target *-*-* } DO_PRAGMA_loc }
> +     { dg-note "3:in expansion of macro 'check_reduction'" "" { target *-*-* } check_reduction_loc } */

Oh my, PR101551 "[offloading] Differences in diagnostics etc."
strikes again...  The latter two 'note' diagnostics are currently
only emitted in non-offloading configurations.  I've now pushed to
master branch commit 3723aedaad20a129741c2f6f3c22b3dd1220a3fc
"XFAIL 'offloading_enabled' diagnostics issue in
'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551]", see attached.


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-XFAIL-offloading_enabled-diagnostics-issue-in-libgom.patch --]
[-- Type: text/x-diff, Size: 2368 bytes --]

From 3723aedaad20a129741c2f6f3c22b3dd1220a3fc Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 12 Jul 2022 08:17:37 +0200
Subject: [PATCH] XFAIL 'offloading_enabled' diagnostics issue in
 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551]

Fix-up for recent commit 06b2a2abe26554c6f9365676683d67368cbba206
"Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases".
Supposedly it's the same issue as in
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101551#c2>, where I'd
noted that:

| [...] with an offloading-enabled build of GCC we're losing
| "note: in expansion of macro '[...]'" diagnostics.
| (Effectively '-ftrack-macro-expansion=0'?)

	PR middle-end/101551
	libgomp/
	* testsuite/libgomp.oacc-c-c++-common/reduction-5.c: XFAIL
	'offloading_enabled' diagnostics issue.
---
 libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
index 72094609f0f..ddccfe89e73 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
@@ -45,10 +45,11 @@ main (void)
 
   /* Nvptx targets require a vector_length or 32 in to allow spinlocks with
      gangs.  */
-  check_reduction (num_workers (nw) vector_length (vl), worker); /* { dg-line check_reduction_loc }
+  check_reduction (num_workers (nw) vector_length (vl), worker); /* { dg-line check_reduction_loc } */
   /* { dg-warning "22:region is vector partitioned but does not contain vector partitioned code" "" { target *-*-* } pragma_loc }
-     { dg-note "1:in expansion of macro 'DO_PRAGMA'" "" { target *-*-* } DO_PRAGMA_loc }
-     { dg-note "3:in expansion of macro 'check_reduction'" "" { target *-*-* } check_reduction_loc } */
+     { dg-note "1:in expansion of macro 'DO_PRAGMA'" "" { target *-*-* xfail offloading_enabled } DO_PRAGMA_loc }
+     { dg-note "3:in expansion of macro 'check_reduction'" "" { target *-*-* xfail offloading_enabled } check_reduction_loc }
+     TODO See PR101551 for 'offloading_enabled' XFAILs.  */
   check_reduction (vector_length (vl), vector);
   check_reduction (num_gangs (ng) num_workers (nw) vector_length (vl), gang
 		   worker vector);
-- 
2.35.1

Re: XFAIL 'offloading_enabled' diagnostics issue in 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551] (was: Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases)

[Lewis Hyatt]

On Tue, Jul 12, 2022 at 2:33 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
>
> Hi!
>
> On 2022-07-11T11:27:12+0200, I wrote:
> > [...], I've just pushed to master branch
> > commit 06b2a2abe26554c6f9365676683d67368cbba206
> > "Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases"
>
> > --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
> > +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/reduction-5.c
> > @@ -17,7 +17,7 @@ const int n = 100;
> >  #define check_reduction(gwv_par, gwv_loop)           \
> >    {                                                  \
> >    s1 = 2; s2 = 5;                                    \
> > -DO_PRAGMA (acc parallel gwv_par copy (s1, s2))               \
> > +DO_PRAGMA (acc parallel gwv_par copy (s1, s2)) /* { dg-line DO_PRAGMA_loc } */ \
> >  DO_PRAGMA (acc loop gwv_loop reduction (+:s1, s2))   \
> >      for (i = 0; i < n; i++)                          \
> >        {                                                      \
> > @@ -45,8 +45,10 @@ main (void)
> >
> >    /* Nvptx targets require a vector_length or 32 in to allow spinlocks with
> >       gangs.  */
> > -  check_reduction (num_workers (nw) vector_length (vl), worker);
> > -  /* { dg-warning "region is vector partitioned but does not contain vector partitioned code" "test1" { target *-*-* } pragma_loc } */
> > +  check_reduction (num_workers (nw) vector_length (vl), worker); /* { dg-line check_reduction_loc }
> > +  /* { dg-warning "22:region is vector partitioned but does not contain vector partitioned code" "" { target *-*-* } pragma_loc }
> > +     { dg-note "1:in expansion of macro 'DO_PRAGMA'" "" { target *-*-* } DO_PRAGMA_loc }
> > +     { dg-note "3:in expansion of macro 'check_reduction'" "" { target *-*-* } check_reduction_loc } */
>
> Oh my, PR101551 "[offloading] Differences in diagnostics etc."
> strikes again...  The latter two 'note' diagnostics are currently
> only emitted in non-offloading configurations.  I've now pushed to
> master branch commit 3723aedaad20a129741c2f6f3c22b3dd1220a3fc
> "XFAIL 'offloading_enabled' diagnostics issue in
> 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551]", see attached.
>

Would you mind please confirming how I need to run configure in order
to get this configuration? Then I can look into why the difference in
location information there. Thanks....

-Lewis

Re: XFAIL 'offloading_enabled' diagnostics issue in 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551] (was: Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases)

[Tobias Burnus]

Hi,

On 12.07.22 13:50, Lewis Hyatt via Gcc-patches wrote:
> On Tue, Jul 12, 2022 at 2:33 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
>> On 2022-07-11T11:27:12+0200, I wrote:
>>> Oh my, PR101551 "[offloading] Differences in diagnostics etc."
>>> strikes again...  The latter two 'note' diagnostics are currently
>>> only emitted in non-offloading configurations.  I've now pushed to
>>> master branch commit 3723aedaad20a129741c2f6f3c22b3dd1220a3fc
>>> "XFAIL 'offloading_enabled' diagnostics issue in
>>> 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551]", see attached.
> Would you mind please confirming how I need to run configure in order
> to get this configuration? Then I can look into why the difference in
> location information there. Thanks.

I think the simplest to replicate it without much effort is to run:

cd ${GCC-SRC}/gcc
sed -e 's/ENABLE_OFFLOADING/true/' *.cc */*.cc

I think that covers all cases, which do not need the target lto1.
If they do do - then it becomes more difficult as you need an
offloading compiler. (But that is rather about: diagnostic or
no diagostic and not about having a different diagnostic.)

I think the different diagnostic has the reason stated in
commit r12-135-gbd7ebe9da745a62184052dd1b15f4dd10fbdc9f4

Namely:
----cut---
     It turned out that a compiler built without offloading support
     and one with can produce slightly different diagnostic.

     Offloading support implies ENABLE_OFFLOAD which implies that
     g->have_offload is set when offloading is actually needed.
     In cgraphunit.c, the latter causes flag_generate_offload = 1,
     which in turn affects tree.c's free_lang_data.

     The result is that the front-end specific diagnostic gets reset
     ('tree_diagnostics_defaults (global_dc)'), which affects in this
     case 'Warning' vs. 'warning' via the Fortran frontend.

     Result: 'Warning:' vs. 'warning:'.
     Side note: Other FE also override the diagnostic, leading to
     similar differences, e.g. the C++ FE outputs mangled function
     names differently
----cut------

If the message is from the offload-device's lto1 compiler, it
becomes more difficult to configure+build GCC. See
https://gcc.gnu.org/wiki/Offloading under
"How to build an offloading-enabled GCC"

I hope it helps.

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: XFAIL 'offloading_enabled' diagnostics issue in 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551] (was: Enhance '_Pragma' diagnostics verification in OMP C/C++ test cases)

[Lewis Hyatt]

On Tue, Jul 12, 2022 at 9:10 AM Tobias Burnus <tobias@codesourcery.com> wrote:
> On 12.07.22 13:50, Lewis Hyatt via Gcc-patches wrote:
> > On Tue, Jul 12, 2022 at 2:33 AM Thomas Schwinge <thomas@codesourcery.com> wrote:
> >> On 2022-07-11T11:27:12+0200, I wrote:
> >>> Oh my, PR101551 "[offloading] Differences in diagnostics etc."
> >>> strikes again...  The latter two 'note' diagnostics are currently
> >>> only emitted in non-offloading configurations.  I've now pushed to
> >>> master branch commit 3723aedaad20a129741c2f6f3c22b3dd1220a3fc
> >>> "XFAIL 'offloading_enabled' diagnostics issue in
> >>> 'libgomp.oacc-c-c++-common/reduction-5.c' [PR101551]", see attached.
> > Would you mind please confirming how I need to run configure in order
> > to get this configuration? Then I can look into why the difference in
> > location information there. Thanks.
>
> I think the simplest to replicate it without much effort is to run:
>
> cd ${GCC-SRC}/gcc
> sed -e 's/ENABLE_OFFLOADING/true/' *.cc */*.cc
>
> I think that covers all cases, which do not need the target lto1.
> If they do do - then it becomes more difficult as you need an
> offloading compiler. (But that is rather about: diagnostic or
> no diagostic and not about having a different diagnostic.)
>
> I think the different diagnostic has the reason stated in
> commit r12-135-gbd7ebe9da745a62184052dd1b15f4dd10fbdc9f4
>
> Namely:
> ----cut---
>      It turned out that a compiler built without offloading support
>      and one with can produce slightly different diagnostic.
>
>      Offloading support implies ENABLE_OFFLOAD which implies that
>      g->have_offload is set when offloading is actually needed.
>      In cgraphunit.c, the latter causes flag_generate_offload = 1,
>      which in turn affects tree.c's free_lang_data.
>
>      The result is that the front-end specific diagnostic gets reset
>      ('tree_diagnostics_defaults (global_dc)'), which affects in this
>      case 'Warning' vs. 'warning' via the Fortran frontend.
>
>      Result: 'Warning:' vs. 'warning:'.
>      Side note: Other FE also override the diagnostic, leading to
>      similar differences, e.g. the C++ FE outputs mangled function
>      names differently
> ----cut------
>
> If the message is from the offload-device's lto1 compiler, it
> becomes more difficult to configure+build GCC. See
> https://gcc.gnu.org/wiki/Offloading under
> "How to build an offloading-enabled GCC"
>
> I hope it helps.

Yes, very much, thank you. I am trying something that should improve
it, and also a similar issue that happens with -flto, I made this PR
about the latter: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106274

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

[Lewis Hyatt]

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

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

[Jeff Law]

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

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

[Lewis Hyatt]

On Sat, Jul 30, 2022 at 10:43 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> > 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

Thank you, I'll do that. One question, does a backport need to be an
exact cherry-pick, or is it OK if I need to tweak a few things as
well? I wasn't sure if I need to re-post the patch here in that case.
The patch itself applies to gcc 12 branch fine, however I think I need
a couple small changes to the testsuite parts. Thanks...

-Lewis

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

[Jeff Law]

On 7/31/2022 6:44 AM, Lewis Hyatt wrote:
> On Sat, Jul 30, 2022 at 10:43 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>> 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
> Thank you, I'll do that. One question, does a backport need to be an
> exact cherry-pick, or is it OK if I need to tweak a few things as
> well? I wasn't sure if I need to re-post the patch here in that case.
> The patch itself applies to gcc 12 branch fine, however I think I need
> a couple small changes to the testsuite parts. Thanks...
I personally prefer cherry-pick when we can, but as you note, sometimes 
minor twiddling is necessary, particularly as you go to older and older 
branches.  If you need to make minor changes, go ahead.  Consider the 
patch pre-approved with any minor changes you need to make to work with 
the branch and just post the patch with a note that it was installed as 
pre-approved.

Jeff