Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Lewis Hyatt]

Hello-

https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49

Would a C++ maintainer have some time to take a look at this patch
please? I feel like the PR is still worth resolving. If this doesn't
seem like a good way, I am happy to try another -- would really
appreciate any feedback. Thanks!

-Lewis

On Tue, May 24, 2022 at 5:28 PM Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> Hello-
>
> Now that we're back in stage 1, I thought it might be a better time to
> ask for feedback on this pair of patches that tries to resolve PR53431
> please?
>
> https://gcc.gnu.org/pipermail/gcc-patches/2021-December/587357.html
> https://gcc.gnu.org/pipermail/gcc-patches/2021-December/587358.html
>
> Part 1/2 is a trivial cleanup in the C++ parser that simplifies
> adding the support for early pragma handling.
>
> Part 2/2 adds the concept of early pragma handling and makes the C++
> and preprocessor frontends use it.
>
> The patches required some minor rebasing, so I have attached updated
> versions here.
>
> bootstrap + regtest all languages still looks good:
>
> FAIL 103 103
> PASS 541178 541213
> UNSUPPORTED 15177 15177
> UNTESTED 136 136
> XFAIL 4140 4140
> XPASS 17 17
>
> Thanks! If this approach doesn't seem like the right one, I am happy
> to try another way.
>
> -Lewis
>
>
> On Fri, Dec 24, 2021 at 04:23:08PM -0500, Lewis Hyatt wrote:
> > Hello-
> >
> > I would like please to follow up on this patch submitted for PR53431 here:
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586191.html
> >
> > However, it was suggested on the PR that part of it could be split into a
> > separate simpler patch. I have now done that, and also made a few tweaks to
> > the first version at the same time, so may I please request that you review
> > this version 2 instead? This email contains the first smaller cleanup patch,
> > and the next email contains the main part of it. Thanks very much.
> >
> > bootstrap and regtest were performed on x86-64 Linux, all tests look the same
> > before + after, plus the new passing testcases.
> >
> > FAIL 112 112
> > PASS 528007 528042
> > UNSUPPORTED 14888 14888
> > UNTESTED 132 132
> > XFAIL 3238 3238
> > XPASS 17 17
> >
> > -Lewis
>
> > From: Lewis Hyatt <lhyatt@gmail.com>
> > Date: Thu, 23 Dec 2021 17:03:04 -0500
> > Subject: [PATCH] c++: Minor cleanup in parser.c
> >
> > The code to determine whether a given token starts a module directive is
> > currently repeated in 4 places in parser.c. I am about to submit a patch
> > that needs to add it in a 5th place, so since the code is not completely
> > trivial (needing to check for 3 different token types), it seems worthwhile
> > to factor this logic into its own function.
> >
> > gcc/cp/ChangeLog:
> >
> >       * parser.c (cp_token_is_module_directive): New function
> >       refactoring common code.
> >       (cp_parser_skip_to_closing_parenthesis_1): Use the new function.
> >       (cp_parser_skip_to_end_of_statement): Likewise.
> >       (cp_parser_skip_to_end_of_block_or_statement): Likewise.
> >       (cp_parser_declaration): Likewise.
> >
> > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > index 33fb40a5b59..9b7446655be 100644
> > --- a/gcc/cp/parser.c
> > +++ b/gcc/cp/parser.c
> > @@ -629,6 +629,16 @@ cp_lexer_alloc (void)
> >    return lexer;
> >  }
> >
> > +/* Return TRUE if token is the start of a module declaration that will be
> > +   terminated by a CPP_PRAGMA_EOL token.  */
> > +static inline bool
> > +cp_token_is_module_directive (cp_token *token)
> > +{
> > +  return token->keyword == RID__EXPORT
> > +    || token->keyword == RID__MODULE
> > +    || token->keyword == RID__IMPORT;
> > +}
> > +
> >  /* Create a new main C++ lexer, the lexer that gets tokens from the
> >     preprocessor.  */
> >
> > @@ -3805,9 +3815,7 @@ cp_parser_skip_to_closing_parenthesis_1 (cp_parser *parser,
> >         break;
> >
> >       case CPP_KEYWORD:
> > -       if (token->keyword != RID__EXPORT
> > -           && token->keyword != RID__MODULE
> > -           && token->keyword != RID__IMPORT)
> > +       if (!cp_token_is_module_directive (token))
> >           break;
> >         /* FALLTHROUGH  */
> >
> > @@ -3908,9 +3916,7 @@ cp_parser_skip_to_end_of_statement (cp_parser* parser)
> >         break;
> >
> >       case CPP_KEYWORD:
> > -       if (token->keyword != RID__EXPORT
> > -           && token->keyword != RID__MODULE
> > -           && token->keyword != RID__IMPORT)
> > +       if (!cp_token_is_module_directive (token))
> >           break;
> >         /* FALLTHROUGH  */
> >
> > @@ -3997,9 +4003,7 @@ cp_parser_skip_to_end_of_block_or_statement (cp_parser* parser)
> >         break;
> >
> >       case CPP_KEYWORD:
> > -       if (token->keyword != RID__EXPORT
> > -           && token->keyword != RID__MODULE
> > -           && token->keyword != RID__IMPORT)
> > +       if (!cp_token_is_module_directive (token))
> >           break;
> >         /* FALLTHROUGH  */
> >
> > @@ -14860,9 +14864,7 @@ cp_parser_declaration (cp_parser* parser, tree prefix_attrs)
> >        else
> >       cp_parser_module_export (parser);
> >      }
> > -  else if (token1->keyword == RID__EXPORT
> > -        || token1->keyword == RID__IMPORT
> > -        || token1->keyword == RID__MODULE)
> > +  else if (cp_token_is_module_directive (token1))
> >      {
> >        bool exporting = token1->keyword == RID__EXPORT;
> >        cp_token *next = exporting ? token2 : token1;
>

Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Jason Merrill]

On 6/23/22 13:03, Lewis Hyatt via Gcc-patches wrote:
> Hello-
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49
> 
> Would a C++ maintainer have some time to take a look at this patch
> please? I feel like the PR is still worth resolving. If this doesn't
> seem like a good way, I am happy to try another -- would really
> appreciate any feedback. Thanks!

Thanks for your persistence, I'll take a look now.

Incidentally, when pinging it's often useful to ping someone from 
MAINTAINERS directly, as well as the list.  I think your last ping got 
eaten by some trouble Red Hat email was having at the time.

The cp_token_is_module_directive cleanup is OK.

> +  bool skip_this_pragma;

This member seems to be equivalent to
  in_pragma && !should_output_pragmas ()
Maybe it could be a member function instead of a data member?

More soon.

Jason

Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Jason Merrill]

On 6/29/22 12:59, Jason Merrill wrote:
> On 6/23/22 13:03, Lewis Hyatt via Gcc-patches wrote:
>> Hello-
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49
>>
>> Would a C++ maintainer have some time to take a look at this patch
>> please? I feel like the PR is still worth resolving. If this doesn't
>> seem like a good way, I am happy to try another -- would really
>> appreciate any feedback. Thanks!
> 
> Thanks for your persistence, I'll take a look now.
> 
> Incidentally, when pinging it's often useful to ping someone from 
> MAINTAINERS directly, as well as the list.  I think your last ping got 
> eaten by some trouble Red Hat email was having at the time.
> 
> The cp_token_is_module_directive cleanup is OK.
> 
>> +  bool skip_this_pragma;
> 
> This member seems to be equivalent to
>   in_pragma && !should_output_pragmas ()
> Maybe it could be a member function instead of a data member?
> 
> More soon.

Looks good, just a few minor comments:

> +      PD_KIND_INVALID,
> +      PD_KIND_PUSH,
> +      PD_KIND_POP,
> +      PD_KIND_IGNORED_ATTRIBUTES,
> +      PD_KIND_DIAGNOSTIC,

The PD_KIND_ prefix seems unnecessary for a scoped enum.

> +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
> +   directly from libcpp.  */
> +static void
> +pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)

Hmm, we could make a temporary lexer, but I guess this is short enough 
that the duplication is OK.

> +/* Similar, for the portion of a diagnostic pragma that was parsed
> +   internally and so not seen by our token streamer.  */

Can we rewind after parsing so that the token streamer sees it?

> +  if (early && arg)
> +    {
> +      /* This warning is needed because of PR71603 - popping the diagnostic
> +        state does not currently reset changes to option arguments, only
> +        changes to the option dispositions.  */
> +      warning_at (data.loc_option, OPT_Wpragmas,
> +                 "a diagnostic pragma attempting to modify a preprocessor"
> +                 " option argument may not work as expected");
> +    }

Maybe only warn if we subsequently see a pop?

> +/* Handle #pragma gcc diagnostic, which needs to be done during preprocessing
> +   for the case of preprocessing-related diagnostics.  */
> +static void
> +cp_lexer_handle_early_pragma (cp_lexer *lexer)

Let's mention in the comment that this is called before appending the 
CPP_PRAGMA_EOL to the lexer buffer.

Jason

Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Lewis Hyatt]

On Fri, Jul 1, 2022 at 3:59 PM Jason Merrill <jason@redhat.com> wrote:
>
> On 6/29/22 12:59, Jason Merrill wrote:
> > On 6/23/22 13:03, Lewis Hyatt via Gcc-patches wrote:
> >> Hello-
> >>
> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49
> >>
> >> Would a C++ maintainer have some time to take a look at this patch
> >> please? I feel like the PR is still worth resolving. If this doesn't
> >> seem like a good way, I am happy to try another -- would really
> >> appreciate any feedback. Thanks!
> >
> > Thanks for your persistence, I'll take a look now.
> >
> > Incidentally, when pinging it's often useful to ping someone from
> > MAINTAINERS directly, as well as the list.  I think your last ping got
> > eaten by some trouble Red Hat email was having at the time.
> >
> > The cp_token_is_module_directive cleanup is OK.

Thank you very much for the advice and for going through the patch, I
really appreciate it. I went ahead and pushed the small cleanup patch.
I have responses to your comments on the main patch below too.

> >
> >> +  bool skip_this_pragma;
> >
> > This member seems to be equivalent to
> >   in_pragma && !should_output_pragmas ()
> > Maybe it could be a member function instead of a data member?
> >

Yeah, makes sense, although I hope that by implementing your
suggestion below regarding rewinding the tokens for preprocessing,
then this can be removed anyway.

> > More soon.
>
> Looks good, just a few minor comments:
>
> > +      PD_KIND_INVALID,
> > +      PD_KIND_PUSH,
> > +      PD_KIND_POP,
> > +      PD_KIND_IGNORED_ATTRIBUTES,
> > +      PD_KIND_DIAGNOSTIC,
>
> The PD_KIND_ prefix seems unnecessary for a scoped enum.
>

Sure, will shorten it to PK_ instead.

> > +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
> > +   directly from libcpp.  */
> > +static void
> > +pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)
>
> Hmm, we could make a temporary lexer, but I guess this is short enough
> that the duplication is OK.
>

I see. It would look like a version of pragma_lex() (the one in
c-parser.cc) which took a c_parser* argument so it wouldn't need to
use the global the_parser. I didn't consider this because I was
starting from Manuel's prototype patch on the PR
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c10), which was
doing the parsing in libcpp itself. Perhaps it would make sense to
move to this approach in the future, if it became necessary sometime
to lex some other pragmas during preprocessing?

> > +/* Similar, for the portion of a diagnostic pragma that was parsed
> > +   internally and so not seen by our token streamer.  */
>
> Can we rewind after parsing so that the token streamer sees it?
>

Oh that's an interesting idea. It would avoid some potential issues.
For instance, I have another patch submitted to fix PR55971
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55971#c8), which is that
you can't put raw strings containing newlines into a preprocessing
directive. It occurs to me now, once that's applied, then I think a
#pragma GCC diagnostic with such a raw string (useless though it would
be) would not get output correctly by gcc -E with this current patch's
approach. An edge case certainly, but would be nice to get it right
also, and your approach would automatically handle it. I'm going to
explore this now and then follow up with a new version of the patch.

> > +  if (early && arg)
> > +    {
> > +      /* This warning is needed because of PR71603 - popping the diagnostic
> > +        state does not currently reset changes to option arguments, only
> > +        changes to the option dispositions.  */
> > +      warning_at (data.loc_option, OPT_Wpragmas,
> > +                 "a diagnostic pragma attempting to modify a preprocessor"
> > +                 " option argument may not work as expected");
> > +    }
>
> Maybe only warn if we subsequently see a pop?
>

Right, that makes sense. Changing the option does work just fine until
you try to pop it. But actually this warning was kinda an
afterthought, now I just checked and at the time I wrote it, there was
only one option it could possibly apply to, since it needs to be an
option that's both for libcpp, and taking an argument, which was
-Wnormalized=. In the meantime one more has been added, -Wbidi-chars=.
Rather than add more complicated logic to remember to warn on pop for
these 2 options only, feels like maybe it would be better to either
just fix PR71603 (which I can work on sometime), or add this warning
for all options, not just libcpp options, which I guess means it
should go inside the implementation of pop... So in either case feels
like it's not really relevant to this patch and I'd propose just to
remove it for now, and then address it subsequently?

> > +/* Handle #pragma gcc diagnostic, which needs to be done during preprocessing
> > +   for the case of preprocessing-related diagnostics.  */
> > +static void
> > +cp_lexer_handle_early_pragma (cp_lexer *lexer)
>
> Let's mention in the comment that this is called before appending the
> CPP_PRAGMA_EOL to the lexer buffer.

Will do.

Thanks again, I will work on a new version and reply to this thread soon.


-Lewis

Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Jason Merrill]

On 7/1/22 18:05, Lewis Hyatt wrote:
> On Fri, Jul 1, 2022 at 3:59 PM Jason Merrill <jason@redhat.com> wrote:
>>
>> On 6/29/22 12:59, Jason Merrill wrote:
>>> On 6/23/22 13:03, Lewis Hyatt via Gcc-patches wrote:
>>>> Hello-
>>>>
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49
>>>>
>>>> Would a C++ maintainer have some time to take a look at this patch
>>>> please? I feel like the PR is still worth resolving. If this doesn't
>>>> seem like a good way, I am happy to try another -- would really
>>>> appreciate any feedback. Thanks!
>>>
>>> Thanks for your persistence, I'll take a look now.
>>>
>>> Incidentally, when pinging it's often useful to ping someone from
>>> MAINTAINERS directly, as well as the list.  I think your last ping got
>>> eaten by some trouble Red Hat email was having at the time.
>>>
>>> The cp_token_is_module_directive cleanup is OK.
> 
> Thank you very much for the advice and for going through the patch, I
> really appreciate it. I went ahead and pushed the small cleanup patch.
> I have responses to your comments on the main patch below too.
> 
>>>
>>>> +  bool skip_this_pragma;
>>>
>>> This member seems to be equivalent to
>>>    in_pragma && !should_output_pragmas ()
>>> Maybe it could be a member function instead of a data member?
>>>
> 
> Yeah, makes sense, although I hope that by implementing your
> suggestion below regarding rewinding the tokens for preprocessing,
> then this can be removed anyway.
> 
>>> More soon.
>>
>> Looks good, just a few minor comments:
>>
>>> +      PD_KIND_INVALID,
>>> +      PD_KIND_PUSH,
>>> +      PD_KIND_POP,
>>> +      PD_KIND_IGNORED_ATTRIBUTES,
>>> +      PD_KIND_DIAGNOSTIC,
>>
>> The PD_KIND_ prefix seems unnecessary for a scoped enum.
>>
> 
> Sure, will shorten it to PK_ instead.
> 
>>> +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
>>> +   directly from libcpp.  */
>>> +static void
>>> +pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)
>>
>> Hmm, we could make a temporary lexer, but I guess this is short enough
>> that the duplication is OK.
> 
> I see. It would look like a version of pragma_lex() (the one in
> c-parser.cc) which took a c_parser* argument so it wouldn't need to
> use the global the_parser.

Or creates the_parser itself and feeds it tokens somewhat like 
cp_parser_handle_statement_omp_attributes.

> I didn't consider this because I was
> starting from Manuel's prototype patch on the PR
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c10), which was
> doing the parsing in libcpp itself. Perhaps it would make sense to
> move to this approach in the future, if it became necessary sometime
> to lex some other pragmas during preprocessing?

Sure.

>>> +/* Similar, for the portion of a diagnostic pragma that was parsed
>>> +   internally and so not seen by our token streamer.  */
>>
>> Can we rewind after parsing so that the token streamer sees it?
>>
> 
> Oh that's an interesting idea. It would avoid some potential issues.
> For instance, I have another patch submitted to fix PR55971
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55971#c8), which is that
> you can't put raw strings containing newlines into a preprocessing
> directive. It occurs to me now, once that's applied, then I think a
> #pragma GCC diagnostic with such a raw string (useless though it would
> be) would not get output correctly by gcc -E with this current patch's
> approach. An edge case certainly, but would be nice to get it right
> also, and your approach would automatically handle it. I'm going to
> explore this now and then follow up with a new version of the patch.
> 
>>> +  if (early && arg)
>>> +    {
>>> +      /* This warning is needed because of PR71603 - popping the diagnostic
>>> +        state does not currently reset changes to option arguments, only
>>> +        changes to the option dispositions.  */
>>> +      warning_at (data.loc_option, OPT_Wpragmas,
>>> +                 "a diagnostic pragma attempting to modify a preprocessor"
>>> +                 " option argument may not work as expected");
>>> +    }
>>
>> Maybe only warn if we subsequently see a pop?
>>
> 
> Right, that makes sense. Changing the option does work just fine until
> you try to pop it. But actually this warning was kinda an
> afterthought, now I just checked and at the time I wrote it, there was
> only one option it could possibly apply to, since it needs to be an
> option that's both for libcpp, and taking an argument, which was
> -Wnormalized=. In the meantime one more has been added, -Wbidi-chars=.
> Rather than add more complicated logic to remember to warn on pop for
> these 2 options only, feels like maybe it would be better to either
> just fix PR71603 (which I can work on sometime), or add this warning
> for all options, not just libcpp options, which I guess means it
> should go inside the implementation of pop... So in either case feels
> like it's not really relevant to this patch and I'd propose just to
> remove it for now, and then address it subsequently?

Sounds good.

>>> +/* Handle #pragma gcc diagnostic, which needs to be done during preprocessing
>>> +   for the case of preprocessing-related diagnostics.  */
>>> +static void
>>> +cp_lexer_handle_early_pragma (cp_lexer *lexer)
>>
>> Let's mention in the comment that this is called before appending the
>> CPP_PRAGMA_EOL to the lexer buffer.
> 
> Will do.
> 
> Thanks again, I will work on a new version and reply to this thread soon.
> 
> 
> -Lewis
> 

Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Lewis Hyatt]

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

On Fri, Jul 01, 2022 at 08:23:53PM -0400, Jason Merrill wrote:
> On 7/1/22 18:05, Lewis Hyatt wrote:
> > On Fri, Jul 1, 2022 at 3:59 PM Jason Merrill <jason@redhat.com> wrote:
> > > 
> > > On 6/29/22 12:59, Jason Merrill wrote:
> > > > On 6/23/22 13:03, Lewis Hyatt via Gcc-patches wrote:
> > > > > Hello-
> > > > > 
> > > > > https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
> > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49
> > > > > 
> > > > > Would a C++ maintainer have some time to take a look at this patch
> > > > > please? I feel like the PR is still worth resolving. If this doesn't
> > > > > seem like a good way, I am happy to try another -- would really
> > > > > appreciate any feedback. Thanks!
> > > > 
> > > > Thanks for your persistence, I'll take a look now.
> > > > 
> > > > Incidentally, when pinging it's often useful to ping someone from
> > > > MAINTAINERS directly, as well as the list.  I think your last ping got
> > > > eaten by some trouble Red Hat email was having at the time.
> > > > 
> > > > The cp_token_is_module_directive cleanup is OK.
> > 
> > Thank you very much for the advice and for going through the patch, I
> > really appreciate it. I went ahead and pushed the small cleanup patch.
> > I have responses to your comments on the main patch below too.
> > 
> > > > 
> > > > > +  bool skip_this_pragma;
> > > > 
> > > > This member seems to be equivalent to
> > > >    in_pragma && !should_output_pragmas ()
> > > > Maybe it could be a member function instead of a data member?
> > > > 
> > 
> > Yeah, makes sense, although I hope that by implementing your
> > suggestion below regarding rewinding the tokens for preprocessing,
> > then this can be removed anyway.
> > 
> > > > More soon.
> > > 
> > > Looks good, just a few minor comments:
> > > 
> > > > +      PD_KIND_INVALID,
> > > > +      PD_KIND_PUSH,
> > > > +      PD_KIND_POP,
> > > > +      PD_KIND_IGNORED_ATTRIBUTES,
> > > > +      PD_KIND_DIAGNOSTIC,
> > > 
> > > The PD_KIND_ prefix seems unnecessary for a scoped enum.
> > > 
> > 
> > Sure, will shorten it to PK_ instead.
> > 
> > > > +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
> > > > +   directly from libcpp.  */
> > > > +static void
> > > > +pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)
> > > 
> > > Hmm, we could make a temporary lexer, but I guess this is short enough
> > > that the duplication is OK.
> > 
> > I see. It would look like a version of pragma_lex() (the one in
> > c-parser.cc) which took a c_parser* argument so it wouldn't need to
> > use the global the_parser.
> 
> Or creates the_parser itself and feeds it tokens somewhat like
> cp_parser_handle_statement_omp_attributes.
> 
> > I didn't consider this because I was
> > starting from Manuel's prototype patch on the PR
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c10), which was
> > doing the parsing in libcpp itself. Perhaps it would make sense to
> > move to this approach in the future, if it became necessary sometime
> > to lex some other pragmas during preprocessing?
> 
> Sure.
> 
> > > > +/* Similar, for the portion of a diagnostic pragma that was parsed
> > > > +   internally and so not seen by our token streamer.  */
> > > 
> > > Can we rewind after parsing so that the token streamer sees it?
> > > 
> > 
> > Oh that's an interesting idea. It would avoid some potential issues.
> > For instance, I have another patch submitted to fix PR55971
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55971#c8), which is that
> > you can't put raw strings containing newlines into a preprocessing
> > directive. It occurs to me now, once that's applied, then I think a
> > #pragma GCC diagnostic with such a raw string (useless though it would
> > be) would not get output correctly by gcc -E with this current patch's
> > approach. An edge case certainly, but would be nice to get it right
> > also, and your approach would automatically handle it. I'm going to
> > explore this now and then follow up with a new version of the patch.
> > 
> > > > +  if (early && arg)
> > > > +    {
> > > > +      /* This warning is needed because of PR71603 - popping the diagnostic
> > > > +        state does not currently reset changes to option arguments, only
> > > > +        changes to the option dispositions.  */
> > > > +      warning_at (data.loc_option, OPT_Wpragmas,
> > > > +                 "a diagnostic pragma attempting to modify a preprocessor"
> > > > +                 " option argument may not work as expected");
> > > > +    }
> > > 
> > > Maybe only warn if we subsequently see a pop?
> > > 
> > 
> > Right, that makes sense. Changing the option does work just fine until
> > you try to pop it. But actually this warning was kinda an
> > afterthought, now I just checked and at the time I wrote it, there was
> > only one option it could possibly apply to, since it needs to be an
> > option that's both for libcpp, and taking an argument, which was
> > -Wnormalized=. In the meantime one more has been added, -Wbidi-chars=.
> > Rather than add more complicated logic to remember to warn on pop for
> > these 2 options only, feels like maybe it would be better to either
> > just fix PR71603 (which I can work on sometime), or add this warning
> > for all options, not just libcpp options, which I guess means it
> > should go inside the implementation of pop... So in either case feels
> > like it's not really relevant to this patch and I'd propose just to
> > remove it for now, and then address it subsequently?
> 
> Sounds good.
> 
> > > > +/* Handle #pragma gcc diagnostic, which needs to be done during preprocessing
> > > > +   for the case of preprocessing-related diagnostics.  */
> > > > +static void
> > > > +cp_lexer_handle_early_pragma (cp_lexer *lexer)
> > > 
> > > Let's mention in the comment that this is called before appending the
> > > CPP_PRAGMA_EOL to the lexer buffer.
> > 
> > Will do.
> > 
> > Thanks again, I will work on a new version and reply to this thread soon.
> > 

> 

Here is an updated patch addressing all your comments, thanks again for the
review. Regarding the idea about rewinding tokens, it seems potentially
problematic to try to make use of libcpp's backup token feature. I think there
can be pragmas which allow macro expansion, and then libcpp is not able to
step back through the expansion AFAIK. It would work fine for `#pragma GCC
diagnostic' but would break if a similar approach was added for some other
pragma allowing expansion in the future, and I think potentially also for
_Pragma. But it seems pretty tenable to handle this case just by providing an
interface in c-ppoutput.c to ask it to stream a given token that you have
lexed external to the token streaming process. That's how I set it up here,
it's much better than my first way IMHO, hopefully it looks OK?

I also attached a diff of this version vs the previous version in case that
makes it easier to review. The C++ parts were not touched other than adding
the comment you suggested, the changes in this version are mostly in
c-ppoutput.cc and c-pragma.cc.

Thanks again.

-Lewis

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

[PATCH] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

As discussed on PR c++/53431, currently, "#pragma GCC diagnostic" does
not always take effect for diagnostics generated by libcpp. The reason
is that libcpp itself does not interpret this pragma and only sends it on
to the frontend, hence the pragma is only honored if the frontend
arranges for it. The C frontend does process the pragma immediately
(more or less) after seeing the token, so things work fine there. The PR
points out that it doesn't work for C++, because the C++ frontend
doesn't handle anything until it has read all the tokens from
libcpp. The underlying problem is not C++-specific, though, and for
instance, gcc -E has the same issue.

This commit fixes the PR by adding the concept of an early pragma handler that
can be registered by frontends, which gives them a chance to process
diagnostic pragmas from libcpp before it is too late for them to take
effect. The C++ and preprocess-only frontends are modified to use early
pragmas and correct the behavior.

gcc/c-family/ChangeLog:

	PR preprocessor/53920
	PR c++/53431
	* c-common.cc (c_option_is_from_cpp_diagnostics): New function.
	* c-common.h (c_option_is_from_cpp_diagnostics): Declare.
	(c_pp_stream_token): Declare.
	* c-ppoutput.cc (init_pp_output): Refactor logic about skipping
	pragmas to...
	(should_output_pragmas): ...here. New function.
	(token_streamer::stream): Support handling early pragmas.
	(do_line_change): Likewise.
	(c_pp_stream_token): New function.
	* c-pragma.cc (struct pragma_diagnostic_data): New helper class.
	(pragma_diagnostic_lex_normal): New function. Moved logic for
	interpreting GCC diagnostic pragmas here.
	(pragma_diagnostic_lex_pp): New function for parsing diagnostic pragmas
	directly from libcpp.
	(handle_pragma_diagnostic): Refactor into helper function...
	(handle_pragma_diagnostic_impl): ...here.  New function.
	(handle_pragma_diagnostic_early): New function.
	(handle_pragma_diagnostic_early_pp): New function.
	(struct pragma_ns_name): Renamed to...
	(struct pragma_pp_data): ...this.  Add new "early_handler" member.
	(c_register_pragma_1): Support early pragmas in the preprocessor.
	(c_register_pragma_with_early_handler): New function.
	(c_register_pragma): Support the new early handlers in struct
	internal_pragma_handler.
	(c_register_pragma_with_data): Likewise.
	(c_register_pragma_with_expansion): Likewise.
	(c_register_pragma_with_expansion_and_data): Likewise.
	(c_invoke_early_pragma_handler): New function.
	(c_pp_invoke_early_pragma_handler): New function.
	(init_pragma): Add early pragma support for diagnostic pragmas.
	* c-pragma.h (struct internal_pragma_handler): Add new early handler
	members.
	(c_register_pragma_with_early_handler): Declare.
	(c_invoke_early_pragma_handler): Declare.
	(c_pp_invoke_early_pragma_handler): Declare.

gcc/cp/ChangeLog:

	PR c++/53431
	* parser.cc (cp_parser_pragma_kind): Move earlier in the file.
	(cp_lexer_handle_early_pragma): New function.
	(cp_lexer_new_main): Support parsing and handling early pragmas.
	(c_parse_file): Adapt to changes in cp_lexer_new_main.

gcc/testsuite/ChangeLog:

	PR preprocessor/53920
	PR c++/53431
	* c-c++-common/pragma-diag-11.c: New test.
	* c-c++-common/pragma-diag-12.c: New test.
	* c-c++-common/pragma-diag-13.c: New test.

diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
index c9c9e720bc8..1b8e73f7bc5 100644
--- a/gcc/c-family/c-common.cc
+++ b/gcc/c-family/c-common.cc
@@ -6611,6 +6611,20 @@ c_option_controlling_cpp_diagnostic (enum cpp_warning_reason reason)
   return 0;
 }
 
+/* Return TRUE if the given option index corresponds to a diagnostic
+   issued by libcpp.  Linear search seems fine for now.  */
+bool
+c_option_is_from_cpp_diagnostics (int option_index)
+{
+  for (auto entry = cpp_reason_option_codes; entry->reason != CPP_W_NONE;
+       ++entry)
+    {
+      if (entry->option_code == option_index)
+	return true;
+    }
+  return false;
+}
+
 /* Callback from cpp_diagnostic for PFILE to print diagnostics from the
    preprocessor.  The diagnostic is of type LEVEL, with REASON set
    to the reason code if LEVEL is represents a warning, at location
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index a1e6a55158d..c0900848965 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -911,6 +911,7 @@ extern tree fold_for_warn (tree);
 extern tree c_common_get_narrower (tree, int *);
 extern bool get_attribute_operand (tree, unsigned HOST_WIDE_INT *);
 extern void c_common_finalize_early_debug (void);
+extern bool c_option_is_from_cpp_diagnostics (int);
 
 /* Used by convert_and_check; in front ends.  */
 extern tree convert_init (tree, tree);
@@ -1191,6 +1192,7 @@ extern void preprocess_file (cpp_reader *);
 extern void pp_file_change (const line_map_ordinary *);
 extern void pp_dir_change (cpp_reader *, const char *);
 extern bool check_missing_format_attribute (tree, tree);
+extern void c_pp_stream_token (cpp_reader *, const cpp_token *, location_t loc);
 
 /* In c-omp.cc  */
 typedef wide_int_bitmask omp_clause_mask;
diff --git a/gcc/c-family/c-ppoutput.cc b/gcc/c-family/c-ppoutput.cc
index 9de46a9655f..cd38c969ea0 100644
--- a/gcc/c-family/c-ppoutput.cc
+++ b/gcc/c-family/c-ppoutput.cc
@@ -25,6 +25,8 @@
 #include "c-pragma.h"		/* For parse_in.  */
 #include "file-prefix-map.h"    /* remap_macro_filename()  */
 
+class token_streamer;
+
 /* Encapsulates state used to convert a stream of tokens into a text
    file.  */
 static struct
@@ -38,6 +40,8 @@ static struct
   bool prev_was_system_token;	/* True if the previous token was a
 				   system token.*/
   const char *src_file;		/* Current source file.  */
+  token_streamer *streamer;     /* Instance of class token_streamer using this
+				   object.  */
 } print;
 
 /* Defined and undefined macros being queued for output with -dU at
@@ -110,6 +114,14 @@ preprocess_file (cpp_reader *pfile)
     putc ('\n', print.outf);
 }
 
+/* Don't emit #pragma or #ident directives if we are processing
+   assembly language; the assembler may choke on them.  */
+static bool
+should_output_pragmas ()
+{
+  return cpp_get_options (parse_in)->lang != CLK_ASM;
+}
+
 /* Set up the callbacks as appropriate.  */
 void
 init_pp_output (FILE *out_stream)
@@ -119,9 +131,7 @@ init_pp_output (FILE *out_stream)
   if (!flag_no_output)
     {
       cb->line_change = cb_line_change;
-      /* Don't emit #pragma or #ident directives if we are processing
-	 assembly language; the assembler may choke on them.  */
-      if (cpp_get_options (parse_in)->lang != CLK_ASM)
+      if (should_output_pragmas ())
 	{
 	  cb->ident      = cb_ident;
 	  cb->def_pragma = cb_def_pragma;
@@ -163,6 +173,7 @@ init_pp_output (FILE *out_stream)
   print.first_time = 1;
   print.src_file = "";
   print.prev_was_system_token = false;
+  print.streamer = nullptr;
 }
 
 // FIXME: Ideally we'd just turn the entirety of the print struct into
@@ -183,6 +194,8 @@ class token_streamer
     in_pragma (false),
     line_marker_emitted (false)
     {
+      gcc_assert (!print.streamer);
+      print.streamer = this;
     }
 
   void begin_pragma () 
@@ -235,7 +248,7 @@ token_streamer::stream (cpp_reader *pfile, const cpp_token *token,
 	  print.printed = true;
 	}
     }
-  else if (token->flags & PREV_WHITE)
+  else if (token->flags & PREV_WHITE && token->type != CPP_PRAGMA)
     {
       unsigned src_line = LOCATION_LINE (loc);
 
@@ -252,22 +265,28 @@ token_streamer::stream (cpp_reader *pfile, const cpp_token *token,
   print.prev = token;
   if (token->type == CPP_PRAGMA)
     {
-      const char *space;
-      const char *name;
-
-      line_marker_emitted = maybe_print_line (token->src_loc);
-      fputs ("#pragma ", print.outf);
-      c_pp_lookup_pragma (token->val.pragma, &space, &name);
-      if (space)
-	fprintf (print.outf, "%s %s", space, name);
-      else
-	fprintf (print.outf, "%s", name);
-      print.printed = true;
       in_pragma = true;
+      if (should_output_pragmas ())
+	{
+	  const char *space;
+	  const char *name;
+
+	  line_marker_emitted = maybe_print_line (token->src_loc);
+	  fputs ("#pragma ", print.outf);
+	  c_pp_lookup_pragma (token->val.pragma, &space, &name);
+	  if (space)
+	    fprintf (print.outf, "%s %s", space, name);
+	  else
+	    fprintf (print.outf, "%s", name);
+	  print.printed = true;
+	}
+      if (token->val.pragma >= PRAGMA_FIRST_EXTERNAL)
+	c_pp_invoke_early_pragma_handler (token->val.pragma);
     }
   else if (token->type == CPP_PRAGMA_EOL)
     {
-      maybe_print_line (UNKNOWN_LOCATION);
+      if (should_output_pragmas ())
+	maybe_print_line (UNKNOWN_LOCATION);
       in_pragma = false;
     }
   else
@@ -287,9 +306,12 @@ token_streamer::stream (cpp_reader *pfile, const cpp_token *token,
 	  do_line_change (pfile, token, loc, false);
 	  print.prev_was_system_token = !!in_system_header_at (loc);
 	}
-      cpp_output_token (token, print.outf);
-      line_marker_emitted = false;
-      print.printed = true;
+      if (!in_pragma || should_output_pragmas ())
+	{
+	  cpp_output_token (token, print.outf);
+	  line_marker_emitted = false;
+	  print.printed = true;
+	}
     }
 
   /* CPP_COMMENT tokens and raw-string literal tokens can have
@@ -561,8 +583,12 @@ do_line_change (cpp_reader *pfile, const cpp_token *token,
      one space per column greater than 2, since scan_translation_unit
      will provide a space if PREV_WHITE.  Don't bother trying to
      reconstruct tabs; we can't get it right in general, and nothing
-     ought to care.  Some things do care; the fault lies with them.  */
-  if (!CPP_OPTION (pfile, traditional))
+     ought to care.  Some things do care; the fault lies with them.
+
+     Also do not output the spaces if this is a CPP_PRAGMA token.  In this
+     case, libcpp has provided the location of the first token after #pragma,
+     so we would start at the wrong column.  */
+  if (!CPP_OPTION (pfile, traditional) && token->type != CPP_PRAGMA)
     {
       int spaces = LOCATION_COLUMN (src_loc) - 2;
       print.printed = true;
@@ -782,6 +808,16 @@ cb_def_pragma (cpp_reader *pfile, location_t line)
   print.src_line++;
 }
 
+/* Stream a token as if we had seen it directly ourselves; needed
+   in case a token was lexed externally, e.g. while processing a
+   pragma.  */
+void
+c_pp_stream_token (cpp_reader *pfile, const cpp_token *tok, location_t loc)
+{
+  gcc_assert (print.streamer);
+  print.streamer->stream (pfile, tok, loc);
+}
+
 /* Dump out the hash table.  */
 static int
 dump_macro (cpp_reader *pfile, cpp_hashnode *node, void *v ATTRIBUTE_UNUSED)
diff --git a/gcc/c-family/c-pragma.cc b/gcc/c-family/c-pragma.cc
index cc05b2580fa..2b93e03852d 100644
--- a/gcc/c-family/c-pragma.cc
+++ b/gcc/c-family/c-pragma.cc
@@ -764,139 +764,319 @@ handle_pragma_visibility (cpp_reader *)
     warning (OPT_Wpragmas, "junk at end of %<#pragma GCC visibility%>");
 }
 
+/* Helper routines for parsing #pragma GCC diagnostic.  */
+class pragma_diagnostic_data
+{
+  pragma_diagnostic_data (const pragma_diagnostic_data &) = delete;
+  pragma_diagnostic_data& operator= (const pragma_diagnostic_data &) = delete;
+
+public:
+  bool valid;
+  location_t loc_kind, loc_option;
+  enum pd_kind_t
+    {
+      PK_INVALID,
+      PK_PUSH,
+      PK_POP,
+      PK_IGNORED_ATTRIBUTES,
+      PK_DIAGNOSTIC,
+    } pd_kind;
+  diagnostic_t diagnostic_kind;
+  const char *kind_str;
+  const char *option_str;
+  bool own_option_str;
+
+  pragma_diagnostic_data () { clear (); }
+  void clear ()
+  {
+    valid = false;
+    loc_kind = loc_option = UNKNOWN_LOCATION;
+    pd_kind = PK_INVALID;
+    diagnostic_kind = DK_UNSPECIFIED;
+    kind_str = option_str = nullptr;
+    own_option_str = false;
+  }
+
+  ~pragma_diagnostic_data ()
+  {
+    if (own_option_str && option_str)
+      XDELETEVEC (const_cast<char *> (option_str));
+  }
+
+  void set_kind (const char *kind_string)
+  {
+    kind_str = kind_string;
+
+    pd_kind = PK_INVALID;
+    diagnostic_kind = DK_UNSPECIFIED;
+    if (strcmp (kind_str, "push") == 0)
+      pd_kind = PK_PUSH;
+    else if (strcmp (kind_str, "pop") == 0)
+      pd_kind = PK_POP;
+    else if (strcmp (kind_str, "ignored_attributes") == 0)
+      pd_kind = PK_IGNORED_ATTRIBUTES;
+    else if (strcmp (kind_str, "error") == 0)
+      {
+	pd_kind = PK_DIAGNOSTIC;
+	diagnostic_kind = DK_ERROR;
+      }
+    else if (strcmp (kind_str, "warning") == 0)
+      {
+	pd_kind = PK_DIAGNOSTIC;
+	diagnostic_kind = DK_WARNING;
+      }
+    else if (strcmp (kind_str, "ignored") == 0)
+      {
+	pd_kind = PK_DIAGNOSTIC;
+	diagnostic_kind = DK_IGNORED;
+      }
+  }
+
+  bool needs_option () const
+  {
+    return pd_kind == PK_IGNORED_ATTRIBUTES
+      || pd_kind == PK_DIAGNOSTIC;
+  }
+
+};
+
+/* When compiling normally, use pragma_lex() to obtain the needed tokens.  This
+   will call into either the C or C++ frontends as appropriate.  */
 static void
-handle_pragma_diagnostic(cpp_reader *)
+pragma_diagnostic_lex_normal (pragma_diagnostic_data *result)
 {
+  result->clear ();
   tree x;
-  location_t loc;
-  enum cpp_ttype token = pragma_lex (&x, &loc);
-  if (token != CPP_NAME)
+  auto ttype = pragma_lex (&x, &result->loc_kind);
+  if (ttype != CPP_NAME)
+    return;
+  result->set_kind (IDENTIFIER_POINTER (x));
+  if (result->pd_kind == pragma_diagnostic_data::PK_INVALID)
+    return;
+
+  if (result->needs_option ())
     {
-      warning_at (loc, OPT_Wpragmas,
-		  "missing %<error%>, %<warning%>, %<ignored%>, %<push%>, "
-		  "%<pop%>, or %<ignored_attributes%> after "
-		  "%<#pragma GCC diagnostic%>");
-      return;
+      ttype = pragma_lex (&x, &result->loc_option);
+      if (ttype != CPP_STRING)
+	return;
+      result->option_str = TREE_STRING_POINTER (x);
     }
 
-  diagnostic_t kind;
-  const char *kind_string = IDENTIFIER_POINTER (x);
-  if (strcmp (kind_string, "error") == 0)
-    kind = DK_ERROR;
-  else if (strcmp (kind_string, "warning") == 0)
-    kind = DK_WARNING;
-  else if (strcmp (kind_string, "ignored") == 0)
-    kind = DK_IGNORED;
-  else if (strcmp (kind_string, "push") == 0)
+  result->valid = true;
+}
+
+/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
+   directly from libcpp.  We also need to inform the token streamer about all
+   tokens we lex ourselves here, so it outputs them too; this is done by calling
+   c_pp_stream_token () for each.  */
+static void
+pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)
+{
+  result->clear ();
+
+  auto tok = cpp_get_token_with_location (parse_in, &result->loc_kind);
+  c_pp_stream_token (parse_in, tok, result->loc_kind);
+  if (!(tok->type == CPP_NAME || tok->type == CPP_KEYWORD))
+    return;
+  const unsigned char *const kind_u = cpp_token_as_text (parse_in, tok);
+  result->set_kind ((const char *)kind_u);
+  if (result->pd_kind == pragma_diagnostic_data::PK_INVALID)
+    return;
+
+  if (result->needs_option ())
     {
-      diagnostic_push_diagnostics (global_dc, input_location);
-      return;
+      tok = cpp_get_token_with_location (parse_in, &result->loc_option);
+      c_pp_stream_token (parse_in, tok, result->loc_option);
+      if (tok->type != CPP_STRING)
+	return;
+      cpp_string str;
+      if (!cpp_interpret_string_notranslate (parse_in, &tok->val.str, 1, &str,
+					     CPP_STRING)
+	  || !str.len)
+	return;
+      result->option_str = (const char *)str.text;
+      result->own_option_str = true;
     }
-  else if (strcmp (kind_string, "pop") == 0)
+
+  result->valid = true;
+}
+
+/* Handle #pragma GCC diagnostic.  Early mode is used by frontends (such as C++)
+   that do not process the deferred pragma while they are consuming tokens; they
+   can use early mode to make sure diagnostics affecting the preprocessor itself
+   are correctly modified by the #pragma.  */
+template<bool early, bool is_pp> static void
+handle_pragma_diagnostic_impl ()
+{
+  static const bool want_diagnostics = (is_pp || !early);
+
+  pragma_diagnostic_data data;
+  if (is_pp)
+    pragma_diagnostic_lex_pp (&data);
+  else
+    pragma_diagnostic_lex_normal (&data);
+
+  if (!data.kind_str)
     {
-      diagnostic_pop_diagnostics (global_dc, input_location);
+      if (want_diagnostics)
+	warning_at (data.loc_kind, OPT_Wpragmas,
+		    "missing %<error%>, %<warning%>, %<ignored%>, %<push%>, "
+		    "%<pop%>, or %<ignored_attributes%> after "
+		    "%<#pragma GCC diagnostic%>");
       return;
     }
-  else if (strcmp (kind_string, "ignored_attributes") == 0)
+
+  switch (data.pd_kind)
     {
-      token = pragma_lex (&x, &loc);
-      if (token != CPP_STRING)
-	{
-	  warning_at (loc, OPT_Wpragmas,
-		      "missing attribute name after %<#pragma GCC diagnostic "
-		      "ignored_attributes%>");
-	  return;
-	}
-      char *args = xstrdup (TREE_STRING_POINTER (x));
-      const size_t l = strlen (args);
-      if (l == 0)
-	{
-	  warning_at (loc, OPT_Wpragmas, "missing argument to %<#pragma GCC "
-		      "diagnostic ignored_attributes%>");
-	  free (args);
+
+    case pragma_diagnostic_data::PK_PUSH:
+      diagnostic_push_diagnostics (global_dc, input_location);
+      return;
+
+    case pragma_diagnostic_data::PK_POP:
+      diagnostic_pop_diagnostics (global_dc, input_location);
+      return;
+
+    case pragma_diagnostic_data::PK_IGNORED_ATTRIBUTES:
+      {
+	if (early)
 	  return;
-	}
-      else if (args[l - 1] == ',')
+	if (!data.option_str)
+	  {
+	    warning_at (data.loc_option, OPT_Wpragmas,
+		       "missing attribute name after %<#pragma GCC diagnostic "
+			"ignored_attributes%>");
+	    return;
+	  }
+	char *args = xstrdup (data.option_str);
+	const size_t l = strlen (args);
+	if (l == 0)
+	  {
+	    warning_at (data.loc_option, OPT_Wpragmas,
+			"missing argument to %<#pragma GCC "
+			"diagnostic ignored_attributes%>");
+	    free (args);
+	    return;
+	  }
+	else if (args[l - 1] == ',')
+	  {
+	    warning_at (data.loc_option, OPT_Wpragmas,
+			"trailing %<,%> in arguments for "
+			"%<#pragma GCC diagnostic ignored_attributes%>");
+	    free (args);
+	    return;
+	  }
+	auto_vec<char *> v;
+	for (char *p = strtok (args, ","); p; p = strtok (NULL, ","))
+	  v.safe_push (p);
+	handle_ignored_attributes_option (&v);
+	free (args);
+	return;
+      }
+
+    case pragma_diagnostic_data::PK_DIAGNOSTIC:
+      if (!data.option_str)
 	{
-	  warning_at (loc, OPT_Wpragmas, "trailing %<,%> in arguments for "
-		      "%<#pragma GCC diagnostic ignored_attributes%>");
-	  free (args);
+	  if (want_diagnostics)
+	    warning_at (data.loc_option, OPT_Wpragmas,
+			"missing option after %<#pragma GCC diagnostic%> kind");
 	  return;
 	}
-      auto_vec<char *> v;
-      for (char *p = strtok (args, ","); p; p = strtok (NULL, ","))
-	v.safe_push (p);
-      handle_ignored_attributes_option (&v);
-      free (args);
-      return;
-    }
-  else
-    {
-      warning_at (loc, OPT_Wpragmas,
-		  "expected %<error%>, %<warning%>, %<ignored%>, %<push%>, "
-		  "%<pop%>, %<ignored_attributes%> after "
-		  "%<#pragma GCC diagnostic%>");
-      return;
-    }
+      break;
 
-  token = pragma_lex (&x, &loc);
-  if (token != CPP_STRING)
-    {
-      warning_at (loc, OPT_Wpragmas,
-		  "missing option after %<#pragma GCC diagnostic%> kind");
+    default:
+      if (want_diagnostics)
+	warning_at (data.loc_kind, OPT_Wpragmas,
+		    "expected %<error%>, %<warning%>, %<ignored%>, %<push%>, "
+		    "%<pop%>, %<ignored_attributes%> after "
+		    "%<#pragma GCC diagnostic%>");
       return;
+
     }
 
-  const char *option_string = TREE_STRING_POINTER (x);
+  gcc_assert (data.pd_kind == pragma_diagnostic_data::PK_DIAGNOSTIC);
+  gcc_assert (data.valid);
+
   unsigned int lang_mask = c_common_option_lang_mask () | CL_COMMON;
   /* option_string + 1 to skip the initial '-' */
-  unsigned int option_index = find_opt (option_string + 1, lang_mask);
+  unsigned int option_index = find_opt (data.option_str + 1, lang_mask);
+
+  if (early && !c_option_is_from_cpp_diagnostics (option_index))
+    return;
+
+  const char *arg = NULL;
+  if (cl_options[option_index].flags & CL_JOINED)
+    arg = data.option_str + 1 + cl_options[option_index].opt_len;
+
   if (option_index == OPT_SPECIAL_unknown)
     {
-      auto_diagnostic_group d;
-      if (warning_at (loc, OPT_Wpragmas,
-		      "unknown option after %<#pragma GCC diagnostic%> kind"))
+      if (want_diagnostics)
 	{
-	  option_proposer op;
-	  const char *hint = op.suggest_option (option_string + 1);
-	  if (hint)
-	    inform (loc, "did you mean %<-%s%>?", hint);
+	  auto_diagnostic_group d;
+	  if (warning_at (data.loc_option, OPT_Wpragmas,
+			"unknown option after %<#pragma GCC diagnostic%> kind"))
+	    {
+	      option_proposer op;
+	      const char *hint = op.suggest_option (data.option_str + 1);
+	      if (hint)
+		inform (data.loc_option, "did you mean %<-%s%>?", hint);
+	    }
 	}
       return;
     }
   else if (!(cl_options[option_index].flags & CL_WARNING))
     {
-      warning_at (loc, OPT_Wpragmas,
-		  "%qs is not an option that controls warnings", option_string);
+      if (want_diagnostics)
+	warning_at (data.loc_option, OPT_Wpragmas,
+		    "%qs is not an option that controls warnings",
+		    data.option_str);
       return;
     }
   else if (!(cl_options[option_index].flags & lang_mask))
     {
-      char *ok_langs = write_langs (cl_options[option_index].flags);
-      char *bad_lang = write_langs (c_common_option_lang_mask ());
-      warning_at (loc, OPT_Wpragmas,
-		  "option %qs is valid for %s but not for %s",
-		  option_string, ok_langs, bad_lang);
-      free (ok_langs);
-      free (bad_lang);
+      if (want_diagnostics)
+	{
+	  char *ok_langs = write_langs (cl_options[option_index].flags);
+	  char *bad_lang = write_langs (c_common_option_lang_mask ());
+	  warning_at (data.loc_option, OPT_Wpragmas,
+		      "option %qs is valid for %s but not for %s",
+		      data.option_str, ok_langs, bad_lang);
+	  free (ok_langs);
+	  free (bad_lang);
+	}
       return;
     }
 
   struct cl_option_handlers handlers;
   set_default_handlers (&handlers, NULL);
-  const char *arg = NULL;
-  if (cl_options[option_index].flags & CL_JOINED)
-    arg = option_string + 1 + cl_options[option_index].opt_len;
   /* FIXME: input_location isn't the best location here, but it is
      what we used to do here before and changing it breaks e.g.
      PR69543 and PR69558.  */
-  control_warning_option (option_index, (int) kind,
-			  arg, kind != DK_IGNORED,
+  control_warning_option (option_index, (int) data.diagnostic_kind,
+			  arg, data.diagnostic_kind != DK_IGNORED,
 			  input_location, lang_mask, &handlers,
 			  &global_options, &global_options_set,
 			  global_dc);
 }
 
+static void
+handle_pragma_diagnostic (cpp_reader *)
+{
+  handle_pragma_diagnostic_impl<false, false> ();
+}
+
+static void
+handle_pragma_diagnostic_early (cpp_reader *)
+{
+  handle_pragma_diagnostic_impl<true, false> ();
+}
+
+static void
+handle_pragma_diagnostic_early_pp (cpp_reader *)
+{
+  handle_pragma_diagnostic_impl<true, true> ();
+}
+
 /*  Parse #pragma GCC target (xxx) to set target specific options.  */
 static void
 handle_pragma_target(cpp_reader *)
@@ -1332,14 +1512,15 @@ handle_pragma_float_const_decimal64 (cpp_reader *)
 
 static vec<internal_pragma_handler> registered_pragmas;
 
-struct pragma_ns_name
+struct pragma_pp_data
 {
   const char *space;
   const char *name;
+  pragma_handler_1arg early_handler;
 };
 
 
-static vec<pragma_ns_name> registered_pp_pragmas;
+static vec<pragma_pp_data> registered_pp_pragmas;
 
 struct omp_pragma_def { const char *name; unsigned int id; };
 static const struct omp_pragma_def oacc_pragmas[] = {
@@ -1452,14 +1633,14 @@ c_register_pragma_1 (const char *space, const char *name,
 
   if (flag_preprocess_only)
     {
-      pragma_ns_name ns_name;
-
-      if (!allow_expansion)
+      if (!(allow_expansion || ihandler.early_handler.handler_1arg))
 	return;
 
-      ns_name.space = space;
-      ns_name.name = name;
-      registered_pp_pragmas.safe_push (ns_name);
+      pragma_pp_data pp_data;
+      pp_data.space = space;
+      pp_data.name = name;
+      pp_data.early_handler = ihandler.early_handler.handler_1arg;
+      registered_pp_pragmas.safe_push (pp_data);
       id = registered_pp_pragmas.length ();
       id += PRAGMA_FIRST_EXTERNAL - 1;
     }
@@ -1484,10 +1665,17 @@ c_register_pragma_1 (const char *space, const char *name,
 void
 c_register_pragma (const char *space, const char *name,
                    pragma_handler_1arg handler)
+{
+  c_register_pragma_with_early_handler (space, name, handler, nullptr);
+}
+void c_register_pragma_with_early_handler (const char *space, const char *name,
+					   pragma_handler_1arg handler,
+					   pragma_handler_1arg early_handler)
 {
   internal_pragma_handler ihandler;
 
   ihandler.handler.handler_1arg = handler;
+  ihandler.early_handler.handler_1arg = early_handler;
   ihandler.extra_data = false;
   ihandler.data = NULL;
   c_register_pragma_1 (space, name, ihandler, false);
@@ -1504,6 +1692,7 @@ c_register_pragma_with_data (const char *space, const char *name,
   internal_pragma_handler ihandler;
 
   ihandler.handler.handler_2arg = handler;
+  ihandler.early_handler.handler_2arg = nullptr;
   ihandler.extra_data = true;
   ihandler.data = data;
   c_register_pragma_1 (space, name, ihandler, false);
@@ -1523,6 +1712,7 @@ c_register_pragma_with_expansion (const char *space, const char *name,
   internal_pragma_handler ihandler;
 
   ihandler.handler.handler_1arg = handler;
+  ihandler.early_handler.handler_1arg = nullptr;
   ihandler.extra_data = false;
   ihandler.data = NULL;
   c_register_pragma_1 (space, name, ihandler, true);
@@ -1544,6 +1734,7 @@ c_register_pragma_with_expansion_and_data (const char *space, const char *name,
   internal_pragma_handler ihandler;
 
   ihandler.handler.handler_2arg = handler;
+  ihandler.early_handler.handler_2arg = nullptr;
   ihandler.extra_data = true;
   ihandler.data = data;
   c_register_pragma_1 (space, name, ihandler, true);
@@ -1570,6 +1761,38 @@ c_invoke_pragma_handler (unsigned int id)
     }
 }
 
+/* In contrast to the normal handler, the early handler is optional.  */
+void
+c_invoke_early_pragma_handler (unsigned int id)
+{
+  internal_pragma_handler *ihandler;
+  pragma_handler_1arg handler_1arg;
+  pragma_handler_2arg handler_2arg;
+
+  id -= PRAGMA_FIRST_EXTERNAL;
+  ihandler = &registered_pragmas[id];
+  if (ihandler->extra_data)
+    {
+      handler_2arg = ihandler->early_handler.handler_2arg;
+      if (handler_2arg)
+	handler_2arg (parse_in, ihandler->data);
+    }
+  else
+    {
+      handler_1arg = ihandler->early_handler.handler_1arg;
+      if (handler_1arg)
+	handler_1arg (parse_in);
+    }
+}
+
+void
+c_pp_invoke_early_pragma_handler (unsigned int id)
+{
+  const auto data = &registered_pp_pragmas[id - PRAGMA_FIRST_EXTERNAL];
+  if (data->early_handler)
+    data->early_handler (parse_in);
+}
+
 /* Set up front-end pragmas.  */
 void
 init_pragma (void)
@@ -1625,7 +1848,14 @@ init_pragma (void)
 
   c_register_pragma ("GCC", "visibility", handle_pragma_visibility);
 
-  c_register_pragma ("GCC", "diagnostic", handle_pragma_diagnostic);
+  if (flag_preprocess_only)
+    c_register_pragma_with_early_handler ("GCC", "diagnostic",
+					  nullptr,
+					  handle_pragma_diagnostic_early_pp);
+  else
+    c_register_pragma_with_early_handler ("GCC", "diagnostic",
+					  handle_pragma_diagnostic,
+					  handle_pragma_diagnostic_early);
   c_register_pragma ("GCC", "target", handle_pragma_target);
   c_register_pragma ("GCC", "optimize", handle_pragma_optimize);
   c_register_pragma ("GCC", "push_options", handle_pragma_push_options);
diff --git a/gcc/c-family/c-pragma.h b/gcc/c-family/c-pragma.h
index d5d4fe3490c..de806a647cd 100644
--- a/gcc/c-family/c-pragma.h
+++ b/gcc/c-family/c-pragma.h
@@ -219,7 +219,7 @@ union gen_pragma_handler {
 };
 /* Internally used to keep the data of the handler.  */
 struct internal_pragma_handler {
-  union gen_pragma_handler handler;
+  union gen_pragma_handler handler, early_handler;
   /* Permits to know if handler is a pragma_handler_1arg (extra_data is false)
      or a pragma_handler_2arg (extra_data is true).  */
   bool extra_data;
@@ -242,6 +242,17 @@ extern void c_register_pragma_with_expansion_and_data (const char *space,
                                                        void *data);
 extern void c_invoke_pragma_handler (unsigned int);
 
+/* Early pragma handlers run in addition to the normal ones.  They can be used
+   by frontends such as C++ that may want to process some pragmas during lexing
+   before they start processing them.  */
+extern void
+c_register_pragma_with_early_handler (const char *space, const char *name,
+				      pragma_handler_1arg handler,
+				      pragma_handler_1arg early_handler);
+extern void c_invoke_early_pragma_handler (unsigned int);
+extern void c_pp_invoke_early_pragma_handler (unsigned int);
+
+
 extern void maybe_apply_pragma_weak (tree);
 extern void maybe_apply_pending_pragma_weaks (void);
 extern tree maybe_apply_renaming_pragma (tree, tree);
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index df657a3fb2b..dde1e7f3975 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -639,8 +639,61 @@ cp_token_is_module_directive (cp_token *token)
     || token->keyword == RID__IMPORT;
 }
 
+/* Return TOKEN's pragma_kind if it is CPP_PRAGMA, otherwise
+   PRAGMA_NONE.  */
+
+static enum pragma_kind
+cp_parser_pragma_kind (cp_token *token)
+{
+  if (token->type != CPP_PRAGMA)
+    return PRAGMA_NONE;
+  /* We smuggled the cpp_token->u.pragma value in an INTEGER_CST.  */
+  return (enum pragma_kind) TREE_INT_CST_LOW (token->u.value);
+}
+
+/* Handle early pragmas such as #pragma GCC diagnostic, which needs to be done
+   during preprocessing for the case of preprocessing-related diagnostics.  This
+   is called immediately after pushing the CPP_PRAGMA_EOL token onto
+   lexer->buffer.  */
+
+static void
+cp_lexer_handle_early_pragma (cp_lexer *lexer)
+{
+  const auto first_token = lexer->buffer->address ();
+  const auto last_token = first_token + lexer->buffer->length () - 1;
+
+  /* Back up to the start of the pragma so pragma_lex () can parse it when
+     c-pragma lib asks it to.  */
+  auto begin = last_token;
+  gcc_assert (begin->type == CPP_PRAGMA_EOL);
+  while (begin->type != CPP_PRAGMA)
+    {
+      if (cp_token_is_module_directive (begin))
+	return;
+      gcc_assert (begin != first_token);
+      --begin;
+    }
+  gcc_assert (!lexer->next_token);
+  gcc_assert (!lexer->last_token);
+  lexer->next_token = begin;
+  lexer->last_token = last_token;
+
+  /* Dispatch it.  */
+  const unsigned int id
+    = cp_parser_pragma_kind (cp_lexer_consume_token (lexer));
+  if (id >= PRAGMA_FIRST_EXTERNAL)
+    c_invoke_early_pragma_handler (id);
+
+  /* Reset to normal state.  */
+  lexer->next_token = lexer->last_token = nullptr;
+}
+
+/* The parser.  */
+static cp_parser *cp_parser_new (cp_lexer *);
+static GTY (()) cp_parser *the_parser;
+
 /* Create a new main C++ lexer, the lexer that gets tokens from the
-   preprocessor.  */
+   preprocessor, and also create the main parser.  */
 
 static cp_lexer *
 cp_lexer_new_main (void)
@@ -662,6 +715,10 @@ cp_lexer_new_main (void)
   if (modules_p ())
     filter = module_token_cdtor (parse_in, filter);
 
+  /* Create the parser now, so we can use it to handle early pragmas.  */
+  gcc_assert (!the_parser);
+  the_parser = cp_parser_new (lexer);
+
   /* Get the remaining tokens from the preprocessor.  */
   while (tok->type != CPP_EOF)
     {
@@ -669,6 +726,11 @@ cp_lexer_new_main (void)
 	/* Process the previous token.  */
 	module_token_lang (tok->type, tok->keyword, tok->u.value,
 			   tok->location, filter);
+
+      /* Check for early pragmas that need to be handled now.  */
+      if (tok->type == CPP_PRAGMA_EOL)
+	cp_lexer_handle_early_pragma (lexer);
+
       tok = vec_safe_push (lexer->buffer, cp_token ());
       cp_lexer_get_preprocessor_token (C_LEX_STRING_NO_JOIN, tok);
     }
@@ -2131,11 +2193,6 @@ pop_unparsed_function_queues (cp_parser *parser)
 
 /* Prototypes.  */
 
-/* Constructors and destructors.  */
-
-static cp_parser *cp_parser_new
-  (cp_lexer *);
-
 /* Routines to parse various constructs.
 
    Those that return `tree' will return the error_mark_node (rather
@@ -2898,18 +2955,6 @@ cp_parser_is_keyword (cp_token* token, enum rid keyword)
   return token->keyword == keyword;
 }
 
-/* Return TOKEN's pragma_kind if it is CPP_PRAGMA, otherwise
-   PRAGMA_NONE.  */
-
-static enum pragma_kind
-cp_parser_pragma_kind (cp_token *token)
-{
-  if (token->type != CPP_PRAGMA)
-    return PRAGMA_NONE;
-  /* We smuggled the cpp_token->u.pragma value in an INTEGER_CST.  */
-  return (enum pragma_kind) TREE_INT_CST_LOW (token->u.value);
-}
-
 /* Helper function for cp_parser_error.
    Having peeked a token of kind TOK1_KIND that might signify
    a conflict marker, peek successor tokens to determine
@@ -47940,11 +47985,7 @@ cp_parser_transaction_cancel (cp_parser *parser)
   return stmt;
 }
 \f
-/* The parser.  */
-
-static GTY (()) cp_parser *the_parser;
 
-\f
 /* Special handling for the first token or line in the file.  The first
    thing in the file might be #pragma GCC pch_preprocess, which loads a
    PCH file, which is a GC collection point.  So we need to handle this
@@ -48439,9 +48480,7 @@ c_parse_file (void)
 
   /* cp_lexer_new_main is called before doing any GC allocation
      because tokenization might load a PCH file.  */
-  cp_lexer *lexer = cp_lexer_new_main ();
-
-  the_parser = cp_parser_new (lexer);
+  cp_lexer_new_main ();
 
   cp_parser_translation_unit (the_parser);
   class_decl_loc_t::diag_mismatched_tags ();
diff --git a/gcc/testsuite/c-c++-common/pragma-diag-11.c b/gcc/testsuite/c-c++-common/pragma-diag-11.c
new file mode 100644
index 00000000000..2eef5c418df
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pragma-diag-11.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-Wundef" } */
+#pragma GCC diagnostic ignored "-Wundef"
+#if FOO
+#endif
+#define P _Pragma ("GCC diagnostic push") _Pragma ("GCC diagnostic warning \"-Wundef\"")
+P
+#if FOO2 /* { dg-warning "is not defined" } */
+#endif
+#pragma GCC diagnostic pop
+#if FOO3
+#endif
+int i;
diff --git a/gcc/testsuite/c-c++-common/pragma-diag-12.c b/gcc/testsuite/c-c++-common/pragma-diag-12.c
new file mode 100644
index 00000000000..0043a4287a0
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pragma-diag-12.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-E -Wdate-time" } */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdate-time"
+const char *date = __DATE__;
+_Pragma ("GCC diagnostic pop");
+const char *date2 = __DATE__; /* { dg-warning "__DATE__" } */
+/* { dg-final { scan-assembler "#pragma GCC diagnostic push" } } */
+/* { dg-final { scan-assembler "#pragma GCC diagnostic ignored \"-Wdate-time\"" } } */
+/* { dg-final { scan-assembler "#pragma GCC diagnostic pop" } } */
diff --git a/gcc/testsuite/c-c++-common/pragma-diag-13.c b/gcc/testsuite/c-c++-common/pragma-diag-13.c
new file mode 100644
index 00000000000..d67b3655639
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pragma-diag-13.c
@@ -0,0 +1,4 @@
+/* { dg-do compile } */
+#pragma GCC diagnostic /* { dg-warning "missing" } */
+#pragma GCC diagnostic warn /* { dg-warning "24:expected" } */
+#pragma GCC diagnostic ignored "-Wfoo" /* { dg-warning "32:unknown" } */

[-- Attachment #3: diff_vs_v2.txt.gz --]
[-- Type: application/x-gunzip, Size: 3565 bytes --]

Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Jason Merrill]

On 7/6/22 10:23, Lewis Hyatt wrote:
> On Fri, Jul 01, 2022 at 08:23:53PM -0400, Jason Merrill wrote:
>> On 7/1/22 18:05, Lewis Hyatt wrote:
>>> On Fri, Jul 1, 2022 at 3:59 PM Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> On 6/29/22 12:59, Jason Merrill wrote:
>>>>> On 6/23/22 13:03, Lewis Hyatt via Gcc-patches wrote:
>>>>>> Hello-
>>>>>>
>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595556.html
>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c49
>>>>>>
>>>>>> Would a C++ maintainer have some time to take a look at this patch
>>>>>> please? I feel like the PR is still worth resolving. If this doesn't
>>>>>> seem like a good way, I am happy to try another -- would really
>>>>>> appreciate any feedback. Thanks!
>>>>>
>>>>> Thanks for your persistence, I'll take a look now.
>>>>>
>>>>> Incidentally, when pinging it's often useful to ping someone from
>>>>> MAINTAINERS directly, as well as the list.  I think your last ping got
>>>>> eaten by some trouble Red Hat email was having at the time.
>>>>>
>>>>> The cp_token_is_module_directive cleanup is OK.
>>>
>>> Thank you very much for the advice and for going through the patch, I
>>> really appreciate it. I went ahead and pushed the small cleanup patch.
>>> I have responses to your comments on the main patch below too.
>>>
>>>>>
>>>>>> +  bool skip_this_pragma;
>>>>>
>>>>> This member seems to be equivalent to
>>>>>     in_pragma && !should_output_pragmas ()
>>>>> Maybe it could be a member function instead of a data member?
>>>>>
>>>
>>> Yeah, makes sense, although I hope that by implementing your
>>> suggestion below regarding rewinding the tokens for preprocessing,
>>> then this can be removed anyway.
>>>
>>>>> More soon.
>>>>
>>>> Looks good, just a few minor comments:
>>>>
>>>>> +      PD_KIND_INVALID,
>>>>> +      PD_KIND_PUSH,
>>>>> +      PD_KIND_POP,
>>>>> +      PD_KIND_IGNORED_ATTRIBUTES,
>>>>> +      PD_KIND_DIAGNOSTIC,
>>>>
>>>> The PD_KIND_ prefix seems unnecessary for a scoped enum.
>>>>
>>>
>>> Sure, will shorten it to PK_ instead.
>>>
>>>>> +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
>>>>> +   directly from libcpp.  */
>>>>> +static void
>>>>> +pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)
>>>>
>>>> Hmm, we could make a temporary lexer, but I guess this is short enough
>>>> that the duplication is OK.
>>>
>>> I see. It would look like a version of pragma_lex() (the one in
>>> c-parser.cc) which took a c_parser* argument so it wouldn't need to
>>> use the global the_parser.
>>
>> Or creates the_parser itself and feeds it tokens somewhat like
>> cp_parser_handle_statement_omp_attributes.
>>
>>> I didn't consider this because I was
>>> starting from Manuel's prototype patch on the PR
>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c10), which was
>>> doing the parsing in libcpp itself. Perhaps it would make sense to
>>> move to this approach in the future, if it became necessary sometime
>>> to lex some other pragmas during preprocessing?
>>
>> Sure.
>>
>>>>> +/* Similar, for the portion of a diagnostic pragma that was parsed
>>>>> +   internally and so not seen by our token streamer.  */
>>>>
>>>> Can we rewind after parsing so that the token streamer sees it?
>>>>
>>>
>>> Oh that's an interesting idea. It would avoid some potential issues.
>>> For instance, I have another patch submitted to fix PR55971
>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55971#c8), which is that
>>> you can't put raw strings containing newlines into a preprocessing
>>> directive. It occurs to me now, once that's applied, then I think a
>>> #pragma GCC diagnostic with such a raw string (useless though it would
>>> be) would not get output correctly by gcc -E with this current patch's
>>> approach. An edge case certainly, but would be nice to get it right
>>> also, and your approach would automatically handle it. I'm going to
>>> explore this now and then follow up with a new version of the patch.
>>>
>>>>> +  if (early && arg)
>>>>> +    {
>>>>> +      /* This warning is needed because of PR71603 - popping the diagnostic
>>>>> +        state does not currently reset changes to option arguments, only
>>>>> +        changes to the option dispositions.  */
>>>>> +      warning_at (data.loc_option, OPT_Wpragmas,
>>>>> +                 "a diagnostic pragma attempting to modify a preprocessor"
>>>>> +                 " option argument may not work as expected");
>>>>> +    }
>>>>
>>>> Maybe only warn if we subsequently see a pop?
>>>>
>>>
>>> Right, that makes sense. Changing the option does work just fine until
>>> you try to pop it. But actually this warning was kinda an
>>> afterthought, now I just checked and at the time I wrote it, there was
>>> only one option it could possibly apply to, since it needs to be an
>>> option that's both for libcpp, and taking an argument, which was
>>> -Wnormalized=. In the meantime one more has been added, -Wbidi-chars=.
>>> Rather than add more complicated logic to remember to warn on pop for
>>> these 2 options only, feels like maybe it would be better to either
>>> just fix PR71603 (which I can work on sometime), or add this warning
>>> for all options, not just libcpp options, which I guess means it
>>> should go inside the implementation of pop... So in either case feels
>>> like it's not really relevant to this patch and I'd propose just to
>>> remove it for now, and then address it subsequently?
>>
>> Sounds good.
>>
>>>>> +/* Handle #pragma gcc diagnostic, which needs to be done during preprocessing
>>>>> +   for the case of preprocessing-related diagnostics.  */
>>>>> +static void
>>>>> +cp_lexer_handle_early_pragma (cp_lexer *lexer)
>>>>
>>>> Let's mention in the comment that this is called before appending the
>>>> CPP_PRAGMA_EOL to the lexer buffer.
>>>
>>> Will do.
>>>
>>> Thanks again, I will work on a new version and reply to this thread soon.
> 
> Here is an updated patch addressing all your comments, thanks again for the
> review. Regarding the idea about rewinding tokens, it seems potentially
> problematic to try to make use of libcpp's backup token feature. I think there
> can be pragmas which allow macro expansion, and then libcpp is not able to
> step back through the expansion AFAIK. It would work fine for `#pragma GCC
> diagnostic' but would break if a similar approach was added for some other
> pragma allowing expansion in the future, and I think potentially also for
> _Pragma. But it seems pretty tenable to handle this case just by providing an
> interface in c-ppoutput.c to ask it to stream a given token that you have
> lexed external to the token streaming process. That's how I set it up here,
> it's much better than my first way IMHO, hopefully it looks OK?
> 
> I also attached a diff of this version vs the previous version in case that
> makes it easier to review. The C++ parts were not touched other than adding
> the comment you suggested, the changes in this version are mostly in
> c-ppoutput.cc and c-pragma.cc.


> +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
> +   directly from libcpp.  We also need to inform the token streamer about all
> +   tokens we lex ourselves here, so it outputs them too; this is done by calling
> +   c_pp_stream_token () for each.  */

Let's add to this comment

??? If we need to support more pragmas in the future, maybe initialize 
this_parser with the pragma tokens and call pragma_lex?

OK with that change.

> +static void
> +pragma_diagnostic_lex_pp (pragma_diagnostic_data *result)

Re: Ping^2: [PATCH v2] diagnostics: Honor #pragma GCC diagnostic in the preprocessor [PR53431]

[Lewis Hyatt]

On Wed, Jul 6, 2022 at 2:17 PM Jason Merrill <jason@redhat.com> wrote:
>> On 7/6/22 10:23, Lewis Hyatt wrote:
> > Here is an updated patch addressing all your comments, thanks again for the
> > review. Regarding the idea about rewinding tokens, it seems potentially
> > problematic to try to make use of libcpp's backup token feature. I think there
> > can be pragmas which allow macro expansion, and then libcpp is not able to
> > step back through the expansion AFAIK. It would work fine for `#pragma GCC
> > diagnostic' but would break if a similar approach was added for some other
> > pragma allowing expansion in the future, and I think potentially also for
> > _Pragma. But it seems pretty tenable to handle this case just by providing an
> > interface in c-ppoutput.c to ask it to stream a given token that you have
> > lexed external to the token streaming process. That's how I set it up here,
> > it's much better than my first way IMHO, hopefully it looks OK?
> >
> > I also attached a diff of this version vs the previous version in case that
> > makes it easier to review. The C++ parts were not touched other than adding
> > the comment you suggested, the changes in this version are mostly in
> > c-ppoutput.cc and c-pragma.cc.
>
>
> > +/* When preprocessing only, pragma_lex() is not available, so obtain the tokens
> > +   directly from libcpp.  We also need to inform the token streamer about all
> > +   tokens we lex ourselves here, so it outputs them too; this is done by calling
> > +   c_pp_stream_token () for each.  */
>
> Let's add to this comment
>
> ??? If we need to support more pragmas in the future, maybe initialize
> this_parser with the pragma tokens and call pragma_lex?
>
> OK with that change.
>

Thank you very much, done.


-Lewis