From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 603DB3AAB033 for ; Fri, 11 Jun 2021 17:04:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 603DB3AAB033 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-147-wNN6dWHmNdeu4hjfSledwA-1; Fri, 11 Jun 2021 13:04:44 -0400 X-MC-Unique: wNN6dWHmNdeu4hjfSledwA-1 Received: by mail-qv1-f70.google.com with SMTP id 2-20020a0562140d62b02902357adaa890so9390145qvs.20 for ; Fri, 11 Jun 2021 10:04:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=lIsll51tsxfua7czSmmjPqjU+81GlY8ew7Ft1KPNz7E=; b=FrfVGbdzYOFs0cwR+WoDliDjeDoiKVJJPsUSWtO/J1fpZMTZ6T8hsjgZNyuZqNqjEZ 04PjOZ2bYxY91KNr9y3PFuVobJTNW7jgmfG0aJia/NoEzrAz6Hhk0Bb5XF5pd8xgSQGU 4PoyK+YujjAPc1hIUaTIkzDJmU9ed+ZmFo1Aiy52bYIi/JC/SFwTZskoWV2PRZYImGpq HsKUpop8J4lp+LYOUn4Hp0q3qrl7xSqYkX7dBIhztX1Y5rUXoNnvV9/kERS0/+JJCNKt vD6tjSZ/cFQsi1kjIoI0ASNC2C1lvv4S4+kjDEPBMIypZXKb1UGA2h0K10tilxZbeMnp Hj/Q== X-Gm-Message-State: AOAM532JIND7VPsGMTk+o3eDPxIH4+Dt1CDIhWYcA+SXmMXEsKCGXcCy g3042YxzcOQx3Sfz9C1R6OtWCYyqVLCOaKEP6hoVnlIFAdeFeHn4o+YLJnZWf04VFYcOJsZaClm Oec9kzXaCOYDUsPKmKg== X-Received: by 2002:ac8:7d87:: with SMTP id c7mr4809738qtd.258.1623431083754; Fri, 11 Jun 2021 10:04:43 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz4Tne8SETHGRqhnkQ2RvOep+S7XK2M+dtTTSfK6N2k99MzZfKbDiuFZdGwIs3pn7Hiaib5Fg== X-Received: by 2002:ac8:7d87:: with SMTP id c7mr4809714qtd.258.1623431083485; Fri, 11 Jun 2021 10:04:43 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id h6sm4567807qtr.73.2021.06.11.10.04.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Jun 2021 10:04:43 -0700 (PDT) Message-ID: <314ae442b5972ed23468e20698e678a26ff1bc1e.camel@redhat.com> Subject: Re: [PATCH 1/4] introduce diagnostic infrastructure changes (PR 98512) From: David Malcolm To: Martin Sebor , gcc-patches Date: Fri, 11 Jun 2021 13:04:41 -0400 In-Reply-To: References: <2e49b6c6-a403-a207-c41e-58f78df96b84@gmail.com> <945093c7-de5e-0350-6030-e4e79ea41161@gmail.com> <5aff247f-dfcb-cecf-e07e-b5fca877f911@gmail.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: Fri, 11 Jun 2021 17:04:49 -0000 On Thu, 2021-06-10 at 17:26 -0600, Martin Sebor wrote: > This diff introduces the diagnostic infrastructure changes to support > controlling warnings at any call site in the inlining stack and > printing > the inlining context without the %K and %G directives. Thanks for working on this, looks very promising. > Improve warning suppression for inlined functions. > > Resolves: > PR middle-end/98871 - Cannot silence -Wmaybe-uninitialized at declaration site > PR middle-end/98512 - #pragma GCC diagnostic ignored ineffective in conjunction with alias attribute Am I right in thinking that you add test coverage for both of these in patch 2 of the kit? > > gcc/ChangeLog: > > * diagnostic.c (update_inlining_context): New. > (update_effective_level_from_pragmas): Handle inlining context. > (diagnostic_report_diagnostic): Same. > * diagnostic.h (struct diagnostic_info): Add ctor. > (struct diagnostic_context): Add members. > * tree-diagnostic.c (get_inlining_locations): New. > (set_inlining_location): New. > (tree_diagnostics_defaults): Set new callback pointers. [..snip...] > @@ -1204,7 +1256,7 @@ diagnostic_report_diagnostic (diagnostic_context *context, > /* We do this to avoid giving the message for -pedantic-errors. */ > orig_diag_kind = diagnostic->kind; > } > - > + Stray whitespace change? Though it looks like a fix of a stray space, so not a big deal. > if (diagnostic->kind == DK_NOTE && context->inhibit_notes_p) > return false; > [..snip...] > diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h > index 1b9d6b1f64d..b95ee23dda0 100644 > --- a/gcc/diagnostic.h > +++ b/gcc/diagnostic.h > @@ -87,6 +87,10 @@ enum diagnostics_extra_output_kind > list in diagnostic.def. */ > struct diagnostic_info > { > + diagnostic_info () > + : message (), richloc (), metadata (), x_data (), kind (), option_index () > + { } > + Why does the patch add this ctor? > /* Text to be formatted. */ > text_info message; > > @@ -343,6 +347,32 @@ struct diagnostic_context > > /* Callback for final cleanup. */ > void (*final_cb) (diagnostic_context *context); > + > + /* The inlining context of the diagnostic (may have just one > + element if a diagnostic is not for an inlined expression). */ > + struct inlining_ctx > + { > + void reset () > + { > + ilocs.release (); > + loc = UNKNOWN_LOCATION; > + ao = NULL; > + allsyslocs = false; > + } > + > + /* Locations along the inlining stack. */ > + auto_vec ilocs; > + /* The locus of the diagnostic. */ > + location_t loc; > + /* The abstract origin of the location. */ > + void *ao; > + /* Set of every ILOCS element is in a system header. */ > + bool allsyslocs; > + } ictx; Why is the inlining ctx part of the diagnostic_context? That feels strange to me. This inlining information relates to a particular diagnostic, so it seems more appropriate to me that it should be part of the diagnostic_info (which might thus necessitate having a ctor for diagnostic_info). Doing that might avoid the need for "reset", if I'm right in assuming that getting the data is done once per diagnostic during diagnostic_report_diagnostic. Alternatively, could this be state that's created on the stack during diagnostic_report_diagnostic and passed around by pointer as another parameter? (putting it in diagnostic_info might be simplest though) Maybe rename it to "inlining_info"? How involved would it be to make it be a class with private fields? Can the field names have "m_" prefixes, please? > + /* Callbacks to get and set the inlining context. */ Probably should spell out in the comment here that doing so requires knowledge of trees, which is why it's a callback (to avoid diagnostic.c from having to know about trees). > + void (*get_locations_cb)(diagnostic_context *, diagnostic_info *); > + void (*set_location_cb)(const diagnostic_context *, diagnostic_info *); > }; > > static inline void > diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c > index 95b8ef30070..a8c5484849a 100644 > --- a/gcc/tree-diagnostic.c > +++ b/gcc/tree-diagnostic.c > @@ -305,6 +305,84 @@ default_tree_printer (pretty_printer *pp, text_info *text, const char *spec, > return true; > } > > +/* Get the inlining stack corresponding to the DIAGNOSTIC location. */ > + > +static void > +get_inlining_locations (diagnostic_context *context, > + diagnostic_info *diagnostic) > +{ > + context->ictx.reset (); > + > + location_t loc = diagnostic_location (diagnostic); > + tree block = LOCATION_BLOCK (loc); > + > + /* Count the number of locations in system headers. When all are, > + warnings are suppressed by -Wno-system-headers. Otherwise, they > + involve some user code, possibly inlined into a function in a system > + header, and are not treated as coming from system headers. */ > + unsigned nsyslocs = 0; > + > + while (block && TREE_CODE (block) == BLOCK > + && BLOCK_ABSTRACT_ORIGIN (block)) > + { > + tree ao = BLOCK_ABSTRACT_ORIGIN (block); > + if (TREE_CODE (ao) == FUNCTION_DECL) > + { > + if (!context->ictx.ao) > + context->ictx.ao = block; > + > + location_t loc = BLOCK_SOURCE_LOCATION (block); > + context->ictx.ilocs.safe_push (loc); > + if (in_system_header_at (loc)) > + ++nsyslocs; > + } > + else if (TREE_CODE (ao) != BLOCK) > + break; > + > + block = BLOCK_SUPERCONTEXT (block); > + } > + > + if (context->ictx.ilocs.length ()) > + { > + /* When there is an inlining context use the macro expansion > + location for the original location and bump up NSYSLOCS if > + it's in a system header since it's not counted above. */ > + context->ictx.loc = expansion_point_location_if_in_system_header (loc); > + if (context->ictx.loc != loc) > + ++nsyslocs; > + } > + else > + { > + /* When there's no inlining context use the original location > + and set NSYSLOCS accordingly. */ > + context->ictx.loc = loc; > + nsyslocs = in_system_header_at (loc) != 0; > + } > + > + context->ictx.ilocs.safe_push (context->ictx.loc); > + > + /* Set if all locations are in a system header. */ > + context->ictx.allsyslocs = nsyslocs == context->ictx.ilocs.length ();; Surplus trailing semicolon. Maybe store nsyslocs as private member data ("m_nsyslocs"), and have an all_system_locations_p accessor? (since if I'm reading things right that's the question that diagnostic_report_diagnostic is asking of the inlining_info that we need this information for) > +/* Set the inlining location for to the DIAGNOSTIC based on the saved > + inlining context. */ > + > +static void > +set_inlining_location (const diagnostic_context *context, > + diagnostic_info *diagnostic) > +{ > + if (!pp_ti_abstract_origin (&diagnostic->message) > + || !context->ictx.ao > + || context->ictx.loc == UNKNOWN_LOCATION) > + /* Do nothing when there's no inlining context. */ > + return; > + > + *pp_ti_abstract_origin (&diagnostic->message) = (tree)context->ictx.ao; > + diagnostic->message.set_location (0, context->ictx.loc, > + SHOW_RANGE_WITH_CARET); > +} > + If the inlining_info becomes a member of the diagnostic_info, does the need for this "set" callback go away? [...snip...] Hope this is constructive and makes sense; thanks again for the patch Dave