Bug 24820 - .debug_names has incorrect contents
Summary: .debug_names has incorrect contents
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: symtab (show other bugs)
Version: unknown
: P2 normal
Target Milestone: 15.1
Assignee: Tom Tromey
URL:
Keywords:
Depends on:
Blocks: 23504 27374 27453 31132 24549 25950
  Show dependency treegraph
 
Reported: 2019-07-17 19:34 UTC by Tom Tromey
Modified: 2024-04-22 21:18 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2019-07-17 19:34:21 UTC
gdb's "save gdb-index -dwarf-5" command creates a .debug_names index
that does not follow what I believe the DWARF standard specifies.
Then, gdb's .debug_names reader assumes that the index is gdb-like.

This is a problem for other readers and other writers; and anyway
gdb should follow the spec regardless.

Here's a simple test case that Pedro wrote:

namespace A {

namespace B {

namespace C {

void D ()
{
}

}

}
}

int main ()
{
  A::B::C::D();
  return 0;
}


gdb creates this index:

Symbol table:
[  0] #0b888030 int: <1> DW_TAG_typedef DW_IDX_compile_unit=0 DW_IDX_GNU_internal=1
[  1] #7c9a7f6a main: <2> DW_TAG_subprogram DW_IDX_compile_unit=0 DW_IDX_GNU_external=1
[  2] #ba2c1ff3 A::B::C: <3> DW_TAG_typedef DW_IDX_compile_unit=0 DW_IDX_GNU_external=1
[  3] #ac31fdab A::B::C::D: <2> DW_TAG_subprogram DW_IDX_compile_unit=0 DW_IDX_GNU_external=1
[  4] #7c933edc A::B: <3> DW_TAG_typedef DW_IDX_compile_unit=0 DW_IDX_GNU_external=1
[  5] #0002b606 A: <3> DW_TAG_typedef DW_IDX_compile_unit=0 DW_IDX_GNU_external=1


However, clang creates this index:

[  0] #960aadf5 _ZN1A1B1C1DEv: <46> DW_TAG_subprogram DW_IDX_die_offset=<0x2c>
[  1] #0002b606 A: <57> DW_TAG_namespace DW_IDX_die_offset=<0x26>
[  2] #0b888030 int: <36> DW_TAG_base_type DW_IDX_die_offset=<0x58>
[  3] #7c9a7f6a main: <46> DW_TAG_subprogram DW_IDX_die_offset=<0x42>
[  4] #0002b607 B: <57> DW_TAG_namespace DW_IDX_die_offset=<0x28>
[  5] #0002b608 C: <57> DW_TAG_namespace DW_IDX_die_offset=<0x2a>
[  6] #0002b609 D: <46> DW_TAG_subprogram DW_IDX_die_offset=<0x2c>


clang is more correct here.

First, it puts the mangled name into the index.  From 6.1.1.1:

• If a subprogram or inlined subroutine is included, and has a
DW_AT_linkage_name attribute, there will be an additional index entry for
the linkage name.


Second, my understanding is that the index is intended to hold
simple names, essentially just the DW_AT_name; but gdb puts fully-qualified
names into the index instead.  This does not seem to be spelled out
very explicitly in the DWARF standard, but it's what I recall from a
talk with Cary Coutant and Eric Christopher at a GCC Summit years ago.
Supporting evidence is what clang does (since this was based on some
earlier work by Apple) and also this text in 6.1.1.1:

The intent of the above rules is to provide the consumer with some assurance that looking
up an unqualified name in the index will yield all relevant debugging information entries
that provide a defining declaration at global scope for that name.


Note that it's fine for gdb to supplement the name list, though IMO
it would be better not to bother.
Comment 1 Tom Tromey 2021-02-18 16:27:19 UTC
Note to self, you need -gpubnames to get this output from clang.
Comment 2 Tom Tromey 2021-06-11 16:30:55 UTC
If the executable does not include a .debug_aranges section,
gdb will not add it.  However, it is required for .debug_names
to work properly.  I think this is another bug in the .debug_names
implementation.
Comment 3 Tom Tromey 2022-04-22 18:03:02 UTC
The new indexer makes rewriting this more feasible, as the new
cooked index internal representation is more or less explicitly
designed after .debug_names.

The reader will have to be rewritten as well.

We can keep the old reader around for a while if we want.
This can be done by looking at the .debug_names augmentation
string, and by using a new string for the new version.
This would also let us implement the "main name" hack that
we discussed somewhere else -- that is, putting the name-of-main
or perhaps some other bit of data just after the augmentation string.

If we want gdb to read clang-style output, I think some extra
work would be needed to deduce the namespace structure from
the index.  Perhaps some DIE scanning would be required.
Comment 4 Tom Tromey 2023-01-23 19:52:56 UTC
Aaron's comment here is worth noting: https://sourceware.org/pipermail/gdb-patches/2023-January/195718.html

   In the case of libxul, the combined size of its .debug_names, .debug_str and
   .debug_aranges is about 72% larger than the size of its .gdb_index (690 MB
   vs. 400 MB).  At least for this library we are much better off sticking with
   .gdb_index.

Even if we fix .debug_names generation, it's likely to be larger
than .gdb_index.

I wonder if we should just abandon .debug_names and instead upgrade
.gdb_index to work more like the cooked index.
Comment 5 Tom Tromey 2023-12-03 00:07:17 UTC
.debug_names is a bit weird.

The 'parent' entry refers to an index in the name table,
but this leaves no way to decide which of several meanings
of the name might be the parent.  Normally for gdb this doesn't
matter, but it's still strange.

clang doesn't emit parent entries so I think those tables
can't be used.

gdb needs to know the CU language but this isn't in the tables.
gdb can emit it as an extension of course.  I suppose we could
read the first DIE of every CU -- but that's precisely the
kind of work we want to avoid.

gdb will have to mark some entries as linkage names.  Otherwise
there's no way to tell.

The special case for "(anonymous namespace)" in the DWARF spec
is bad, because it means that gdb might have to add to the
string table -- but only for this one string.  (I used to think
we'd need to emit more strings, but that was mistaken.)
For the index cache we can probably handle this via a trick.

It's kind of sad that DWARF invented a new format and then didn't
provide enough guidance for tools to be compatible at all.
Comment 6 Tom Tromey 2023-12-03 00:13:09 UTC
(In reply to Tom Tromey from comment #5)

> It's kind of sad that DWARF invented a new format and then didn't
> provide enough guidance for tools to be compatible at all.

Well, maybe this isn't really fair, the missing parentage is
maybe just a clang bug, not a bug in the spec.

Another extension we'll need is to emit a flag for the main name.
Comment 7 Tom Tromey 2023-12-03 00:16:20 UTC
Oh, finally, the hash table is useless to gdb, so I think
gdb should simply not emit it.  It's not required by the spec.
Comment 8 Tom Tromey 2023-12-03 20:32:49 UTC
I wrote the reader part of this.  Going to start the writer soon.

It was simpler to base this on the DWARF background reader, since
that somewhat generalizes the cooked index code.

This should also solve the race that happens due to a warning
coming from the Ada code when writing the index -- by removing
these calls entirely.
Comment 9 Tom Tromey 2023-12-04 14:19:56 UTC
I thought "(anonymous namespace)" was going to be the only
name requiring special treatment, but I was wrong --
DW_FORM_string is often used for short strings, which then
do not appear in .debug_str, and so must be created for
the index to work.

Lame, but it only affects the index cache.
Comment 11 David Blaikie 2024-01-10 18:37:30 UTC
Hey Tom - catching up on this bug & giving whatever context I have from DWARF and Clang.

It'd be great to get this working for Clang/GCC/GDB/LLDB to have a portable index that provides good performance for at least both GDB and LLDB, and maybe other consumers - but two would at least be a good start.

If there are bugs in clang, we're interested in fixing them.
If there are bugs/unclear things in the DWARF spec, we're interested in fixing them.

> However, it is required for .debug_names to work properly.

I don't think that's true - at least doesn't seem to be true for lldb. Maybe this dovetails into:

> gdb needs to know the CU language but this isn't in the tables. gdb can emit it as an extension of course.  I suppose we could read the first DIE of every CU -- but that's precisely the kind of work we want to avoid.

Yeah, that's what I'd recommend for the language and address ranges - reading the first DIE of each CU. My experience is that's still quite cheap and quite a different proposition compared to "read all the DIEs and discover all the names from scratch".

(I wouldn't be averse to a new DWARFv5+ version of "aranges" that points to the rnglist the CU points to anyway - it'd add some overhead compared to discovering the ranges by reading the first DIE, but would be slightly faster too - but the current aranges are really verbose/totally duplicate with the rnglist for the CU - Clang hasn't emitted aranges by default for a decade or so because the size/perf tradeoff wasn't great)

> Oh, finally, the hash table is useless to gdb, so I think gdb should simply not emit it.

That surprises me - does gdb_index not contain some kind of fast lookup? Or is it a flat list that gdb loads on startup into a hash table?

Could gdb benefit from using the hash table, or are there some architectural reasons that's infeasible?

> Well, maybe this isn't really fair, the missing parentage is maybe just a clang bug, not a bug in the spec.

The spec doesn't mandate the parent references - but the Apple LLDB folks (they're in the process of moving to DWARFv5, so they're finding all the regressions in .debug_names compared to the original .apple_names that were introduced in the standardization process) are in the process of adding the parent links to Clang's output. https://github.com/llvm/llvm-project/pull/77457 and related work/discussions.

> gdb will have to mark some entries as linkage names.  Otherwise
there's no way to tell.

Is it not enough to check the mangling prefix? If it starts with `_Z` or another mangling prefix, it's a mangled name otherwise it's the name? (unless you need to differentiate the other direction - that a name isn't the linkage name, but flagging linkage names wouldn't be enough for that (if you have DW_AT_name "foo" you couldn't tell if that's a C linkage function where the name matches the linkage name, or a C++ linkage function where it's overloaded/etc and has some other linkage name))

Interested in understanding that more.

> It's kind of sad that DWARF invented a new format and then didn't provide enough guidance for tools to be compatible at all.

Happy to hear more about what guidance is missing/clarify things, if some things that the spec says are optional are basically necessities, we could make them mandatory in future DWARF versions, etc.

> In the case of libxul, the combined size of its .debug_names, .debug_str and .debug_aranges is about 72% larger than the size of its .gdb_index (690 MB vs. 400 MB).  At least for this library we are much better off sticking with .gdb_index.

Yeah, that's something that's concerned me a bit too - including the .o representation of .debug_names (at Google I've been mostly concerned with .o file sizes - the aggregate size of the objects needed by the linker in some large Google programs is quite problematic)... so, again, interested to know more about whether there are ways we could improve the format to be more dense or terse.
Comment 12 Tom Tromey 2024-01-10 20:51:54 UTC
Thanks for your notes.

My impression is that ranges can't reliably be reconstructed
from just the top DIE, but that a full scan must be done.
I'm not really sure about this -- possibly this is only true
for older compiler or something.  But gdb supports some truly 
awful crud so maybe we're stuck.

If it does work, we could certainly implement it though.
That would solve the language problem as well... it's not
great to have to scan abbrevs and whatnot like that, but meh,
that's DWARF for you.

About the hash table:

gdb_index is basically just a hash table.  It explicitly uses
the gdb canonicalized form of symbols.  This makes it possible
to do lookups.  I think completion is done by walking all the names.  

For debug_names (after this bug is fixed), though, the scanner
creates the "cooked" internal form (same that the DWARF scanner
creates).  This lets us share data structures, lookup code, etc.
This code doesn't use a hash table at all, so adding one seemed
like extra work.  And since canonicalization isn't really solved
in the spec or in the written hash table, we'd have to do some
kind of secondary backward map (from canonical form to lookup form)
to use it.

(It's not just gcc that doesn't canonicalize names btw, there's
another bug from some other vendor in this area.)

For the linkage name flag:

I see gdb uses this in two spots.

One spot is when constructing the full name of an entry.  There's
probably some test where a linkage name is in some scope, and this
approach was expedient.  We could just register these without parents.

However, it's also used in gdb's post-scanning finalization step.
This is gross but some languages don't emit DWARF hierarchical forms.
So, for instance, for Ada gdb will synthesize the package names for
a fully-qualified symbol.

Also it isn't reliable to look for _Z for linkage names.  This works
for C++ and to some extent Rust (Rust is / will be switching mangling
schemes), but not for Go, Ada, Fortran.
Comment 13 Tom Tromey 2024-01-10 20:58:53 UTC
One last note about the size and whatnot.

To my mind, one of the biggest problems with DWARF is
that it is very difficult to read.  The new scanner,
which has acceptable-ish performance, was a pretty big
effort and we're still tracking down the occasional
data race (since threading was the only way to make it
really fast).

However, this situation seems absurd to me.  DWARF is
hard to read -- but this is due to decisions made in
the design, not really anything intrinsic to the problem area.
That is, DWARF gives us abbrevs and a generically
hierarchical structure, when really gdb (and IMO debuggers
in general) wants something different.  In theory this stuff
can be used for other things, but in practice this approach
means optimizing for these hypothetical other uses at
the expense of the 90% use case.

So, rather than putting effort into an index, whether it
be .gdb_index or .debug_names, it would be much better to
tackle this at the source and make reading cheap and easy.
(This btw is why I put off .debug_names so long, I just
had trouble getting myself over the feeling that I was
working on the wrong end of the problem.)

This is partly the idea of CTF, though they went the C-only
route and also didn't really integrate into gdb very well.
Comment 14 Sourceware Commits 2024-01-18 20:38:15 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=91a42a618085f236c5f6527322f88044e7550a63

commit 91a42a618085f236c5f6527322f88044e7550a63
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Dec 3 18:20:37 2023 -0700

    Rewrite .debug_names writer
    
    This rewrites GDB's .debug_names writer.  It is now closer to the form
    imagined in the DWARF spec.  In particular, names are emitted exactly
    as they appear in the original DWARF.
    
    In order to make the reader work nicely, some extensions were needed.
    These were all documented in an earlier patch.  Note that in
    particular this writer solves the "main name" problem by putting a
    flag into the table.
    
    GDB does not use the .debug_names hash table, so it also does not
    write one.  I consider this hash table to be essentially useless in
    general, due to the name canonicalization problem -- while DWARF says
    that writers should use the system demangling style, (1) this style
    varies across systems, so it can't truly be relied on; and (2) at
    least GCC and one other compiler don't actually follow this part of
    the spec anyway.
    
    It's important to note, though, that even if the hash was somehow
    useful, GDB probably still would not use it -- a sorted list of names
    is needed for completion and performs reasonably well for other
    lookups, so a hash table is just overhead, IMO.
    
    String emission is also simplified.  There's no need in this writer to
    ingest the contents of .debug_str.
    
    A couple of tests are updated to reflect the fact that they now "fail"
    because the tests don't include .debug_aranges in the .S file.
    Arguably the .debug_names writer should also create this section; but
    I did not implement that in this series, and there is a separate bug
    about it.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24820
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24549
Comment 15 Tom Tromey 2024-01-18 20:38:33 UTC
Fixed.
Comment 16 David Blaikie 2024-04-22 21:18:37 UTC
(In reply to Tom Tromey from comment #13)
> One last note about the size and whatnot.
> 
> To my mind, one of the biggest problems with DWARF is
> that it is very difficult to read.  The new scanner,
> which has acceptable-ish performance, was a pretty big
> effort and we're still tracking down the occasional
> data race (since threading was the only way to make it
> really fast).
> 
> However, this situation seems absurd to me.  DWARF is
> hard to read -- but this is due to decisions made in
> the design, not really anything intrinsic to the problem area.
> That is, DWARF gives us abbrevs and a generically
> hierarchical structure, when really gdb (and IMO debuggers
> in general) wants something different.  In theory this stuff
> can be used for other things, but in practice this approach
> means optimizing for these hypothetical other uses at
> the expense of the 90% use case.
> 
> So, rather than putting effort into an index, whether it
> be .gdb_index or .debug_names, it would be much better to
> tackle this at the source and make reading cheap and easy.
> (This btw is why I put off .debug_names so long, I just
> had trouble getting myself over the feeling that I was
> working on the wrong end of the problem.)
> 
> This is partly the idea of CTF, though they went the C-only
> route and also didn't really integrate into gdb very well.

I'd love to subscribe to your newsletter/talk with you more about your ideas on this front - is there any particular forum that'd be preferred/most useful to do that?