Re: [PATCH 1/1] c++/106423: Fix pragma suppression of -Wc++20-compat diagnostics.

[Tom Honermann <tom@honermann.net>]

On 7/31/22 11:05 AM, Lewis Hyatt wrote:
> On Sat, Jul 30, 2022 at 7:06 PM Tom Honermann via Gcc-patches
> <gcc-patches@gcc.gnu.org>  wrote:
>> On 7/27/22 7:09 PM, Joseph Myers wrote:
>>> On Sun, 24 Jul 2022, Tom Honermann via Gcc-patches wrote:
>>>
>>>> Gcc's '#pragma GCC diagnostic' directives are processed in "early mode"
>>>> (see handle_pragma_diagnostic_early) for the C++ frontend and, as such,
>>>> require that the target diagnostic option be enabled for the preprocessor
>>>> (see c_option_is_from_cpp_diagnostics).  This change modifies the
>>>> -Wc++20-compat option definition to register it as a preprocessor option
>>>> so that its associated diagnostics can be suppressed.  The changes also
>>> There are lots of C++ warning options, all of which should support pragma
>>> suppression regardless of whether they are relevant to the preprocessor or
>>> not.  Do they all need this kind of handling, or is it only -Wc++20-compat
>>> that has some kind of problem?
>> I had only checked -Wc++20-compat when working on the patch.
>>
>> I did some spot checking now and confirmed that suppression works as
>> expected for C++ for at least the following warnings:
>>     -Wuninitialized
>>     -Warray-compare
>>     -Wbool-compare
>>     -Wtautological-compare
>>     -Wterminate
>>
>> I don't know the diagnostic framework well. As best I can tell, this
>> issue is specific to the -Wc++20-compat option and when the particular
>> diagnostic is issued (e.g., during lexing as opposed to during parsing).
>> The following call chains appear to be relevant.
>>     cp_lexer_new_main -> cp_lexer_handle_early_pragma ->
>> c_invoke_early_pragma_handler
>>     cp_parser_* -> cp_parser_pragma -> c_invoke_pragma_handler
>>     (where * might be "declaration", "toplevel_declaration",
>> "class_head", "objc_interstitial_code", ...)
>>
>> The -Wc++20-compat enabled warning regarding new keywords in C++20 is
>> issued from cp_lexer_get_preprocessor_token.
>>
>> Tom.
>>
> I have been working on improving the handling of "#pragma GCC
> diagnostic" lately. The behavior for C++ changed since r13-1544
> (https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=e46f4d7430c5210465791603735ab219ef263c51).
> I have some more comments about the patch's approach on the PR
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431#c44).
>
> "#pragma GCC diagnostic" formerly did not work in C++ at all, for
> diagnostics generated by libcpp, because C++ obtains all the tokens
> from libcpp first (including deferred pragmas), and then processes
> them afterward, too late to take effect for diagnostics that libcpp
> has already emitted. r13-1544 fixed this up by adding an early pragma
> handler, which runs as soon as a deferred pragma token is seen and
> handles diagnostic pragmas if they pertain to libcpp-controlled
> diagnostics. Non-libcpp diagnostics still need to be handled later,
> during parsing, or else they get processed too early and it leads to
> other problems. Basically, now each diagnostic pragma is handled as
> close in time as possible to the time the associated diagnostics might
> be generated.
>
> The early pragma handler determines that an option comes from libcpp,
> and so should be subject to early processing, if it was marked as such
> in the options definition file. Tom's patch points out that
> -Wc++20-compat needs to be handled early, and so marking it as a
> libcpp diagnostic in c-family/c.opt arranges for that to work as
> intended. Now one potential objection here is that -Wc++20-compat
> warnings are not technically generated by libcpp. They are generated
> by the C++ frontend immediately after lexing an identifier token from
> libcpp (cp_lexer_get_preprocessor_token()). But the distinction
> between these two steps is rather blurry and it seems logical to me,
> to denote this as a libcpp-related option. Also, the same is already
> done for -Wc++11-compat. Otherwise, we would need to add some new
> option property to indicate which ones need to be handled for pragmas
> at lexing time rather than parsing time.
>
> At the moment I don't see any other diagnostics issued from
> cp_lexer_get_preprocessor_token() that would need similar adjustments.
> Assuming the approach is OK, it might be nice to add a comment to that
> function, indicating that any diagnostics emitted there should be
> annotated as libcpp options in the .opt file?

Thank you for those details; I wasn't aware of that history.

If I'm interpreting your response correctly, it sounds like you agree 
with the direction of the patch.

If you like, I can add a comment as you suggested and re-post the patch. 
Perhaps:

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 4f67441eeb1..c3584446827 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -924,7 +924,10 @@cp_lexer_saving_tokens (const cp_lexer* lexer)
/* Store the next token from the preprocessor in *TOKEN.  Return true
    if we reach EOF.  If LEXER is NULL, assume we are handling an
    initial #pragma pch_preprocess, and thus want the lexer to return
-   processed strings.  */
+   processed strings.
+
+   Diagnostics issued from this function must have their controlling 
option (if
+   any) in c.opt annotated as a libcpp option via the CppReason 
property.  */

static void
cp_lexer_get_preprocessor_token (unsigned flags, cp_token *token)

Tom.

> -Lewis