Message ID | 20220113220836.3781146-1-jason@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6B23A385781B for <patchwork@sourceware.org>; Thu, 13 Jan 2022 22:09:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6B23A385781B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1642111750; bh=6f9xyJ0v9HPau84qhJMVexzkWhzsOw96ZzLVZn9o0yI=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=J2jMVqPKhUJJqYYcq//tAHHDIXqRKLvxbbGLGji+FULbEoPGsM5nHu+gMRcaFIJG+ QJK1sJuqv92V1i9faiZazdieGnaX/Wn17sFy5YFmrqaMPbLLTj+CayxAwzsx/eKpsH bY/cqsf6EDBAk+OUx1dGAkEkGCfuf/Ps/fJ7jbFE= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 00374385840D for <gcc-patches@gcc.gnu.org>; Thu, 13 Jan 2022 22:08:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 00374385840D Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-641-Rh2cE3rSN6em5B_0BcZ81w-1; Thu, 13 Jan 2022 17:08:39 -0500 X-MC-Unique: Rh2cE3rSN6em5B_0BcZ81w-1 Received: by mail-qt1-f200.google.com with SMTP id o5-20020ac84285000000b002c7aa152905so5108778qtl.18 for <gcc-patches@gcc.gnu.org>; Thu, 13 Jan 2022 14:08:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=6f9xyJ0v9HPau84qhJMVexzkWhzsOw96ZzLVZn9o0yI=; b=JxXmvWjpvFw9H/iBatExJCWhg912TZnJIHeu9i8acKPJ0wOjBmopJJZXMSrX+OoUig NMeU6VnIGX18OFf78UzRKwjikXhCkmqoOY8cI6NI0kgXfh7+8NC0eaGDtS2MQsqk9x1U 1NsmkXrbSNLr6Q85nIxLB/NVMWYItN1wwKk9TDnwycq7mMv+bh/yfP2cDaW46mUO0wXL aMpz/wtSBT0JhnV/4s9+4GmeyFDYvyAWgR+QVhlVw03/+CFCUDmPXFOPaixZyNT4EwlP QZ/Ddltux3aNa8gJFNOdTOpRB6GwYANACDmwo+lheUGcX1IthBPIZuU9TqKPCJesGlq3 lc4w== X-Gm-Message-State: AOAM531+HQPK3wDOaGoLxD7Pw0LQZBaEjQAAnqFUBe5C9oVKDvt+ntE6 urqnlW6A5ysGbnLrUOSmsQbYhtbHVCQpvzaiSDf/SovMf/kiZypZJQOMA3ZfxAJFQPTgPBG6S9M kJsCPNQOZYaz8c4+oDM4BHg3r36JOB4cuh2PfBWO8HTJplIjBbej/CgDI9OA0CxSZFw== X-Received: by 2002:ac8:7fcc:: with SMTP id b12mr5618249qtk.164.1642111718526; Thu, 13 Jan 2022 14:08:38 -0800 (PST) X-Google-Smtp-Source: ABdhPJznuaUxDDpoE9S89V528pjd0F8SEFGuIb+ejvNq7qn4e2sHeD7ai3qGJjOTuwAmBrOmph7khw== X-Received: by 2002:ac8:7fcc:: with SMTP id b12mr5618216qtk.164.1642111718077; Thu, 13 Jan 2022 14:08:38 -0800 (PST) Received: from barrymore.redhat.com (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id m14sm2682083qkp.112.2022.01.13.14.08.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Jan 2022 14:08:37 -0800 (PST) To: gcc-patches@gcc.gnu.org Subject: [PATCH RFA] diagnostic: avoid repeating include path Date: Thu, 13 Jan 2022 17:08:36 -0500 Message-Id: <20220113220836.3781146-1-jason@redhat.com> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII" X-Spam-Status: No, score=-13.3 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_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jason Merrill <jason@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[RFA] diagnostic: avoid repeating include path
|
|
Commit Message
Jason Merrill
Jan. 13, 2022, 10:08 p.m. UTC
When a sequence of diagnostic messages bounces back and forth repeatedly between two includes, as with #include <map> std::map<const char*, const char*> m ("123", "456"); The output is quite a bit longer than necessary because we dump the include path each time it changes. I'd think we could print the include path once for each header file, and then expect that the user can look earlier in the output if they're wondering. Tested x86_64-pc-linux-gnu, OK for trunk? gcc/ChangeLog: * diagnostic.c (includes_seen): New. (diagnostic_report_current_module): Use it. --- gcc/diagnostic.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) base-commit: b8ffa71e4271ae562c2d315b9b24c4979bbf8227 prerequisite-patch-id: e45065ef320968d982923dd44da7bed07e3326ef
Comments
On Thu, 2022-01-13 at 17:08 -0500, Jason Merrill wrote: > When a sequence of diagnostic messages bounces back and forth > repeatedly > between two includes, as with > > #include <map> > std::map<const char*, const char*> m ("123", "456"); > > The output is quite a bit longer than necessary because we dump the > include > path each time it changes. I'd think we could print the include path > once > for each header file, and then expect that the user can look earlier > in the > output if they're wondering. > > Tested x86_64-pc-linux-gnu, OK for trunk? > > gcc/ChangeLog: > > * diagnostic.c (includes_seen): New. > (diagnostic_report_current_module): Use it. > --- > gcc/diagnostic.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c > index 58139427d01..e56441a2dbf 100644 > --- a/gcc/diagnostic.c > +++ b/gcc/diagnostic.c > @@ -700,6 +700,16 @@ set_last_module (diagnostic_context *context, > const line_map_ordinary *map) > context->last_module = map; > } > > +/* Only dump the "In file included from..." stack once for each > file. */ > + > +static bool > +includes_seen (const line_map_ordinary *map) > +{ > + using hset = hash_set<const line_map_ordinary *>; > + static hset *set = new hset; > + return set->add (map); > +} Overall, I like the idea, but... - the patch works at the level of line_map_ordinary instances, rather than header files. There are various ways in which a single header file can have multiple line maps e.g. due to very long lines, or including another file, etc. I think it makes sense to do it at the per-file level, assuming we aren't in a horrible situation where a header is being included repeatedly, with different effects. So maybe this ought to look at what include directive led to this map, i.e. looking at the ord_map->included_from field, and having a hash_set<location_t> ? - there's no test coverage, but it's probably not feasible to write DejaGnu tests for this, given the way prune.exp's prune_gcc_output strips these strings. Maybe a dg directive to selectively disable the pertinent pruning operations in prune_gcc_output??? Gah... - global state is a pet peeve of mine; can the above state be put inside the diagnostic_context instead? (perhaps via a pointer to a wrapper class to avoid requiring all users of diagnostic.h to include hash-set.h?). Hope this is constructive Dave > + > void > diagnostic_report_current_module (diagnostic_context *context, > location_t where) > { > @@ -721,7 +731,7 @@ diagnostic_report_current_module > (diagnostic_context *context, location_t where) > if (map && last_module_changed_p (context, map)) > { > set_last_module (context, map); > - if (! MAIN_FILE_P (map)) > + if (! MAIN_FILE_P (map) && !includes_seen (map)) > { > bool first = true, need_inc = true, was_module = > MAP_MODULE_P (map); > expanded_location s = {}; > > base-commit: b8ffa71e4271ae562c2d315b9b24c4979bbf8227 > prerequisite-patch-id: e45065ef320968d982923dd44da7bed07e3326ef
On 1/13/22 17:30, David Malcolm wrote: > On Thu, 2022-01-13 at 17:08 -0500, Jason Merrill wrote: >> When a sequence of diagnostic messages bounces back and forth >> repeatedly >> between two includes, as with >> >> #include <map> >> std::map<const char*, const char*> m ("123", "456"); >> >> The output is quite a bit longer than necessary because we dump the >> include >> path each time it changes. I'd think we could print the include path >> once >> for each header file, and then expect that the user can look earlier >> in the >> output if they're wondering. >> >> Tested x86_64-pc-linux-gnu, OK for trunk? >> >> gcc/ChangeLog: >> >> * diagnostic.c (includes_seen): New. >> (diagnostic_report_current_module): Use it. >> --- >> gcc/diagnostic.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c >> index 58139427d01..e56441a2dbf 100644 >> --- a/gcc/diagnostic.c >> +++ b/gcc/diagnostic.c >> @@ -700,6 +700,16 @@ set_last_module (diagnostic_context *context, >> const line_map_ordinary *map) >> context->last_module = map; >> } >> >> +/* Only dump the "In file included from..." stack once for each >> file. */ >> + >> +static bool >> +includes_seen (const line_map_ordinary *map) >> +{ >> + using hset = hash_set<const line_map_ordinary *>; >> + static hset *set = new hset; >> + return set->add (map); >> +} > > Overall, I like the idea, but... > > - the patch works at the level of line_map_ordinary instances, rather > than header files. There are various ways in which a single header > file can have multiple line maps e.g. due to very long lines, or > including another file, etc. I think it makes sense to do it at the > per-file level, assuming we aren't in a horrible situation where a > header is being included repeatedly, with different effects. So maybe > this ought to look at what include directive led to this map, i.e. > looking at the ord_map->included_from field, and having a > hash_set<location_t> ? Done. This version doesn't affect printing of the module import path yet, pending more consideration of whether we always want to identify the module it comes from or just the file/line is enough. > - there's no test coverage, but it's probably not feasible to write > DejaGnu tests for this, given the way prune.exp's prune_gcc_output > strips these strings. Maybe a dg directive to selectively disable the > pertinent pruning operations in prune_gcc_output??? Gah... > > - global state is a pet peeve of mine; can the above state be put > inside the diagnostic_context instead? (perhaps via a pointer to a > wrapper class to avoid requiring all users of diagnostic.h to include > hash-set.h?). It seems that using hash_set directly doesn't break any users.
On Fri, 2022-01-14 at 23:01 -0500, Jason Merrill wrote: > On 1/13/22 17:30, David Malcolm wrote: > > On Thu, 2022-01-13 at 17:08 -0500, Jason Merrill wrote: > > > When a sequence of diagnostic messages bounces back and forth > > > repeatedly > > > between two includes, as with > > > > > > #include <map> > > > std::map<const char*, const char*> m ("123", "456"); > > > > > > The output is quite a bit longer than necessary because we dump > > > the > > > include > > > path each time it changes. I'd think we could print the include > > > path > > > once > > > for each header file, and then expect that the user can look > > > earlier > > > in the > > > output if they're wondering. > > > > > > Tested x86_64-pc-linux-gnu, OK for trunk? > > > > > > gcc/ChangeLog: > > > > > > * diagnostic.c (includes_seen): New. > > > (diagnostic_report_current_module): Use it. > > > --- > > > gcc/diagnostic.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c > > > index 58139427d01..e56441a2dbf 100644 > > > --- a/gcc/diagnostic.c > > > +++ b/gcc/diagnostic.c > > > @@ -700,6 +700,16 @@ set_last_module (diagnostic_context > > > *context, > > > const line_map_ordinary *map) > > > context->last_module = map; > > > } > > > > > > +/* Only dump the "In file included from..." stack once for each > > > file. */ > > > + > > > +static bool > > > +includes_seen (const line_map_ordinary *map) > > > +{ > > > + using hset = hash_set<const line_map_ordinary *>; > > > + static hset *set = new hset; > > > + return set->add (map); > > > +} > > > > Overall, I like the idea, but... > > > > - the patch works at the level of line_map_ordinary instances, > > rather > > than header files. There are various ways in which a single header > > file can have multiple line maps e.g. due to very long lines, or > > including another file, etc. I think it makes sense to do it at > > the > > per-file level, assuming we aren't in a horrible situation where a > > header is being included repeatedly, with different effects. So > > maybe > > this ought to look at what include directive led to this map, i.e. > > looking at the ord_map->included_from field, and having a > > hash_set<location_t> ? > > Done. This version doesn't affect printing of the module import path > yet, pending more consideration of whether we always want to identify > the module it comes from or just the file/line is enough. Seems like a good choice given that everyone's going to be much less familiar with modules than with include files, for some time. > > > - there's no test coverage, but it's probably not feasible to write > > DejaGnu tests for this, given the way prune.exp's prune_gcc_output > > strips these strings. Maybe a dg directive to selectively disable > > the > > pertinent pruning operations in prune_gcc_output??? Gah... > > > > - global state is a pet peeve of mine; can the above state be put > > inside the diagnostic_context instead? (perhaps via a pointer to > > a > > wrapper class to avoid requiring all users of diagnostic.h to > > include > > hash-set.h?). > > It seems that using hash_set directly doesn't break any users. Thanks. The updated patch looks good to me. Dave
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c index 58139427d01..e56441a2dbf 100644 --- a/gcc/diagnostic.c +++ b/gcc/diagnostic.c @@ -700,6 +700,16 @@ set_last_module (diagnostic_context *context, const line_map_ordinary *map) context->last_module = map; } +/* Only dump the "In file included from..." stack once for each file. */ + +static bool +includes_seen (const line_map_ordinary *map) +{ + using hset = hash_set<const line_map_ordinary *>; + static hset *set = new hset; + return set->add (map); +} + void diagnostic_report_current_module (diagnostic_context *context, location_t where) { @@ -721,7 +731,7 @@ diagnostic_report_current_module (diagnostic_context *context, location_t where) if (map && last_module_changed_p (context, map)) { set_last_module (context, map); - if (! MAIN_FILE_P (map)) + if (! MAIN_FILE_P (map) && !includes_seen (map)) { bool first = true, need_inc = true, was_module = MAP_MODULE_P (map); expanded_location s = {};