[2/2] gdb/dwarf: move CU check up in cutu_reader::read_cutu_die_from_dwo

  From: Simon Marchi <simon.marchi@efficios.com>

We have this pattern of check in multiple places:

    /* Skip dummy compilation units.  */
    if (m_info_ptr >= begin_info_ptr + this_cu->length ()
        || peek_abbrev_code (abfd, m_info_ptr) == 0)
      m_dummy_p = true;

In all places except one (read_cutu_die_from_dwo), this is done after
reading the unit header but before potentially reading the first DIE.
The effect is that we consider dummy units that have no DIE at all.
Either the "data" portion of the unit (the portion after the header) has
a size of zero, or the first abbrev code is 0, i.e. "end of list".

According to this old commit I found [1], dummy CUs were used as filler
for incremental LTO linking.  A comment reads:

   WARNING: If THIS_CU is a "dummy CU" (used as filler by the incremental
   linker) then DIE_READER_FUNC will not get called.

In read_cutu_die_from_dwo, however, this check is done after having read
the first DIE.  So at the time of the check, m_info_ptr has already been
advanced just past the first DIE.  As a result, compilations units with
a single DIE are considered (erroneously, IMO) as dummy.

In commit aab6de1613df ("gdb/dwarf: fix spurious error when encountering
dummy CU") [2], I mentioned a real world case where compilation units
with a single top-level DIE were being considered dummy.  I believe that
those units should not actually have been treated as dummy.  A CU with
just one DIE may not be very interesting, but I don't see any reason to
consider it dummy.

Move the dummy check above the read_toplevel_die call, and return early
if the CU is dummy.

I am 99% convinced that it's not even possible to encounter an empty
unit here, and considered turning it into an assert (it did pass the
testsuite).  This function is passed a dwo_unit, and functions that
create a dwo_unit are:

 - create_debug_type_hash_table (creates a dwo_unit for each type unit
   found in a dwo file)
 - create_cus_hash_table (creates a dwo_unit for each comp unit found in
   a dwo file)
 - create_dwo_unit_in_dwp_v1
 - create_dwo_unit_in_dwp_v2
 - create_dwo_unit_in_dwp_v5

In the first two, there are already dummy checks, so we wouldn't even
get to read_cutu_die_from_dwo for such an empty CU.  However, in the
last three, there is no such checks, we just trust the dwp file's index
and create dwo_units out of that.  So I guess it would be possible to
craft a broken dwp file with a CU that has no DIE.  Out of caution, I
didn't switch that to an assert, but I also don't really know what would
be the mode of failure if that were to happen.

Regtested using the various DWARF target boards on Debian 12.

[1] https://gitlab.com/gnutools/binutils-gdb/-/commit/dee91e82ae87f379c90fddff8db7c4b54a116609#dd409f60ba6f9c066432dafbda7093ac5eec76d1_3434_3419
[2] https://gitlab.com/gnutools/binutils-gdb/-/commit/aab6de1613df693059a6a2b505cc8f20d479d109

Change-Id: I90e6fa205cb2d23ebebeae6ae7806461596f9ace
---
 gdb/dwarf2/read.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
  

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> In read_cutu_die_from_dwo, however, this check is done after having read
Simon> the first DIE.  So at the time of the check, m_info_ptr has already been
Simon> advanced just past the first DIE.  As a result, compilations units with
Simon> a single DIE are considered (erroneously, IMO) as dummy.

Simon> In commit aab6de1613df ("gdb/dwarf: fix spurious error when encountering
Simon> dummy CU") [2], I mentioned a real world case where compilation units
Simon> with a single top-level DIE were being considered dummy.  I believe that
Simon> those units should not actually have been treated as dummy.  A CU with
Simon> just one DIE may not be very interesting, but I don't see any reason to
Simon> consider it dummy.

This is definitely a weird / pathological case.  I wonder though if it
happens in the test suite at all?  Like maybe the DWARF assembler emits
this sometime?

I have no problem with the patch, I am just wondering if a "bare"
top-level DIE could cause some previously obscured crash.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
  

On 2025-03-20 16:03, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> In read_cutu_die_from_dwo, however, this check is done after having read
> Simon> the first DIE.  So at the time of the check, m_info_ptr has already been
> Simon> advanced just past the first DIE.  As a result, compilations units with
> Simon> a single DIE are considered (erroneously, IMO) as dummy.
> 
> Simon> In commit aab6de1613df ("gdb/dwarf: fix spurious error when encountering
> Simon> dummy CU") [2], I mentioned a real world case where compilation units
> Simon> with a single top-level DIE were being considered dummy.  I believe that
> Simon> those units should not actually have been treated as dummy.  A CU with
> Simon> just one DIE may not be very interesting, but I don't see any reason to
> Simon> consider it dummy.
> 
> This is definitely a weird / pathological case.  I wonder though if it
> happens in the test suite at all?  Like maybe the DWARF assembler emits
> this sometime?

I don't think an existing test case produces that.  I put an assert in
there:

  gdb_assert (m_top_level_die->has_children);

and ran gdb.dwarf2/*.exp, no crash.

It's actually not difficult to produce: just build an empty .c file, you
get an compile unit with no children.  But only with gcc, with clang you
don't get any debug info.  In my real-world use case, it's because an
entire file is ifdef-ed out.  It should be possible to build a test case
for this with the DWARF assembler, but I would need to remember how to
write those :).

> I have no problem with the patch, I am just wondering if a "bare"
> top-level DIE could cause some previously obscured crash.

I don't really see what kind of problems it could cause.  We just won't
create any index entry nor symbols from it.

Simon
  

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> It's actually not difficult to produce: just build an empty .c file, you
Simon> get an compile unit with no children.  But only with gcc, with clang you
Simon> don't get any debug info.  In my real-world use case, it's because an
Simon> entire file is ifdef-ed out.  It should be possible to build a test case
Simon> for this with the DWARF assembler, but I would need to remember how to
Simon> write those :).

I don't really think we need one but you could copy-paste any existing
one and make an empty compile unit like

cu { .. } { compile_unit {} {} }

>> I have no problem with the patch, I am just wondering if a "bare"
>> top-level DIE could cause some previously obscured crash.

Simon> I don't really see what kind of problems it could cause.  We just won't
Simon> create any index entry nor symbols from it.

I was more concerned that maybe there was code in gdb assuming that a
compilation unit DIE always had children.

Anyway thank you for looking, and this all seems fine to me.

Tom
  

On 2025-03-21 10:20, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> It's actually not difficult to produce: just build an empty .c file, you
> Simon> get an compile unit with no children.  But only with gcc, with clang you
> Simon> don't get any debug info.  In my real-world use case, it's because an
> Simon> entire file is ifdef-ed out.  It should be possible to build a test case
> Simon> for this with the DWARF assembler, but I would need to remember how to
> Simon> write those :).
> 
> I don't really think we need one but you could copy-paste any existing
> one and make an empty compile unit like
> 
> cu { .. } { compile_unit {} {} }
> 
>>> I have no problem with the patch, I am just wondering if a "bare"
>>> top-level DIE could cause some previously obscured crash.
> 
> Simon> I don't really see what kind of problems it could cause.  We just won't
> Simon> create any index entry nor symbols from it.
> 
> I was more concerned that maybe there was code in gdb assuming that a
> compilation unit DIE always had children.
> 
> Anyway thank you for looking, and this all seems fine to me.
> 
> Tom

I went ahead and pushed this.

For the test, beyond checking that GDB does not crash when loading a
file with such a CU, I'm not sure what more I could check.  Perhaps with
my other patch that lists DWARF CUs in "maint print objfiles", I could
check that there is an entry for the child-less CU.  But even then, I
don't think it has a user-visible impact, so it would not be that
useful.

Simon