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

[Jason Merrill <jason@redhat.com>]

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)