Re: [PATCHv3] gdb: resolve class name via DW_AT_signature in cooked index

[Simon Marchi <simark@simark.ca>]

On 2026-09-01 09:31, Andrew Burgess wrote:
> In v3:
> 
>   - I forgot to run check-all-boards on v2.  There were some failures
>     due to GDB's output not matching the patterns, I've fixed what I
>     could in this iteration.
> 
>   - While looking at the different patterns I reworded one of the
>     complaint messages in cooked-index-shard.c to match a warning that
>     is emitted from elsewhere in the DWARF reader.  There's no real
>     functional change, it's just the wording that's updated.
> 
>   - There are still some check-all-boards failures, but I believe
>     these are all wider issues when running DWARF assembler tests
>     using the check-all-boards rule, so I'm ignoring them for now.
> 
> In v2:
> 
>   - Complete rewrite to address the issues that Tom raised.
> 
>   - Rebase to HEAD and retest.
> 
>   - New tests added to cover the DWO case and an error case.
> 
> Thanks,
> Andrew
> 
> ---
> 
> This commit fixes PR gdb/33447, an issue where looking up qualified
> member function names was not working for C++ binaries compiled with
> Clang when using the -fdebug-types-section flag.
> 
> Before this commit we would see this behaviour:
> 
>   (gdb) print base1::a_function
>   There is no field named a_function
> 
> When what we expect to see is:
> 
>   (gdb) print base1::a_function
>   $1 = {void (const base1 * const)} 0x403060 <base1::a_function() const>
> 
> The problem is that the cooked index is unable to determine the name
> of the parent class `base1` in this case, and so decides not to index
> `base1` or any of its child DIEs, which includes its member functions.
> 
> The problem was discovered by running gdb.cp/cpexprs-debug-types.exp
> with Clang:
> 
>   make check-gdb TESTS=gdb.cp/cpexprs-debug-types.exp \
>     RUNTESTFLAGS='CXX_FOR_TARGET=clang++ CC_FOR_TARGET=clang'
> 
> The cpexprs-debug-types.exp test forces use of the
> '-fdebug-types-section' flag, which is not on by default.  With this
> flag, class definitions are placed in type units, and the compile unit
> contains only a declaration stub for each class.  Both clang++ and g++
> emit these stubs, but they differ in one detail: GCC includes
> DW_AT_name on the stub, while clang++ does not, the stub carries only
> DW_AT_declaration and DW_AT_signature.  The class name is only
> available in the type unit, reachable by following the signature.
> 
> For example, Clang emits this in a CU:
> 
>   <1><2e7d>: DW_TAG_class_type
>      DW_AT_declaration : 1
>      DW_AT_signature   : 0x3abb...
>   <2><2eac>: DW_TAG_subprogram
>      DW_AT_name        : a_function
>      DW_AT_declaration : 1
> 
> The definition for a_function is elsewhere in the same CU:
> 
>   <1><3142>: DW_TAG_subprogram
>      DW_AT_specification: <0x2eac>
> 
> And in a TU elsewhere:
> 
>    Compilation Unit @ offset 0xd2e:
>     ... snip ...
>     Signature:     0x3abb...
>   <0><d46>: Abbrev Number: 1 (DW_TAG_type_unit)
>      ... snip ...
>   <1><d51>: Abbrev Number: 30 (DW_TAG_class_type)
>      ... snip ...
>      <d57>   DW_AT_name        : (indexed string: 0xaa): base1
> 
> To find the DW_AT_name the cooked index needs to look up the type
> within the TU.  Without the name the cooked indexer skips indexing
> `base1` as well as its children.
> 
> This look up used to work; it works in GDB 17.  PR gdb/33447
> incorrectly identifies commit c879f4dc3e317cf6353a45a803ecf00d577a13d8
> as the commit that introduced the regression.  This is actually the
> last working commit.  The problem was introduced by the next commit in
> the same series:
> 
>   commit 86ac8c546235a67d6a6bb29476a3a9ac8f7a620a
>   Date:   Thu Jan 2 15:28:18 2025 -0700
> 
>       Convert lookup_symbol_in_objfile
> 
> Prior to this commit GDB's symbol lookup had two phases, a search
> through already expanded symtabs, and a search via
> lookup_symbol_via_quick_fns.  After the above commit only
> lookup_symbol_via_quick_fns remains.
> 
> The lookup_symbol_via_quick_fns lookup, which relies on the indexer,
> was always broken, but the first phase, searching via expanded
> symtabs, could correctly find the type name via the signature.

What this generally means is that it could work if the CU had been
expanded previously, but not otherwise.

> An initial attempt to solve this problem tried to fix this problem
> within cooked_indexer::scan_attributes, calling lookup_signatured_type
> and finding the name that way.  However, there were three problems
> with this approach:
> 
>   1. Possible thread safety issues; calling lookup_signatured_type for
>      a DWO file ends up calling lookup_dwo_signatured_type, which can
>      call add_type_unit and finalize_all_units, which modify state
>      that is shared between parser threads.
> 
>   2. The dwarf2_per_cu::type_offset_in_section for a TU is only set
>      when the cutu_reader is constructed to parse that TU.  Calling
>      lookup_signatured_type doesn't fully parse the TU, it just finds
>      the TU.  The original code relied on type_offset_in_section being
>      valid in order to then parse the TU and extract the name.  This
>      would break if the CU was processed before the TU.
> 
>   3. For skeletonless TUs, these are not indexed until late in the
>      indexing process, after all the CUs have been indexed.  This
>      means that if the TU referenced by the signature was skeletonless
>      then the original approach would fail to find it.

Ack, and reminder (for myself and others) that skeletonless TU is
the norm when using type units + split DWARF.

> The new approach presented here is modeled more along the lines of the
> deferred parent handling that already exists within the indexer.  The
> following changes have been made:
> 
> 1. abbrev.c: Add DW_AT_signature to the set of attributes that mark a
>    DIE as "interesting" in has_specification_or_origin.  Without this,

"in has_specification_or_origin" makes it sound like
"has_specification_or_origin" is a function.

>    the unnamed class stub was classified as uninteresting at the
>    abbreviation level and scan_attributes was never called for it.
> 
> 2. cooked-indexer.c (cooked_indexer::scan_attributes): Capture the
>    DW_AT_signature attribute.
> 
> 3. cooked-indexer.c (cooked_indexer::index_dies): There are two
>    different jobs done here:
> 
>    (a) If a DIE has no name, but does have a signature, then give the
>        DIE a fake name (the empty string), and create an index entry
>        for the DIE.  Also keep a record that the cooked_index_entry
>        for this DIE has a deferred name.

Just wondering, if we end up not patching the entry for some reason,
will an entry with an empty name cause problems / match things it's not
supposed to match?  Like will the child of that nameless entry be
considered to be part of the top-level namespace or something like that?

If it happens that we have an entry with an unresolved name, perhaps we
should consider this entry invalid and just skip anything that would
require it.

> 
>    (b) If we are indexing a TU, make a record of the signature, and
>        the name of the primary type within the TU.  This builds a
>        signature to name map.
> 
> 4. cooked-index-worker.c (cooked_index_worker::done_reading): Merge
>    together all of the signature to name maps.
> 
> 5. cooked-index-shard.c (cooked_index_shard::finalize): Look through
>    all of the cooked_index_entry objects that were recorded in (3a),
>    for each use the signature to name map to lookup the name, and
>    update the cooked_index_entry.
> 
> There are a bunch of header file changes to add the new maps, and
> accessor functions, the steps above describe the core mechanism.
> 
> Added three new DWARF assembler tests.
> 
>   gdb.dwarf2/sig-type-unnamed-class.exp
>   gdb.dwarf2/sig-type-unnamed-class-dwo.exp:
>    These reproduce the problem case; there's a nameless declaration
>    stub that references its full type via DW_AT_signature.  To match
>    the Clang output as much as possible the member function definition
>    is separate and makes use of DW_AT_specification.  The -dwo test
>    places the DWARF into a DWO file and uses a skeletonless TU.
> 
>   gdb.dwarf2/sig-type-unnamed-class-bad-sig.exp:
>    This one tests some invalid DWARF, the type DIE references a
>    signature that doesn't exist.  In this case GDB just leaves the
>    type DIE with an empty name string, which means symbols cannot be
>    found.  I think this is fine though, the DWARF is corrupted in this
>    case.

The approach LGTM.  I had Claude review the patch while I was looking at
it myself, here are some relevant comments from it (him? her? them?):

 - This crashes:

    $ make check TESTS="gdb.dwarf2/sig-type-unnamed-class-bad-sig.exp" RUNTESTFLAGS=--target_board=cc-with-gdb-index

   I build with -D_GLIBCXX_DEBUG, so I see this:

      (gdb) file /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/sig-type-unnamed-class-bad-sig/sig-type-unnamed-class-bad-sig-4
      Reading symbols from /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/sig-type-unnamed-class-bad-sig/sig-type-unnamed-class-bad-sig-4...
      (gdb) /usr/include/c++/16/string_view:291: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::back() const [with _CharT = char; _Traits = std::char_traits<char>; const_reference = const char&]: Assertion 'this->_M_len > 0' failed.

   When we fail to find a signature, we complain and leave the empty
   string in place.  From what I understand, when we try to write the
   .gdb_index, we try to generate the name of the child, which leads to
   "::method" (the name of our entry would have appeared before ::, but
   now it's the empty string).  And something down the line doesn't like
   that.

   So yeah, I wonder if it wouldn't be better to leave the name as
   nullptr, that would force us to add some nullptr checks to realize
   that the entry doesn't have a valid name, and we would skip it.

 - the complaint runs on a thread pool worker, but no
   complaint_interceptor is installed in those threads, so
   complaint_internal writes straight to gdb_stderr from a worker
   thread, outside the collect-and-re-emit-on-the-main-thread machinery.
   Besides the raw thread-safety issue, the message can land at an
   arbitrary point in the main thread's output.  Using the
   complaint interceptor would fix both.

 - The comment on cooked_index_entry::name says that it always points
   into mapped DWARF sections, which is not true anymore.  The comment
   could talk about the "" case (or nullptr if we decided to go that
   route).

 - The complaint should maybe use DWARF_ERROR_PREFIX, like the identical
   complaints in read.c.

  - sig-type-unnamed-class-dwo.exp: the two foreach_with_prefix
    iterations write the same $binfile / -dw.o / .S names, unlike the
    other two tests which suffix with -${version}. Harmless for the run,
    but it makes post-mortem debugging of one version impossible.

> diff --git a/gdb/dwarf2/cooked-index-entry.h b/gdb/dwarf2/cooked-index-entry.h
> index 60ea581cbbe..f0ff356a6d1 100644
> --- a/gdb/dwarf2/cooked-index-entry.h
> +++ b/gdb/dwarf2/cooked-index-entry.h
> @@ -79,6 +79,11 @@ union cooked_index_entry_ref
>    parent_map::addr_type deferred;
>  };
>  
> +/* Type that maps DW_AT_signature values for a TU to the name of the
> +   primary type within the TU.  */
> +
> +using signature_to_name_map = std::unordered_map<ULONGEST, const char *>;

We should be using gdb::unordered_map.

> @@ -190,8 +191,30 @@ struct cooked_index_entry_name_ptr_eq
>  /* See cooked-index-shard.h.  */
>  
>  void
> -cooked_index_shard::finalize (const parent_map_map *parent_maps)
> +cooked_index_shard::finalize (const parent_map_map *parent_maps,
> +			      const signature_to_name_map *sig_names)

These two should be passed by reference, I suppose (maybe push an
obvious patch for the existing one).

>  {
> +  for (const std::pair<cooked_index_entry *, ULONGEST> &entry_to_sig
> +	 : m_deferred_names)

That would be a good use of structured bindings:

for (const auto &[entry, signature] : m_deferred_names)

> +    {
> +      ULONGEST signature = entry_to_sig.second;
> +      cooked_index_entry *entry = entry_to_sig.first;
> +
> +      const auto it = sig_names->find (signature);
> +      if (it != sig_names->end ())

I would suggest:

      if (const auto it = sig_names->find (signature);
	  it != sig_names->end ())

> @@ -134,6 +145,12 @@ class cooked_index_shard
>    addrmap_fixed *m_addrmap = nullptr;
>    /* Storage for canonical names.  */
>    gdb::string_set m_names;
> +
> +  /* Entries without a name, but with a signature.  These entries will
> +     have been given a fake name, the empty string when they were
> +     created, but we need to patch these up with a real name during
> +     finalization.  */
> +  std::vector<std::pair<cooked_index_entry *, ULONGEST>> m_deferred_names;

I would mind if you introduced a little local struct instead of using
std::pair:

    struct deferred_name
    {
      cooked_index_entry *entry;
      ULONGEST signature;
    };


>  using cooked_index_shard_up = std::unique_ptr<cooked_index_shard>;
> diff --git a/gdb/dwarf2/cooked-index-worker.c b/gdb/dwarf2/cooked-index-worker.c
> index 723e027172e..054b25ba147 100644
> --- a/gdb/dwarf2/cooked-index-worker.c
> +++ b/gdb/dwarf2/cooked-index-worker.c
> @@ -249,6 +249,15 @@ cooked_index_worker::done_reading ()
>        m_all_parents_map.add_map (*one_result.get_parent_map ());
>    }
>  
> +  {
> +    scoped_time_it time_it ("DWARF add signature name map", m_per_command_time);
> +
> +    /* Combine all of the signature to name maps.  */
> +    for (cooked_index_worker_result &one_result : m_results)
> +      for (const auto &[sig, name] : one_result.get_sig_name_map ())
> +	m_all_sig_names_map.emplace (sig, name);

I'm curious to see if that can be done more efficiently, but for now
this is fine.  I don't want to waste time prematurely optimizing when
the immediate goal is to fix the bug.

> @@ -637,6 +665,18 @@ cooked_indexer::index_dies (cutu_reader *reader,
>  	  m_die_range_map->add_entry (addr, addr, this_parent_entry);
>  	}
>  
> +      /* If THIS_DIE is the primary type within a TU, and has a valid
> +	 name, then add an entry mapping the signature to the name.  */
> +      if (found_name
> +	  && cu_for_entry->is_debug_types ()
> +	  && this_die == (reader->cu ()->header.sect_off
> +			  + to_underlying (reader->cu ()->header.type_offset_in_tu)))

You can use signatured_type::type_offset_in_section instead of doing the
addition (it should be the same value).  This should work:

    if (const signatured_type *st = cu_for_entry->as_signatured_type ();
        found_name && st != nullptr && this_die == st->type_offset_in_section)


> +	{
> +	  const signatured_type *st = cu_for_entry->as_signatured_type ();
> +	  if (st != nullptr)
> +	    m_index_storage->add_signatured_type_name (st->signature, name);

Here, st can't be nullptr, as you checked is_debug_types above.  You
could assert instead (but it's made irrelevant by the proposal above).

Simon