gdb/dwarf: when in dwarf2_cu, read addr_size from dwarf2_cu::header

New in v2: make the test try with indexes by default

This patch fixes a crash caused by GDB trying to read from a section not
read in.  The bug happens in those specific circumstances:

 - reading a type unit from .dwo
 - that type unit has a stub in the main file
 - there is a GDB index (.gdb_index) present

This crash is the cause of the following test failure, with the
cc-with-gdb-index target board:

    $ make check TESTS="gdb.dwarf2/fission-reread.exp" RUNTESTFLAGS="--target_board=cc-with-gdb-index"
    Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/fission-reread.exp ...
    ERROR: GDB process no longer exists

Or, manually:

    $ ./gdb -nx -q --data-directory=data-directory /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-reread/fission-reread -ex "p 1"
    Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.dwarf2/fission-reread/fission-reread...

    Fatal signal: Segmentation fault

For this last one, you need to interrupt the test (e.g. add a return)
before the test deletes the .dwo file.

The backtrace at the moment of the crash is:

    #0  0x0000555566968f7f in bfd_getl32 (p=0x0) at /home/simark/src/binutils-gdb/bfd/libbfd.c:846
    #1  0x00005555642e561d in read_initial_length (abfd=0x7d1ff1eb0e40, buf=0x0, bytes_read=0x7bfff0962fa0, handle_nonstd=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/leb.c:92
    #2  0x00005555647ca9ea in read_unit_head (header=0x7d0ff1e068b0, info_ptr=0x0, section=0x7c3ff1dea7d0, section_kind=ruh_kind::COMPILE) at /home/simark/src/binutils-gdb/gdb/dwarf2/unit-head.c:44
    #3  0x000055556452e37e in dwarf2_per_cu::get_header (this=0x7d0ff1e06880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18531
    #4  0x000055556452e574 in dwarf2_per_cu::addr_size (this=0x7d0ff1e06880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:18544
    #5  0x000055556406af91 in dwarf2_cu::addr_type (this=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/cu.c:124
    #6  0x0000555564534e48 in set_die_type (die=0x7e0ff1f23dd0, type=0x7e0ff1f027f0, cu=0x7d7ff1e20880, skip_data_location=false) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:19020
    #7  0x00005555644dcc7b in read_structure_type (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:11239
    #8  0x000055556451c834 in read_type_die_1 (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16878
    #9  0x000055556451c5e0 in read_type_die (die=0x7e0ff1f23dd0, cu=0x7d7ff1e20880) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16861
    #10 0x0000555564526f3a in get_signatured_type (die=0x7e0ff1f0ffb0, signature=10386129560629316377, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:17998
    #11 0x000055556451c23b in lookup_die_type (die=0x7e0ff1f0ffb0, attr=0x7e0ff1f10008, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16808
    #12 0x000055556451b2e9 in die_type (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16684
    #13 0x000055556451457f in new_symbol (die=0x7e0ff1f0ffb0, type=0x0, cu=0x7d7ff1e0f480, space=0x0) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:16089
    #14 0x00005555644c52a4 in read_variable (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:9119
    #15 0x0000555564494072 in process_die (die=0x7e0ff1f0ffb0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:5197
    #16 0x000055556449c88e in read_file_scope (die=0x7e0ff1f0fdd0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:6125
    #17 0x0000555564493671 in process_die (die=0x7e0ff1f0fdd0, cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:5098
    #18 0x00005555644912f5 in process_full_comp_unit (cu=0x7d7ff1e0f480) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:4851
    #19 0x0000555564485e18 in process_queue (per_objfile=0x7d6ff1e71100) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:4161
    #20 0x000055556446391d in dw2_do_instantiate_symtab (per_cu=0x7ceff1de42d0, per_objfile=0x7d6ff1e71100, skip_partial=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1650
    #21 0x0000555564463b3c in dw2_instantiate_symtab (per_cu=0x7ceff1de42d0, per_objfile=0x7d6ff1e71100, skip_partial=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1671
    #22 0x00005555644687fd in dwarf2_base_index_functions::expand_all_symtabs (this=0x7c1ff1e04990, objfile=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/dwarf2/read.c:1990
    #23 0x0000555564381050 in cooked_index_functions::expand_all_symtabs (this=0x7c1ff1e04990, objfile=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/dwarf2/cooked-index.h:237
    #24 0x0000555565df5b0d in objfile::expand_all_symtabs (this=0x7d5ff1e46080) at /home/simark/src/binutils-gdb/gdb/symfile-debug.c:372
    #25 0x0000555565eafc4a in maintenance_expand_symtabs (args=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/symmisc.c:914

The main file contains a stub (skeleton) for a compilation unit and a
stub for a type unit.   The .dwo file contains a compilation unit and a
type unit matching those stubs.  When doing the initial scan of the main
file, the DWARF reader parses the CU/TU list from the GDB index
(.gdb_index), and thus creates a signatured_type object based on that.
The section field of this signatured_type points to the .debug_types
section in the main file, the one containing the stub.  And because GDB
trusts the GDB index, it never needs to look at that .debug_types
section in the main file.  That section remains not read in.

When expanding the compilation unit, GDB encounters a type unit
reference (by signature) corresponding to the type in the type unit.  We
get in lookup_dwo_signatured_type, trying to see if there is a type unit
matching that signature in the current .dwo file.  We proceed to read
and expand that type unit, until we eventually get to a
dwarf2_cu::addr_type() call, which does:

     int addr_size = this->per_cu->addr_size ();

dwarf2_per_cu::addr_size() tries to read the header from the section
pointed to by dwarf2_per_cu::section which, if you recall, is the
.debug_types section in the main file that was never read in.  That
causes the segfault.

All this was working fine before these patches of mine, that tried to do
some cleanups:

    a47e2297fc28 ("gdb/dwarf: pass section offset to dwarf2_per_cu_data constructor")
    c44ab627b021 ("gdb/dwarf: pass section to dwarf2_per_cu_data constructor")
    39ee8c928551 ("gdb/dwarf: pass unit length to dwarf2_per_cu_data constructor")

Before these patches, the fill_in_sig_entry_from_dwo_entry function
(called from lookup_dwo_signatured_type, among others) would overwrite
some dwarf2_per_cu fields (including the section) to point to the .dwo,
rather than represent what's in the main file.  Therefore, the header
would have been read from the unit in the .dwo file, and things would
have been fine.

When doing these changes, I mistakenly assumed that the section written
by fill_in_sig_entry_from_dwo_entry was the same as the section already
there, which is why I removed the statements overwriting the section
field (and the two others).  To my defense, here's the comment on
dwarf2_per_cu::section:

    /* The section this CU/TU lives in.
       If the DIE refers to a DWO file, this is always the original die,
       not the DWO file.  */
    struct dwarf2_section_info *section = nullptr;

I would prefer to not reintroduce the behavior of overwriting the
section info in dwarf2_per_cu, because:

 1. I find it confusing, I like the invariant of dwarf2_per_cu::section
    points to the stub, and dwarf2_cu::section points to where we
    actually read the debug info from.
 2. The dwarf2_per_bfd::all_units vector is nowadays sorted by (section,
    section offset).  If we change the section and section offset of a
    dwarf2_per_cu, then we can no longer do binary searches in it, we
    would have to re-sort the vector (not a big deal, but still adds to
    the confusion).

One possible fix would be to make sure that the section is read in when
reading the header, probably in dwarf2_per_cu::get_header.  An approach
like that was proposed by Andrew initially, here:

  https://inbox.sourceware.org/gdb-patches/60ba2b019930fd6164f8e6ab6cb2e396c32c6ac2.1759486109.git.aburgess@redhat.com/

It would work, but there is a more straightforward fix for this
particular problem, I believe.  In dwarf2_cu, we have access to the
header read from the unit we're actually reading the DWARF from.  In the
DWO case, that is the header read from the .dwo file.  We can get the
address size from there instead of going through the dwarf2_per_cu
object.  This is what this patch does.

However, there are other case where we get the address (or offset) size
from the dwarf2_per_cu in the DWARF expression evaluator (expr.c,
loc.c), that could cause a similar crash. The next patch handles these
cases.

Modify the gdb.dwarf2/fission-reread.exp test so that it tries running
with an index even with the standard board (that part was originally
written by Andrew).

Finally, just to put things in context, having a stub in the main file
for a type unit is obsolete.  It happened in the gcc 4.x days, until
this commit:

    commit 4dd7c3b285daf030da0ff9c0d5e2f79b24943d1e
    Author: Cary Coutant <ccoutant@google.com>
    Date:   Fri Aug 8 20:33:26 2014 +0000

        Remove skeleton type units that were being produced with -gsplit-dwarf.

In DWARF 5, split type units don't have stubs, only split compilations
units do.

Change-Id: Icc5014276c75bf3126ccb43a4424e96ca1a51f06
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33307
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>

diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index b3851512f8956f5278665830219d40afd9341496..6e5a41e2aec85b23b88fab2b2d6cb2b41c0c75c6 100644 (file)
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -59,9 +59,8 @@ dwarf2_cu::dwarf2_cu (dwarf2_per_cu *per_cu, dwarf2_per_objfile *per_objfile)
 struct type *
 dwarf2_cu::addr_sized_int_type (bool unsigned_p) const
 {
-  int addr_size = this->per_cu->addr_size ();
   struct type *result = objfile_int_type (this->per_objfile->objfile,
-                                         addr_size, unsigned_p);
+                                         this->header.addr_size, unsigned_p);
   type_allocator alloc (per_objfile->objfile, lang ());
   return alloc.copy_type (result);
 }
@@ -124,9 +123,8 @@ dwarf2_cu::addr_type () const
   struct objfile *objfile = this->per_objfile->objfile;
   struct type *void_type = builtin_type (objfile)->builtin_void;
   struct type *addr_type = lookup_pointer_type (void_type);
-  int addr_size = this->per_cu->addr_size ();
 
-  if (addr_type->length () == addr_size)
+  if (addr_type->length () == this->header.addr_size)
     return addr_type;
 
   addr_type = addr_sized_int_type (addr_type->is_unsigned ());

diff --git a/gdb/testsuite/gdb.dwarf2/fission-reread.exp b/gdb/testsuite/gdb.dwarf2/fission-reread.exp
index 8a123770466126a60277fa3ebdecc02f2dc0012e..e7dc819011661a764b8373b93c95eff3cdd72d2a 100644 (file)
--- a/gdb/testsuite/gdb.dwarf2/fission-reread.exp
+++ b/gdb/testsuite/gdb.dwarf2/fission-reread.exp
@@ -43,26 +43,56 @@ if {[build_executable_and_dwo_files "$testfile.exp" "${binfile}" $options \
     return -1
 }
 
-clean_restart
-gdb_load_no_complaints $binfile
+# Load FILENAME, perform some basic tests to confirm that the debug
+# information has been found, then unload FILENAME.
+proc run_test { filename } {
+    clean_restart
+    gdb_load_no_complaints $filename
 
-gdb_test "break -q main" "Breakpoint.*at.*"
+    gdb_test "break -q main" "Breakpoint.*at.*"
 
-gdb_test "ptype baz" "type = class foo {.*"
+    gdb_test "ptype baz" "type = class foo \{.*"
 
-# If we get this far gdb didn't crash, nor did an error occur.
-pass $testfile
+    # If we get this far gdb didn't crash, nor did an error occur.
+    pass "load executable without crashing"
 
-gdb_unload
-# If we get this far gdb didn't crash, nor did an error occur.
-pass "$testfile - unload"
+    gdb_unload
 
-# Test-case for PR24620: Delete the .dwo file and verify that
-# save gdb-index doesn't crash.
+    # If we get this far gdb didn't crash, nor did an error occur.
+    pass "unload executable without crashing"
+}
+
+# Run the tests with BINFILE as it was built by default.
+run_test $binfile
+
+# Try adding a gdb-index and dwarf-5 style index, and then rerun the test.
+foreach_with_prefix index_type { gdb dwarf5 } {
+    set binfile_with_index ${binfile}-idx-${index_type}
+
+    remote_exec build "cp $binfile $binfile_with_index"
+
+    if { $index_type eq "gdb" } {
+       set style ""
+    } else {
+       set style "-dwarf-5"
+    }
+
+    # This is expected to fail if the binary already has an index.
+    if {[ensure_gdb_index $binfile_with_index $style] != 1} {
+       unsupported "couldn't add $index_type index"
+       break
+    }
+
+    run_test $binfile_with_index
+}
+
+# Test-case for PR24620: Delete the .dwo file and verify that save
+# gdb-index doesn't crash.  This uses the original compilatin result,
+# without any index added by the above block.
 remote_file target delete $dwo
 save_vars { GDBFLAGS } {
     append GDBFLAGS " -iex \"maint set dwarf synchronous on\""
-    clean_restart $::testfile
+    clean_restart $testfile
 }
 set output_dir [standard_output_file ""]
 set cmd "save gdb-index"