From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp76.iad3a.emailsrvr.com (smtp76.iad3a.emailsrvr.com [173.203.187.76]) by sourceware.org (Postfix) with ESMTPS id 1A1033858D28 for ; Sun, 31 Jul 2022 21:41:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1A1033858D28 X-Auth-ID: tom@honermann.net Received: by smtp26.relay.iad3a.emailsrvr.com (Authenticated sender: tom-AT-honermann.net) with ESMTPSA id A3BDA1A17; Sun, 31 Jul 2022 17:41:30 -0400 (EDT) Message-ID: <364afdc3-e84c-deea-7a17-e6a2fcf6115a@honermann.net> Date: Sun, 31 Jul 2022 17:41:30 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH 1/1] c++/106423: Fix pragma suppression of -Wc++20-compat diagnostics. Content-Language: en-US To: Lewis Hyatt , Joseph Myers Cc: gcc-patches List References: <20220724043902.1777378-1-tom@honermann.net> <20220724043902.1777378-2-tom@honermann.net> From: Tom Honermann In-Reply-To: X-Classification-ID: 84012cb7-0386-4ba9-8fa7-3cc9e09d9c0b-1-1 X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 31 Jul 2022 21:41:35 -0000 On 7/31/22 11:05 AM, Lewis Hyatt wrote: > On Sat, Jul 30, 2022 at 7:06 PM Tom Honermann via Gcc-patches > 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