[PATCH] Fix method naming bug in new DWARF indexer

[PATCH] Fix method naming bug in new DWARF indexer

[Tom Tromey]

Pedro pointed out that gdb-add-index is much slower with the new DWARF
indexer.  He also noticed that, in some cases, the generated
.gdb_index would have the wrong fully-qualified name for a method.

I tracked this down to a bug in the indexer.  If a type could have
methods but was marked as a declaration, the indexer was ignoring it.
However, this meant that the internal map to find the qualified name
was not updated for this container.
---
 gdb/dwarf2/abbrev.c                      | 11 ++++
 gdb/dwarf2/read.c                        |  7 ++-
 gdb/testsuite/gdb.dwarf2/struct-decl.exp | 71 ++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/struct-decl.exp

diff --git a/gdb/dwarf2/abbrev.c b/gdb/dwarf2/abbrev.c
index 2db5ea15aca..4ca27eaa7e0 100644
--- a/gdb/dwarf2/abbrev.c
+++ b/gdb/dwarf2/abbrev.c
@@ -280,6 +280,17 @@ abbrev_table::read (struct dwarf2_section_info *section,
 	    = (cur_abbrev->tag == DW_TAG_namespace
 	       || cur_abbrev->tag == DW_TAG_enumeration_type);
 	}
+      else if ((cur_abbrev->tag == DW_TAG_structure_type
+		|| cur_abbrev->tag == DW_TAG_class_type
+		|| cur_abbrev->tag == DW_TAG_union_type)
+	       && cur_abbrev->has_children)
+	{
+	  /* We have to record this as interesting, regardless of how
+	     DW_AT_declaration is set, so that any subsequent
+	     DW_AT_specification pointing at a child of this will get
+	     the correct scope.  */
+	  cur_abbrev->interesting = true;
+	}
       else if (has_hardcoded_declaration
 	       && (cur_abbrev->tag != DW_TAG_variable || !has_external))
 	cur_abbrev->interesting = false;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index bb72e0e4791..6842408ad38 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18097,8 +18097,11 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu,
      that is ok.  Similarly, we allow an external variable without a
      location; those are resolved via minimal symbols.  */
   if (is_declaration && !for_specification
-      && (abbrev->tag != DW_TAG_variable
-	  || (*flags & IS_STATIC) != 0))
+      && !(abbrev->tag == DW_TAG_variable && (*flags & IS_STATIC) == 0)
+      && !((abbrev->tag == DW_TAG_class_type
+	    || abbrev->tag == DW_TAG_structure_type
+	    || abbrev->tag == DW_TAG_union_type)
+	   && abbrev->has_children))
     {
       *linkage_name = nullptr;
       *name = nullptr;
diff --git a/gdb/testsuite/gdb.dwarf2/struct-decl.exp b/gdb/testsuite/gdb.dwarf2/struct-decl.exp
new file mode 100644
index 00000000000..7d7ba52319b
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/struct-decl.exp
@@ -0,0 +1,71 @@
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile main.c -debug.S
+
+# Set up the DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcfile
+
+    cu {} {
+	DW_TAG_compile_unit {
+	    {DW_AT_language @DW_LANG_C_plus_plus}
+	    {DW_AT_name     $srcfile}
+	    {DW_AT_comp_dir /tmp}
+
+	} {
+	    declare_labels origin
+
+	    # The new indexer was ignoring types that were marked as
+	    # declarations, even if they could contain methods --
+	    # which meant that if a method referred back to them via a
+	    # specification, it would get the wrong name.
+	    DW_TAG_structure_type {
+		{DW_AT_byte_size 8 DW_FORM_sdata}
+		{DW_AT_encoding  @DW_ATE_signed}
+		{DW_AT_name the_type}
+		{DW_AT_declaration 1 DW_FORM_flag_present}
+	    } {
+		origin: DW_TAG_subprogram {
+		    {DW_AT_name "method"}
+		    {DW_AT_declaration 1 DW_FORM_flag_present}
+		}
+	    }
+
+	    # The low and high PC are phony: we just need an address
+	    # range that is valid in the program, so we use the main
+	    # function's range.
+	    DW_TAG_subprogram {
+		{DW_AT_specification :$origin}
+		{MACRO_AT_range main}
+	    }
+	}
+    }
+}
+
+if {[prepare_for_testing "failed to prepare" ${testfile} \
+	 [list $srcfile $asm_file] {nodebug}]} {
+    return -1
+}
+
+gdb_breakpoint "the_type::method" message
-- 
2.34.1

Re: [PATCH] Fix method naming bug in new DWARF indexer

[Pedro Alves]

On 2022-04-21 17:38, Tom Tromey via Gdb-patches wrote:
> Pedro pointed out that gdb-add-index is much slower with the new DWARF
> indexer.  He also noticed that, in some cases, the generated
> .gdb_index would have the wrong fully-qualified name for a method.
> 
> I tracked this down to a bug in the indexer.  If a type could have
> methods but was marked as a declaration, the indexer was ignoring it.
> However, this meant that the internal map to find the qualified name
> was not updated for this container.

Thanks, I gave this a try, dumped an index of a gdb binary generated by the new gdb.  The gdb I
dumped is the same gdb binary I used last week, and I was able to diff the new index symbol names
against the symbol names in an index generated by a gdb that predates the new indexer.  I confirm that
the bad symbol names are all fixed.  The main difference we see compared to old index are that the
new index scopes enum class enumerators correctly while the old index didn't, and also there were some
nested types and symbols that were missing in the old index.  Those are all good changes.

The only odd name that jumped at me in the new index is that there's an entry for:

  decltype(nullptr)

But looking at the DWARF dump, it is really there, like:

0x00000575:   DW_TAG_unspecified_type
                DW_AT_name      ("decltype(nullptr)")

0x000025be:     DW_TAG_typedef
                  DW_AT_name    ("nullptr_t")
                  DW_AT_decl_file       ("/usr/include/x86_64-linux-gnu/c++/9/bits/c++config.h")
                  DW_AT_decl_line       (262)
                  DW_AT_decl_column     (0x1d)
                  DW_AT_type    (0x00000575 "decltype(nullptr)")

So it looks right to have it.

The "much slower" aspect remains, this patch does not change it.  The size of the generated
index roughly the same as from before this patch, and it is still much larger than from before
the new DWARF indexer landed.  I think that will require a separate fix.  I sent you more info
about it off list.

> diff --git a/gdb/testsuite/gdb.dwarf2/struct-decl.exp b/gdb/testsuite/gdb.dwarf2/struct-decl.exp
> new file mode 100644
> index 00000000000..7d7ba52319b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/struct-decl.exp
> @@ -0,0 +1,71 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +

Can you add a short intro comment mentioning what this is testing?

> +load_lib dwarf.exp
> +

Re: [PATCH] Fix method naming bug in new DWARF indexer

[Tom Tromey]

Pedro> Can you add a short intro comment mentioning what this is testing?

I did this, and I'm checking it in now.

Tom

Re: [PATCH] Fix method naming bug in new DWARF indexer

[Tom Tromey]

Pedro> The "much slower" aspect remains, this patch does not change it.
Pedro> The size of the generated index roughly the same as from before
Pedro> this patch, and it is still much larger than from before the new
Pedro> DWARF indexer landed.  I think that will require a separate fix.
Pedro> I sent you more info
Pedro> about it off list.

I have a patch to fix the size problem.  It works by only emitting a
given type or variable once.  This is what the  gdb  used to do via the
psymbol bcache.  It could sometimes mean that "info types" would miss
a duplicate -- but the old code did this as well.

I'll send this soon.

However, this patch doesn't fix the performance problem.  I am not sure
it is really fixable, given that the code has to compute the full name
of every entry in the index.

If we check in the background-writing code, this won't be noticeable for
the index cache case.  However, it will still be noticeable for
"gdb-add-index".

Tom

Re: [PATCH] Fix method naming bug in new DWARF indexer

[Pedro Alves]

On 2022-04-25 19:27, Tom Tromey wrote:
> Pedro> The "much slower" aspect remains, this patch does not change it.
> Pedro> The size of the generated index roughly the same as from before
> Pedro> this patch, and it is still much larger than from before the new
> Pedro> DWARF indexer landed.  I think that will require a separate fix.
> Pedro> I sent you more info
> Pedro> about it off list.
> 
> I have a patch to fix the size problem.  It works by only emitting a
> given type or variable once.  This is what the  gdb  used to do via the
> psymbol bcache.  It could sometimes mean that "info types" would miss
> a duplicate -- but the old code did this as well.
> 
> I'll send this soon.
> 

Thanks, I saw it on the list now, and I gave it a try.

> However, this patch doesn't fix the performance problem.  I am not sure
> it is really fixable, given that the code has to compute the full name
> of every entry in the index.
> 

Hmm, it actually fixes most of the performance for me.  With that new patch (and I guess
the linkage names patch made a difference too), I see roughly the same startup time in
new gdb vs old gdb when using the same index, and either an index generated by old gdb, or
by new gdb.  Generating the index is a bit slower than before, 7.1s vs 6.6s, a rough
8% slowdown, but then again, the generated index is still ~13% bigger than the one generated
by old gdb (17MB vs 15MB), which I assume explains the remaining index generation slowdown.

The patch you point at addresses variables/enums, and I wonder whether the remainder of size
increase (and thus the remainder of the slowdown) is explained by functions.

In the old indexer, with an index of a gdb from before the indexer rewrite, we had:

 [369902] intrusive_list<inferior, intrusive_base_node<inferior> >::begin:
          9 [global, function]
          255 [global, function]

while in the new indexer, for the same function, we have:

 [369902] intrusive_list<inferior, intrusive_base_node<inferior> >::begin:
         9 [global, function]
         79 [global, function]
         165 [global, function]
         167 [global, function]
         252 [global, function]
         253 [global, function]
         254 [global, function]
         255 [global, function]
         263 [global, function]
         266 [global, function]
         310 [global, function]
         311 [global, function]
         373 [global, function]
         376 [global, function]
         396 [global, function]
         436 [global, function]
         503 [global, function]
         506 [global, function]
         507 [global, function]
         513 [global, function]

Same with "make_scoped_restore<int, int>", for instance -- one entry in old index, and 21 entries
in new index.

Now, if I load that old gdb under new gdb, and set a breakpoint at one of those functions, I get
as many locations as entries in the old indexes.  For example:

$ g="./gdb -data-directory=data-directory"

$ $g -nx /home/pedro/slowdown/gdb.before-indexer
[...]
(gdb) b intrusive_list<inferior, intrusive_base_node<inferior> >::begin
Breakpoint 2 at 0x555555c9aefe: intrusive_list<inferior, intrusive_base_node<inferior> >::begin. (2 locations)
                                                                                                  ^^^^^^^^^^^

Note the 2 locations found.  "2" matches the number of entries in the old index for that function.

 (gdb) info breakpoints 
 Num     Type           Disp Enb Address            What
 2       breakpoint     keep y   <MULTIPLE>         
 2.1                         y   0x0000555555c9aefe in intrusive_list<inferior, intrusive_base_node<inferior> >::begin() const 
                                                    at ../../src/gdb/../gdbsupport/intrusive_list.h:521
 2.2                         y   0x00005555560ab946 in intrusive_list<inferior, intrusive_base_node<inferior> >::begin() 
                                                    at ../../src/gdb/../gdbsupport/intrusive_list.h:516


Same for "make_scoped_restore<int, int>" -- setting a breakpoint there only finds one location,
just like there's only one entry for that function in the old index:

 (gdb) b make_scoped_restore<int, int>
 Breakpoint 1 at 0x83facc: file ../../src/gdb/../gdbsupport/scoped_restore.h, line 115.
 (gdb) info breakpoints 
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   0x0000555555d93acc in make_scoped_restore<int, int>(int*, int) at ../../src/gdb/../gdbsupport/scoped_restore.h:115


If we used the same htab dedup logic as in your variables/enums patch, for functions, then we'd
end up with only a single entry in the index for the intrusive_list example, which seems incorrect.
What was the logic that the old writer used to come up with the seemingly right number of function entries?


> If we check in the background-writing code, this won't be noticeable for
> the index cache case.  However, it will still be noticeable for
> "gdb-add-index".


For the time being, I'll continue using indexes, because as soon as you need to 
run to a breakpoint from the command line, which I do all the time, then having an index still
beats the new scanner.  E.g., for me:

 g="./gdb -data-directory=data-directory"
 $ time $g -q --batch -iex "set index-cache enabled off" $g -ex "start"
 Setting up the environment for debugging gdb.
 Breakpoint 1 at 0xa364b4: file /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc, line 51.
 Breakpoint 2 at 0x260fa3: file /home/pedro/gdb/binutils-gdb/src/gdb/cli/cli-cmds.c, line 217.
 Temporary breakpoint 3 at 0xeb06c: file /home/pedro/gdb/binutils-gdb/src/gdb/gdb.c, line 25.
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

 Temporary breakpoint 3, main (argc=1, argv=0x7fffffffdc48) at /home/pedro/gdb/binutils-gdb/src/gdb/gdb.c:25
 25      {

 real    0m3.920s
 user    0m6.924s
 sys     0m0.245s


vs (w/ hot index cache):

 $ time $g -q --batch -iex "set index-cache enabled on" $g -ex "start"
 Setting up the environment for debugging gdb.
 Breakpoint 1 at 0xa364b4: file /home/pedro/gdb/binutils-gdb/src/gdbsupport/errors.cc, line 51.
 Breakpoint 2 at 0x260fa3: file /home/pedro/gdb/binutils-gdb/src/gdb/cli/cli-cmds.c, line 217.
 Temporary breakpoint 3 at 0xeb06c: file /home/pedro/gdb/binutils-gdb/src/gdb/gdb.c, line 25.
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

 Temporary breakpoint 3, main (argc=1, argv=0x7fffffffdc48) at /home/pedro/gdb/binutils-gdb/src/gdb/gdb.c:25
 25      {

 real    0m1.431s
 user    0m2.395s
 sys     0m0.091s

Re: [PATCH] Fix method naming bug in new DWARF indexer

[Tom Tromey]

Pedro> Hmm, it actually fixes most of the performance for me.  With that
Pedro> new patch (and I guess the linkage names patch made a difference
Pedro> too), I see roughly the same startup time in new gdb vs old gdb
Pedro> when using the same index, and either an index generated by old
Pedro> gdb, or by new gdb.

I'm going to check it in.

Pedro>  [369902] intrusive_list<inferior, intrusive_base_node<inferior> >::begin:
Pedro>           9 [global, function]
Pedro>           255 [global, function]
Pedro> while in the new indexer, for the same function, we have:
[...]

Pedro> What was the logic that the old writer used to come up with the
Pedro> seemingly right number of function entries?

It looks like the old reader checked the value of DW_AT_inline:

-      if (pdi->has_pc_info || pdi->has_range_info
-	  || (!pdi->is_external && pdi->may_be_inlined))
-	{
-	  if (!pdi->is_declaration)
-	    /* Ignore subprogram DIEs that do not have a name, they are
-	       illegal.  Do not emit a complaint at this point, we will
-	       do so when we convert this psymtab into a symtab.  */
-	    if (pdi->name (cu))
-	      add_partial_symbol (pdi, cu);
-	}

and

-	case DW_AT_inline:
-	  {
-	    LONGEST value = attr.constant_value (-1);
-	    if (value == DW_INL_inlined
-		|| value == DW_INL_declared_inlined)
-	      may_be_inlined = 1;
-	  }
-	  break;

Pedro> For the time being, I'll continue using indexes, because as soon
Pedro> as you need to run to a breakpoint from the command line, which I
Pedro> do all the time, then having an index still beats the new
Pedro> scanner.

Yeah :(

What happens here is that gdb does the finalization step in the
background.  Interactively, this helps make it seem faster.  However,
it's not really faster, it is just deferring some work.

You can see it with an invocation like:

   gdb -q -batch -iex 'set debug timestamp 1' -iex 'set debug dwarf-read 1' -ex start ./gdb

Here for gdb-with-an-index I see:

    0.820423 [dwarf-read] dwarf2_initialize_objfile: found gdb index from file
    1.699175 [dwarf-read] process_queue: Expanding one or more symtabs of objfile /tmp/gdb.idx ...

But for ordinary gdb:

    1.310033 [dwarf-read] dwarf2_build_psymtabs_hard: Done building psymtabs of /tmp/gdb
    4.666649 [dwarf-read] process_queue: Expanding one or more symtabs of objfile /tmp/gdb ...

This is disappointing of course.  It might be possible to reduce this
time at the cost of a bit more complexity in the lookup code and perhaps
a bit more memory use.  For example, right now finalization merges the
results from all the readers into a single entry table -- but maybe the
table could remain sharded instead.  Another possibility might be to do
the work in worker threads and, instead of sharding the result, pre-sort
the vectors and use a sorted merge operation to combine them.

I'll file some bugs about these things.
I'm starting to lose track of what I still need to fix up.

Tom

Re: [PATCH] Fix method naming bug in new DWARF indexer

[Pedro Alves]

On 2022-04-29 20:47, Tom Tromey wrote:
> Pedro> Hmm, it actually fixes most of the performance for me.  With that
> Pedro> new patch (and I guess the linkage names patch made a difference
> Pedro> too), I see roughly the same startup time in new gdb vs old gdb
> Pedro> when using the same index, and either an index generated by old
> Pedro> gdb, or by new gdb.
> 
> I'm going to check it in.
> 
> Pedro>  [369902] intrusive_list<inferior, intrusive_base_node<inferior> >::begin:
> Pedro>           9 [global, function]
> Pedro>           255 [global, function]
> Pedro> while in the new indexer, for the same function, we have:
> [...]
> 
> Pedro> What was the logic that the old writer used to come up with the
> Pedro> seemingly right number of function entries?
> 
> It looks like the old reader checked the value of DW_AT_inline:
> 
> -      if (pdi->has_pc_info || pdi->has_range_info
> -	  || (!pdi->is_external && pdi->may_be_inlined))
> -	{
> -	  if (!pdi->is_declaration)
> -	    /* Ignore subprogram DIEs that do not have a name, they are
> -	       illegal.  Do not emit a complaint at this point, we will
> -	       do so when we convert this psymtab into a symtab.  */
> -	    if (pdi->name (cu))
> -	      add_partial_symbol (pdi, cu);
> -	}
> 
> and
> 
> -	case DW_AT_inline:
> -	  {
> -	    LONGEST value = attr.constant_value (-1);
> -	    if (value == DW_INL_inlined
> -		|| value == DW_INL_declared_inlined)
> -	      may_be_inlined = 1;
> -	  }
> -	  break;

Hmm, inlines was my original suspicion, but seeing that setting a breakpoint at
"intrusive_list<inferior, intrusive_base_node<inferior> >::begin", even with
-readnow gives you 2 locations, same number of locations as entries in the old index,
could it be that this is more about declarations vs definitions?  There's that check for
has_pc_info or has_range_info too.