[RFA] diagnostic: avoid repeating include path

Message ID 20220113220836.3781146-1-jason@redhat.com
State New
Headers show
Series
  • [RFA] diagnostic: avoid repeating include path
Related show

Commit Message

Jonathan Wakely via Gcc-patches Jan. 13, 2022, 10:08 p.m.
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
-- 
2.27.0

Comments

Jonathan Wakely via Gcc-patches Jan. 13, 2022, 10:30 p.m. | #1
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
Jonathan Wakely via Gcc-patches Jan. 15, 2022, 4:01 a.m. | #2
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.
From a6137bc1154bdf237449382f98cd3945a701103f Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Fri, 17 Dec 2021 05:45:02 -0500
Subject: [PATCH] diagnostic: avoid repeating include path
To: gcc-patches@gcc.gnu.org

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.

gcc/ChangeLog:

	* diagnostic.h (struct diagnostic_context): Add includes_seen.
	* diagnostic.c (diagnostic_initialize): Initialize it.
	(diagnostic_finish): Clean it up.
	(includes_seen): New function.
	(diagnostic_report_current_module): Use it.

gcc/testsuite/ChangeLog:

	* c-c++-common/cpp/line-2.c: Only expect includes once.
	* c-c++-common/cpp/line-3.c: Likewise.
---
 gcc/diagnostic.h                        |  4 +++
 gcc/diagnostic.c                        | 36 +++++++++++++++++++++++--
 gcc/testsuite/c-c++-common/cpp/line-2.c |  2 +-
 gcc/testsuite/c-c++-common/cpp/line-3.c |  2 +-
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 6739028a931..ccaa33b5817 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -387,6 +387,10 @@ struct diagnostic_context
      the BLOCK_SUPERCONTEXT() chain hanging off the LOCATION_BLOCK()
      of a diagnostic's location.  */
   void (*set_locations_cb)(diagnostic_context *, diagnostic_info *);
+
+  /* Include files that diagnostic_report_current_module has already listed the
+     include path for.  */
+  hash_set<location_t, false, location_hash> *includes_seen;
 };
 
 static inline void
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 58139427d01..5c02ff05882 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -237,6 +237,7 @@ diagnostic_initialize (diagnostic_context *context, int n_opts)
   context->begin_group_cb = NULL;
   context->end_group_cb = NULL;
   context->final_cb = default_diagnostic_final_cb;
+  context->includes_seen = NULL;
 }
 
 /* Maybe initialize the color support. We require clients to do this
@@ -329,6 +330,12 @@ diagnostic_finish (diagnostic_context *context)
       delete context->edit_context_ptr;
       context->edit_context_ptr = NULL;
     }
+
+  if (context->includes_seen)
+    {
+      delete context->includes_seen;
+      context->includes_seen = nullptr;
+    }
 }
 
 /* Initialize DIAGNOSTIC, where the message MSG has already been
@@ -700,6 +707,31 @@ 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 (diagnostic_context *context, const line_map_ordinary *map)
+{
+  /* No include path for main.  */
+  if (MAIN_FILE_P (map))
+    return true;
+
+  /* Always identify C++ modules, at least for now.  */
+  auto probe = map;
+  if (linemap_check_ordinary (map)->reason == LC_RENAME)
+    /* The module source file shows up as LC_RENAME inside LC_MODULE.  */
+    probe = linemap_included_from_linemap (line_table, map);
+  if (MAP_MODULE_P (probe))
+    return false;
+
+  if (!context->includes_seen)
+    context->includes_seen = new hash_set<location_t, false, location_hash>;
+
+  /* Hash the location of the #include directive to better handle files
+     that are included multiple times with different macros defined.  */
+  return context->includes_seen->add (linemap_included_from (map));
+}
+
 void
 diagnostic_report_current_module (diagnostic_context *context, location_t where)
 {
@@ -721,7 +753,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 (!includes_seen (context, map))
 	{
 	  bool first = true, need_inc = true, was_module = MAP_MODULE_P (map);
 	  expanded_location s = {};
@@ -760,7 +792,7 @@ diagnostic_report_current_module (diagnostic_context *context, location_t where)
 			   "locus", s.file, line_col);
 	      first = false, need_inc = was_module, was_module = is_module;
 	    }
-	  while (! MAIN_FILE_P (map));
+	  while (!includes_seen (context, map));
 	  pp_verbatim (context->printer, ":");
 	  pp_newline (context->printer);
 	}
diff --git a/gcc/testsuite/c-c++-common/cpp/line-2.c b/gcc/testsuite/c-c++-common/cpp/line-2.c
index 97cf398f64c..364ad0e3931 100644
--- a/gcc/testsuite/c-c++-common/cpp/line-2.c
+++ b/gcc/testsuite/c-c++-common/cpp/line-2.c
@@ -8,4 +8,4 @@ int line4;
 
 // { dg-regexp {In file included from <command-line>:\n[^\n]*/line-2.h:4:2: error: #error wrong\n} }
 
-// { dg-regexp {[^\n]*/line-2.c:3:11: error: macro "bill" passed 1 arguments, but takes just 0\nIn file included from <command-line>:\n[^\n]*/line-2.h:3: note: macro "bill" defined here\n} }
+// { dg-regexp {[^\n]*/line-2.c:3:11: error: macro "bill" passed 1 arguments, but takes just 0\n[^\n]*/line-2.h:3: note: macro "bill" defined here\n} }
diff --git a/gcc/testsuite/c-c++-common/cpp/line-3.c b/gcc/testsuite/c-c++-common/cpp/line-3.c
index 2ffc44907a2..b254ae40041 100644
--- a/gcc/testsuite/c-c++-common/cpp/line-3.c
+++ b/gcc/testsuite/c-c++-common/cpp/line-3.c
@@ -15,6 +15,6 @@ int line4;
 
 // { dg-regexp {In file included from <command-line>:\n[^\n]*/line-2.h:4:2: error: #error wrong\n} }
 
-// { dg-regexp {[^\n]*/line-3.c:3:11: error: macro "bill" passed 1 arguments, but takes just 0\nIn file included from <command-line>:\n[^\n]*/line-2.h:3: note: macro "bill" defined here\n} }
+// { dg-regexp {[^\n]*/line-3.c:3:11: error: macro "bill" passed 1 arguments, but takes just 0\n[^\n]*/line-2.h:3: note: macro "bill" defined here\n} }
 
 // { dg-options "-fpreprocessed -fdirectives-only" }
Jonathan Wakely via Gcc-patches Jan. 17, 2022, 1:27 a.m. | #3
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

Patch

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 = {};