[Patch] libcpp: Fix _Pragma expansion [PR102409]

[Patch] libcpp: Fix _Pragma expansion [PR102409]

[Tobias Burnus]

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

Before this patch, running

#define TEST(T) T
#define PARALLEL(X) TEST(X)
PARALLEL(
     for (int i = 0; i < N; i++) { \
       _Pragma("omp ordered") \
       S[0] += C[i] + D[i]; \
     })

through 'gcc -E' yielded

#pragma omp ordered
  for (int i = 0; i < N; i++) { S[0] += C[i] + D[i]; }

Note that the '#pragma omp ordered' is now above the loop, i.e. before
all macro arguments (and macro expansions).

With the patch, the result is the following, which matches Clang and I
assume GCC before 4.2 or 4.3, but I have no GCC 4.x available:

for (int i = 0; i < N; i++) {
#pragma omp ordered
  S[0] += C[i] + D[i]; }


The reason seems to be the addition done for PR34692 in r131819, which
added code to avoid an ICE with
FOO(
#pragma GCC diagnostic
)

There is a length description in macro.c about what it does and the
pragma_buff which is passed around, including to the now modified
collect_args. Namely, the comment above enter_macro_context states:

    If there were additionally any unexpanded deferred #pragma
    directives among macro arguments, push another context containing
    the pragma tokens before the yet-to-be-rescanned replacement list
    and return two.

While that seems to work fine with #pragma, it obviously does not do
what it should for _Pragma. The solution in the patch was to add a
flag to distinguish the CPP_PRAGMA coming from the _Pragma operator
(alias BT_PRAGMA) from the CPP_PRAGMA coming from a user's #pragma.

OK for mainline? – It is a long-standing regression, but it hasn't
been reported for a while. Thus: how do you feel about backporting?

I did test it with a full bootstrap + regtesting. I also tested
omptests (cf. PR).

Tobias

PS: I had the hope that it would fix some of the other _Pragma related
PRs (see e.g. refs in this PR102409 or search Bugzilla), but it does
not seem to help for those. I do note that most of them are related to
diagnostic. In particular, for PR91669, the output of gcc -E is the
same for GCC 10, for a patched GCC and for clang-11, which makes the
result (issue unaffected by this patch) not that surprising.
-----------------
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

[-- Attachment #2: fix-pragma.diff --]
[-- Type: text/x-patch, Size: 2540 bytes --]

libcpp: Fix _Pragma expansion [PR102409]

Both #pragma and _Pragma ended up as CPP_PRAGMA. Presumably since
r131819 (2008, GCC 4.3) for PR34692, pragmas are not expanded in
macro arguments but are output as is before. From the old bug report,
that was to fix usage like
  FOO (
    #pragma GCC diagnostic
  )
However, that change also affected _Pragma such that
  BAR (
    "1";
    _Pragma("omp ..."); )
yielded
  #pragma omp ...
followed by what BAR expanded too, possibly including '"1";'.

This commit adds a flag, PRAGMA_OP, to tokens to make the two
distinguishable - and include again _Pragma in the expanded arguments.

libcpp/ChangeLog:

	PR c++/102409
	* directives.c (destringize_and_run): Add PRAGMA_OP to the
	CPP_PRAGMA token's flags to mark is as coming from _Pragma.
	* include/cpplib.h (PRAGMA_OP): #define, to be used with token flags.
	* macro.c (collect_args): Only handle CPP_PRAGMA special if PRAGMA_OP
	is set.

 libcpp/directives.c     | 2 ++
 libcpp/include/cpplib.h | 1 +
 libcpp/macro.c          | 2 +-
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/libcpp/directives.c b/libcpp/directives.c
index b4bc8b4df30..34f7677f718 100644
--- a/libcpp/directives.c
+++ b/libcpp/directives.c
@@ -1907,6 +1907,8 @@ destringize_and_run (cpp_reader *pfile, const cpp_string *in,
   save_directive = pfile->directive;
   pfile->directive = &dtable[T_PRAGMA];
   do_pragma (pfile);
+  if (pfile->directive_result.type == CPP_PRAGMA)
+    pfile->directive_result.flags |= PRAGMA_OP;
   end_directive (pfile, 1);
   pfile->directive = save_directive;
 
diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
index 6e2fcb6b1f2..56b07acc1d7 100644
--- a/libcpp/include/cpplib.h
+++ b/libcpp/include/cpplib.h
@@ -198,6 +198,7 @@ struct GTY(()) cpp_string {
 				    operator, or before this token
 				    after a # operator.  */
 #define NO_EXPAND	(1 << 10) /* Do not macro-expand this token.  */
+#define PRAGMA_OP	(1 << 11) /* _Pragma token.  */
 
 /* Specify which field, if any, of the cpp_token union is used.  */
 
diff --git a/libcpp/macro.c b/libcpp/macro.c
index f214548de1e..b2f797cae35 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -1259,7 +1259,7 @@ collect_args (cpp_reader *pfile, const cpp_hashnode *node,
 	  else if (token->type == CPP_EOF
 		   || (token->type == CPP_HASH && token->flags & BOL))
 	    break;
-	  else if (token->type == CPP_PRAGMA)
+	  else if (token->type == CPP_PRAGMA && !(token->flags & PRAGMA_OP))
 	    {
 	      cpp_token *newtok = _cpp_temp_token (pfile);
 

Re: [Patch] libcpp: Fix _Pragma expansion [PR102409]

[Martin Sebor]

On 10/28/21 9:51 AM, Tobias Burnus wrote:
> Before this patch, running
> 
> #define TEST(T) T
> #define PARALLEL(X) TEST(X)
> PARALLEL(
>      for (int i = 0; i < N; i++) { \
>        _Pragma("omp ordered") \
>        S[0] += C[i] + D[i]; \
>      })
> 
> through 'gcc -E' yielded
> 
> #pragma omp ordered
>   for (int i = 0; i < N; i++) { S[0] += C[i] + D[i]; }
> 
> Note that the '#pragma omp ordered' is now above the loop, i.e. before
> all macro arguments (and macro expansions).
> 
> With the patch, the result is the following, which matches Clang and I
> assume GCC before 4.2 or 4.3, but I have no GCC 4.x available:
> 
> for (int i = 0; i < N; i++) {
> #pragma omp ordered
>   S[0] += C[i] + D[i]; }

There are a number of bug reports of _Pragma not working right
in macros, including (and especially) to control diagnostics:
https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=_Pragma%20macro&list_id=328003

Just by the description this change seems like it could also
fix some of them.  It would be helpful to check to see if it
does and if so, add tests and resolve the bugs it fixes (I'm
willing to help with that in stage 3).

Martin

> 
> 
> The reason seems to be the addition done for PR34692 in r131819, which
> added code to avoid an ICE with
> FOO(
> #pragma GCC diagnostic
> )
> 
> There is a length description in macro.c about what it does and the
> pragma_buff which is passed around, including to the now modified
> collect_args. Namely, the comment above enter_macro_context states:
> 
>     If there were additionally any unexpanded deferred #pragma
>     directives among macro arguments, push another context containing
>     the pragma tokens before the yet-to-be-rescanned replacement list
>     and return two.
> 
> While that seems to work fine with #pragma, it obviously does not do
> what it should for _Pragma. The solution in the patch was to add a
> flag to distinguish the CPP_PRAGMA coming from the _Pragma operator
> (alias BT_PRAGMA) from the CPP_PRAGMA coming from a user's #pragma.
> 
> OK for mainline? – It is a long-standing regression, but it hasn't
> been reported for a while. Thus: how do you feel about backporting?
> 
> I did test it with a full bootstrap + regtesting. I also tested
> omptests (cf. PR).
> 
> Tobias
> 
> PS: I had the hope that it would fix some of the other _Pragma related
> PRs (see e.g. refs in this PR102409 or search Bugzilla), but it does
> not seem to help for those. I do note that most of them are related to
> diagnostic. In particular, for PR91669, the output of gcc -E is the
> same for GCC 10, for a patched GCC and for clang-11, which makes the
> result (issue unaffected by this patch) not that surprising.
> -----------------
> 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: [Patch] libcpp: Fix _Pragma expansion [PR102409]

[Jakub Jelinek]

On Thu, Oct 28, 2021 at 05:51:59PM +0200, Tobias Burnus wrote:
> libcpp/ChangeLog:
> 
> 	PR c++/102409
> 	* directives.c (destringize_and_run): Add PRAGMA_OP to the
> 	CPP_PRAGMA token's flags to mark is as coming from _Pragma.
> 	* include/cpplib.h (PRAGMA_OP): #define, to be used with token flags.
> 	* macro.c (collect_args): Only handle CPP_PRAGMA special if PRAGMA_OP
> 	is set.
> 

The patch itself looks reasonable to me, but it should come up with
testsuite coverage.  And the testsuite coverage should include both normal
testcases that do use integrated preprocessor, and the same with
-save-temps to make sure that even when preprocessing separately it works
too.

	Jakub

Re: [Patch] libcpp: Fix _Pragma expansion [PR102409]

[Tobias Burnus]

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

On 29.10.21 13:06, Jakub Jelinek wrote:
> On Thu, Oct 28, 2021 at 05:51:59PM +0200, Tobias Burnus wrote:
>> libcpp/ChangeLog:
>>
>>      PR c++/102409
>>      * directives.c (destringize_and_run): Add PRAGMA_OP to the
>>      CPP_PRAGMA token's flags to mark is as coming from _Pragma.
>>      * include/cpplib.h (PRAGMA_OP): #define, to be used with token flags.
>>      * macro.c (collect_args): Only handle CPP_PRAGMA special if PRAGMA_OP
>>      is set.
>>
> The patch itself looks reasonable to me, but it should come up with
> testsuite coverage.  And the testsuite coverage should include both normal
> testcases that do use integrated preprocessor, and the same with
> -save-temps to make sure that even when preprocessing separately it works
> too.

Yes, I realized myself that I missed to include a testcase – thanks for
the -save-temps suggestion!

Updated patch enclosed.

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

[-- Attachment #2: fix-pragma-v2.diff --]
[-- Type: text/x-patch, Size: 6089 bytes --]

libcpp: Fix _Pragma expansion [PR102409]

Both #pragma and _Pragma ended up as CPP_PRAGMA. Presumably since
r131819 (2008, GCC 4.3) for PR34692, pragmas are not expanded in
macro arguments but are output as is before. From the old bug report,
that was to fix usage like
  FOO (
    #pragma GCC diagnostic
  )
However, that change also affected _Pragma such that
  BAR (
    "1";
    _Pragma("omp ..."); )
yielded
  #pragma omp ...
followed by what BAR expanded too, possibly including '"1";'.

This commit adds a flag, PRAGMA_OP, to tokens to make the two
distinguishable - and include again _Pragma in the expanded arguments.

libcpp/ChangeLog:

	PR c++/102409
	* directives.c (destringize_and_run): Add PRAGMA_OP to the
	CPP_PRAGMA token's flags to mark is as coming from _Pragma.
	* include/cpplib.h (PRAGMA_OP): #define, to be used with token flags.
	* macro.c (collect_args): Only handle CPP_PRAGMA special if PRAGMA_OP
	is set.

gcc/testsuite/ChangeLog:

	* c-c++-common/gomp/pragma-1.c: New test.
	* c-c++-common/gomp/pragma-2.c: New test.

 gcc/testsuite/c-c++-common/gomp/pragma-1.c | 50 ++++++++++++++++++++++++++++++
 gcc/testsuite/c-c++-common/gomp/pragma-2.c | 50 ++++++++++++++++++++++++++++++
 libcpp/directives.c                        |  2 ++
 libcpp/include/cpplib.h                    |  1 +
 libcpp/macro.c                             |  2 +-
 5 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/c-c++-common/gomp/pragma-1.c b/gcc/testsuite/c-c++-common/gomp/pragma-1.c
new file mode 100644
index 00000000000..e330f17204a
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/pragma-1.c
@@ -0,0 +1,50 @@
+/* { dg-additional-options "-fdump-tree-original" }  */
+/* PR c++/51484  */
+
+#define TEST(T) { \
+  int fail = 0, trial; \
+  for (int trial = 0; trial < TRIALS && fail == 0; trial++) { \
+    _Pragma("omp target teams num_teams(1) thread_limit(1024)") \
+     {T} \
+  } \
+}
+
+#define TRIALS (1)
+#define N (1024*3)
+
+int main(void) {
+
+  double C[N], D[N];
+  double S[N];
+  double p[2];
+  int i; 
+  for (i = 0; i < N; i++)
+  {C[i] = 1; D[i] = i;}
+
+  int max_threads = 224;
+
+#define PARALLEL(X) TEST({ \
+_Pragma("omp parallel if(threads[0] > 1) num_threads(threads[0])") \
+{ \
+_Pragma("omp for ordered") \
+  X  \
+_Pragma("omp for schedule(auto) ordered") \
+  X  \
+} \
+})
+
+  for (int t = 0; t <= max_threads; t += max_threads) {
+    int threads[1]; threads[0] = t;
+    S[0] = 0;
+    PARALLEL(
+    for (int i = 0; i < N; i++) { \
+      _Pragma("omp ordered") \
+      S[0] += C[i] + D[i]; \
+    })
+  }
+  return 0;
+}
+
+/* On expansion, the _Pragma were wrongly placed, ensure the order is now correct: */
+/* { dg-final { scan-tree-dump "#pragma omp target.*#pragma omp teams num_teams\\(1\\) thread_limit\\(1024\\).*#pragma omp parallel num_threads\\(threads\\\[0\\\]\\) if\\(threads\\\[0\\\] > 1\\).*#pragma omp for ordered.*#pragma omp ordered.*#pragma omp for ordered schedule\\(auto\\).*#pragma omp ordered" "original" } } */
+
diff --git a/gcc/testsuite/c-c++-common/gomp/pragma-2.c b/gcc/testsuite/c-c++-common/gomp/pragma-2.c
new file mode 100644
index 00000000000..5358f875959
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/pragma-2.c
@@ -0,0 +1,50 @@
+/* { dg-additional-options "-fdump-tree-original -save-temps" }  */
+/* PR c++/51484  */
+
+#define TEST(T) { \
+  int fail = 0, trial; \
+  for (int trial = 0; trial < TRIALS && fail == 0; trial++) { \
+    _Pragma("omp target teams num_teams(1) thread_limit(1024)") \
+     {T} \
+  } \
+}
+
+#define TRIALS (1)
+#define N (1024*3)
+
+int main(void) {
+
+  double C[N], D[N];
+  double S[N];
+  double p[2];
+  int i; 
+  for (i = 0; i < N; i++)
+  {C[i] = 1; D[i] = i;}
+
+  int max_threads = 224;
+
+#define PARALLEL(X) TEST({ \
+_Pragma("omp parallel if(threads[0] > 1) num_threads(threads[0])") \
+{ \
+_Pragma("omp for ordered") \
+  X  \
+_Pragma("omp for schedule(auto) ordered") \
+  X  \
+} \
+})
+
+  for (int t = 0; t <= max_threads; t += max_threads) {
+    int threads[1]; threads[0] = t;
+    S[0] = 0;
+    PARALLEL(
+    for (int i = 0; i < N; i++) { \
+      _Pragma("omp ordered") \
+      S[0] += C[i] + D[i]; \
+    })
+  }
+  return 0;
+}
+
+/* On expansion, the _Pragma were wrongly placed, ensure the order is now correct: */
+/* { dg-final { scan-tree-dump "#pragma omp target.*#pragma omp teams num_teams\\(1\\) thread_limit\\(1024\\).*#pragma omp parallel num_threads\\(threads\\\[0\\\]\\) if\\(threads\\\[0\\\] > 1\\).*#pragma omp for ordered.*#pragma omp ordered.*#pragma omp for ordered schedule\\(auto\\).*#pragma omp ordered" "original" } } */
+
diff --git a/libcpp/directives.c b/libcpp/directives.c
index b4bc8b4df30..34f7677f718 100644
--- a/libcpp/directives.c
+++ b/libcpp/directives.c
@@ -1907,6 +1907,8 @@ destringize_and_run (cpp_reader *pfile, const cpp_string *in,
   save_directive = pfile->directive;
   pfile->directive = &dtable[T_PRAGMA];
   do_pragma (pfile);
+  if (pfile->directive_result.type == CPP_PRAGMA)
+    pfile->directive_result.flags |= PRAGMA_OP;
   end_directive (pfile, 1);
   pfile->directive = save_directive;
 
diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
index 6e2fcb6b1f2..56b07acc1d7 100644
--- a/libcpp/include/cpplib.h
+++ b/libcpp/include/cpplib.h
@@ -198,6 +198,7 @@ struct GTY(()) cpp_string {
 				    operator, or before this token
 				    after a # operator.  */
 #define NO_EXPAND	(1 << 10) /* Do not macro-expand this token.  */
+#define PRAGMA_OP	(1 << 11) /* _Pragma token.  */
 
 /* Specify which field, if any, of the cpp_token union is used.  */
 
diff --git a/libcpp/macro.c b/libcpp/macro.c
index f214548de1e..b2f797cae35 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -1259,7 +1259,7 @@ collect_args (cpp_reader *pfile, const cpp_hashnode *node,
 	  else if (token->type == CPP_EOF
 		   || (token->type == CPP_HASH && token->flags & BOL))
 	    break;
-	  else if (token->type == CPP_PRAGMA)
+	  else if (token->type == CPP_PRAGMA && !(token->flags & PRAGMA_OP))
 	    {
 	      cpp_token *newtok = _cpp_temp_token (pfile);
 

Re: [Patch] libcpp: Fix _Pragma expansion [PR102409]

[Jakub Jelinek]

On Fri, Oct 29, 2021 at 06:20:15PM +0200, Tobias Burnus wrote:
> On 29.10.21 13:06, Jakub Jelinek wrote:
> > On Thu, Oct 28, 2021 at 05:51:59PM +0200, Tobias Burnus wrote:
> > > libcpp/ChangeLog:
> > > 
> > >      PR c++/102409
> > >      * directives.c (destringize_and_run): Add PRAGMA_OP to the
> > >      CPP_PRAGMA token's flags to mark is as coming from _Pragma.
> > >      * include/cpplib.h (PRAGMA_OP): #define, to be used with token flags.
> > >      * macro.c (collect_args): Only handle CPP_PRAGMA special if PRAGMA_OP
> > >      is set.
> > > 
> > The patch itself looks reasonable to me, but it should come up with
> > testsuite coverage.  And the testsuite coverage should include both normal
> > testcases that do use integrated preprocessor, and the same with
> > -save-temps to make sure that even when preprocessing separately it works
> > too.
> 
> Yes, I realized myself that I missed to include a testcase – thanks for
> the -save-temps suggestion!
> 
> Updated patch enclosed.

Ok, thanks.
For backports, I'd wait a few weeks.

	Jakub

Re: [Patch] libcpp: Fix _Pragma expansion [PR102409]

[Tobias Burnus]

Hi Martin,

On 28.10.21 18:28, Martin Sebor wrote:
> There are a number of bug reports of _Pragma not working right
> in macros, including (and especially) to control diagnostics:
> https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=_Pragma%20macro&list_id=328003
>
>
> Just by the description this change seems like it could also
> fix some of them.

I think it does not help with them – or only partially.

I believe there currently still two issues:

* _Pragma("GCC foo") – when "foo" or "GCC foo" are
   not registered is immediately processed, leading
   to wrong placement in the output with "-E".

A probably not fully correct draft patch is attached to
https://gcc.gnu.org/PR90400 which fixes the issue
(misses a location before the #pragma).


* With _Pragma("GCC diagnostic") in macros the problem is:
The macro is replaced by all the macro code including the
#pragma and all other code in there.

By construction, all those have the same line. But if
   else { b--;
#pragma GCC diagnostic push
;
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
; a--;
#pragma GCC diagnostic pop
; }

the location is the input_location of the expanded macro,
i.e. all code is in the same line. As the 'pop' check checks
whether the loc is before the pragma, it might pop too early
and the 'ignored' is already ignored for the 'a--' in this
example.  Cf. https://gcc.gnu.org/PR91669

@Martin: As you seem to have spare cycles, how about spending
some time fixing either issue?

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