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 AB4533857C65 for ; Wed, 30 Jun 2021 22:55:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AB4533857C65 Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-463-9bgk0wqgPHmeXll_AR0Wqw-1; Wed, 30 Jun 2021 18:55:38 -0400 X-MC-Unique: 9bgk0wqgPHmeXll_AR0Wqw-1 Received: by mail-qk1-f197.google.com with SMTP id a2-20020a05620a0662b02903ad3598ec02so2782544qkh.17 for ; Wed, 30 Jun 2021 15:55:38 -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=0h2W0bTqcL9ZfBcFRr6qy1KE7MTv3pKVRTEDwV2EHjg=; b=awC5cCWPwfrvvRdcuhcRV4yowtCl5m1XkoVPwdPTlxngyXl+bC1MjRmfmBQrOHFc8+ e+zTck5ZEi0glNVQes1FIRAkuEF4cqQaDbjSCU0N7Sd2aL90/Dqi1vJY1jugwB57vCOj THpEhWtEL8BAn2zHNfFa45dZFh9QEcDqTfJsOUZng4jo78aWUuKLQBl1+jp/DCMa/zT4 yrt4uh1PbQFxa5a2QIFpKa2VwvNKV9UHAwGsDIh09ZOm4go6t7yVIy+9XE+SrLamBioh 4HJeXZNUaywvRyJ33lw7FLG18GfANoMsph4b0NlIW4Eos8UL2rFibcjqFtGvUo+F1R6A fqrA== X-Gm-Message-State: AOAM533ZvpXYllU3H5tQWla01ekaL0q7wRfw2VsK6usOiU3OaOkCsoVb Wmz7Bhw4/wOQX4MCIbkQNLihNx3NghPKn60Pyo2TwBl1wDiZip8LxiAcERT/JA83Gs5+NZRglc1 qqpb60S6Y5OteE52yYw== X-Received: by 2002:a37:e108:: with SMTP id c8mr33542268qkm.67.1625093738306; Wed, 30 Jun 2021 15:55:38 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxHPFGuZPEBS2f1ZKvdARM9qjT/w4rbUh4JQMOS4k4KVHYFGUvGyc/T/f/44YoGjueHhSDiJA== X-Received: by 2002:a37:e108:: with SMTP id c8mr33542253qkm.67.1625093738100; Wed, 30 Jun 2021 15:55:38 -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 y3sm14087894qkf.2.2021.06.30.15.55.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 30 Jun 2021 15:55:37 -0700 (PDT) Message-ID: <0824ba5e8940c00628f74089701289bbddd61787.camel@redhat.com> Subject: Re: [PATCH 1/4] introduce diagnostic infrastructure changes (PR 98512) From: David Malcolm To: Martin Sebor , gcc-patches Date: Wed, 30 Jun 2021 18:55:36 -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> <314ae442b5972ed23468e20698e678a26ff1bc1e.camel@redhat.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: 8bit X-Spam-Status: No, score=-11.8 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_NONE, 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: Wed, 30 Jun 2021 22:55:42 -0000 On Tue, 2021-06-15 at 17:00 -0600, Martin Sebor wrote: > On 6/11/21 11:04 AM, David Malcolm wrote: > > 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? > > Yes, the tests depend on the changes in patch 2 (some existing tests > fail with just patch 1 applied because the initial location passed > to warning_t() is different than with it). > > [...] > > > > > Yep, thanks.  Please see the attached revision. > > Martin Various nits inline below: > diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c > index d58586f2526..3a22d4d26a6 100644 > --- a/gcc/diagnostic.c > +++ b/gcc/diagnostic.c > @@ -991,51 +991,93 @@ print_parseable_fixits (pretty_printer *pp, rich_location *richloc, > pp_set_prefix (pp, saved_prefix); > } > > -/* Update the diag_class of DIAGNOSTIC based on its location > - relative to any > +/* Update the inlininig context in CONTEXT for a DIAGNOSTIC. */ ^^^^^^^^^^^^^^^^^ It's inlining_info now, so please update this comment. > + > +static void > +update_inlining_context (diagnostic_context *context, > + diagnostic_info *diagnostic) ...and please rename to "get_any_inlining_info". > +{ > + auto &ilocs = diagnostic->m_iinfo.m_ilocs; > + > + if (context->set_locations_cb) > + { > + /* Retrieve the locations into which the expression about to be > + diagnosed has been inlined, including those of all the callers > + all the way down the inlining stack. */ > + context->set_locations_cb (context, diagnostic); > + location_t loc = diagnostic->m_iinfo.m_ilocs.last (); > + if (diagnostic->m_iinfo.m_ao && loc != UNKNOWN_LOCATION) > + diagnostic->message.set_location (0, loc, SHOW_RANGE_WITH_CARET); What is the purpose of the above two lines of code? (I believe it's to replace the %G/%K stuff, right?) Please can you add a suitable comment. > + } > + else > + { > + /* When there's no callback use just the one location provided > + by the caller of the diagnostic function. */ > + location_t loc = diagnostic_location (diagnostic); > + ilocs.safe_push (loc); > + diagnostic->m_iinfo.m_allsyslocs = in_system_header_at (loc); > + } > +} > + > +/* Update the kind of DIAGNOSTIC based on its location(s), including > + any of those in its inlining context, relative to any ^^^^^^^ "stack" rather than "context" here; I think we're overusing the word "context". > #pragma GCC diagnostic > directives recorded within CONTEXT. > > - Return the new diag_class of DIAGNOSTIC if it was updated, or > - DK_UNSPECIFIED otherwise. */ > + Return the new kind of DIAGNOSTIC if it was updated, or DK_UNSPECIFIED > + otherwise. */ > > static diagnostic_t > update_effective_level_from_pragmas (diagnostic_context *context, > diagnostic_info *diagnostic) > { > - diagnostic_t diag_class = DK_UNSPECIFIED; > - > - if (context->n_classification_history > 0) > + if (diagnostic->m_iinfo.m_allsyslocs && !context->dc_warn_system_headers) > { > - location_t location = diagnostic_location (diagnostic); > + /* Ignore the diagnostic if all the inlined locations are > + in system headers and -Wno-system-headers is in effect. */ > + diagnostic->kind = DK_IGNORED; > + return DK_IGNORED; > + } > > + if (context->n_classification_history <= 0) > + return DK_UNSPECIFIED; > + > + /* Iterate over the locations, checking the diagnostic disposition > + for the diagnostic at each. If it's explicitly set as opposed > + to unspecified, update the disposition for this instance of > + the diagnostic and return it. */ > + for (location_t loc: diagnostic->m_iinfo.m_ilocs) > + { > /* FIXME: Stupid search. Optimize later. */ > for (int i = context->n_classification_history - 1; i >= 0; i --) > { > - if (linemap_location_before_p > - (line_table, > - context->classification_history[i].location, > - location)) > + const diagnostic_classification_change_t &hist > + = context->classification_history[i]; > + > + location_t pragloc = hist.location; > + if (!linemap_location_before_p (line_table, pragloc, loc)) > + continue; > + > + if (hist.kind == (int) DK_POP) > { > - if (context->classification_history[i].kind == (int) DK_POP) > - { > - i = context->classification_history[i].option; > - continue; > - } > - int option = context->classification_history[i].option; > - /* The option 0 is for all the diagnostics. */ > - if (option == 0 || option == diagnostic->option_index) > - { > - diag_class = context->classification_history[i].kind; > - if (diag_class != DK_UNSPECIFIED) > - diagnostic->kind = diag_class; > - break; > - } > + /* Move on to the next region. */ > + i = hist.option; > + continue; > + } > + > + int option = hist.option; > + /* The option 0 is for all the diagnostics. */ > + if (option == 0 || option == diagnostic->option_index) > + { > + diagnostic_t kind = hist.kind; > + if (kind != DK_UNSPECIFIED) > + diagnostic->kind = kind; > + return kind; > } > } > } > > - return diag_class; > + return DK_UNSPECIFIED; > } > > /* Generate a URL string describing CWE. The caller is responsible for > @@ -1129,6 +1171,9 @@ static bool > diagnostic_enabled (diagnostic_context *context, > diagnostic_info *diagnostic) > { > + /* Update the inlining context for this diagnostic. */ > + update_inlining_context (context, diagnostic); Please rename as described above. [...] Dave