From: Simon Marchi Date: Wed, 6 Aug 2025 19:31:36 +0000 (-0400) Subject: gdb/dwarf: clear per_bfd::num_{comp,type}_units on error X-Git-Tag: gdb-17-branchpoint~314 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=94de78f9d0d4b9eecb4e3efc393ffa4abbae662e;p=thirdparty%2Fbinutils-gdb.git gdb/dwarf: clear per_bfd::num_{comp,type}_units on error Commit bedd6a7a44 ("gdb/dwarf: track compilation and type unit count") causes this internal error: $ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.dwarf2/debug-names-duplicate-cu/debug-names-duplicate-cu -ex "save gdb-index -dwarf-5 /tmp" -batch warning: Section .debug_names has incorrect number of CUs in CU table, ignoring .debug_names. /home/smarchi/src/binutils-gdb/gdb/dwarf2/index-write.c:1454: internal-error: write_debug_names: Assertion `comp_unit_counter == per_bfd->num_comp_units' failed. This is visible when running this test: $ make check TESTS="gdb.dwarf2/debug-names-duplicate-cu.exp" RUNTESTFLAGS="--target_board=cc-with-debug-names" ... Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/debug-names-duplicate-cu.exp ... gdb compile failed, warning: Section .debug_names has incorrect number of CUs in CU table, ignoring .debug_names. /home/smarchi/src/binutils-gdb/gdb/dwarf2/index-write.c:1454: internal-error: write_debug_names: Assertion `comp_unit_counter == per_bfd->num_comp_units' failed. ... === gdb Summary === # of untested testcases 1 However, it's easy to miss because it only causes an "UNTESTED" to be recorded, not a FAIL or UNRESOLVED. This is because the problem happens while trying to create the .debug_names index, as part of the test case compilation. The problem is: when we bail out from using .debug_names because we detect it is inconsistent with the units in .debug_info, we clear per_bfd->all_units, to destroy all units previously created, before proceeding to read the units with an index. However, we don't clear per_bfd->num_{comp,type}_units. As a result, per_bfd->all_units contains one unit, while per_bfd->num_comp_units is 2. Whenever we clear per_bfd->all_units, we should also clear per_bfd->num_{comp,type}_units. While at it, move this logic inside a scoped object. I added an assertion in finalize_all_units to verify that the size of per_bfd->all_units equals per_bfd->num_comp_units + per_bfd->num_type_units. This makes the problem (if omitting the fix) visible when running gdb.dwarf2/debug-names-duplicate-cu.exp with the unix (default) target board: ERROR: Couldn't load debug-names-duplicate-cu into GDB (GDB internal error). FAIL: gdb.dwarf2/debug-names-duplicate-cu.exp: find index type (GDB internal error) FAIL: gdb.dwarf2/debug-names-duplicate-cu.exp: find index type, check type is valid === gdb Summary === # of expected passes 1 # of unexpected failures 2 # of unresolved testcases 1 I considered changing the code to build a local vector of units first, then move it in per_bfd->all_units on success, that would avoid having to clean it up on error. I did not do it because it's a much larger change, but we could consider it. Change-Id: I49bcc0cb4b34aba3d882b27c8a93c168e8875c08 Approved-By: Tom Tromey --- diff --git a/gdb/dwarf2/read-debug-names.c b/gdb/dwarf2/read-debug-names.c index 97677c04f64..ddf49356722 100644 --- a/gdb/dwarf2/read-debug-names.c +++ b/gdb/dwarf2/read-debug-names.c @@ -768,12 +768,12 @@ build_and_check_cu_lists_from_debug_names (dwarf2_per_bfd *per_bfd, return build_and_check_cu_list_from_debug_names (per_bfd, dwz_map, dwz->info); } -/* This does all the work for dwarf2_read_debug_names, but putting it - into a separate function makes some cleanup a bit simpler. */ +/* See read-debug-names.h. */ -static bool -do_dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile) +bool +dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile) { + scoped_remove_all_units remove_all_units (*per_objfile->per_bfd); mapped_debug_names_reader map; mapped_debug_names_reader dwz_map; struct objfile *objfile = per_objfile->objfile; @@ -850,17 +850,7 @@ do_dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile) (per_objfile, std::move (map))); auto idx = std::make_unique (std::move (cidn)); per_bfd->start_reading (std::move (idx)); + remove_all_units.disable (); return true; } - -/* See read-debug-names.h. */ - -bool -dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile) -{ - bool result = do_dwarf2_read_debug_names (per_objfile); - if (!result) - per_objfile->per_bfd->all_units.clear (); - return result; -} diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c index 7dfba732451..fc7b654be1a 100644 --- a/gdb/dwarf2/read-gdb-index.c +++ b/gdb/dwarf2/read-gdb-index.c @@ -1489,6 +1489,7 @@ dwarf2_read_gdb_index offset_type cu_list_elements, types_list_elements, dwz_list_elements = 0; struct objfile *objfile = per_objfile->objfile; dwarf2_per_bfd *per_bfd = per_objfile->per_bfd; + scoped_remove_all_units remove_all_units (*per_bfd); gdb::array_view main_index_contents = get_gdb_index_contents (objfile, per_bfd); @@ -1544,10 +1545,7 @@ dwarf2_read_gdb_index an index. */ if (per_bfd->infos.size () > 1 || per_bfd->types.size () > 1) - { - per_bfd->all_units.clear (); - return false; - } + return false; dwarf2_section_info *section = (per_bfd->types.size () == 1 @@ -1566,6 +1564,7 @@ dwarf2_read_gdb_index set_main_name_from_gdb_index (per_objfile, map.get ()); per_bfd->index_table = std::move (map); + remove_all_units.disable (); return true; } diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index c37da5949b0..7bd0850ee5b 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -3679,6 +3679,10 @@ read_comp_units_from_section (dwarf2_per_objfile *per_objfile, void finalize_all_units (dwarf2_per_bfd *per_bfd) { + /* Sanity check. */ + gdb_assert (per_bfd->all_units.size () + == per_bfd->num_comp_units + per_bfd->num_type_units); + /* Ensure that the all_units vector is in the expected order for dwarf2_find_containing_unit to be able to perform a binary search. */ std::sort (per_bfd->all_units.begin (), per_bfd->all_units.end (), @@ -3694,6 +3698,7 @@ void create_all_units (dwarf2_per_objfile *per_objfile) { gdb_assert (per_objfile->per_bfd->all_units.empty ()); + scoped_remove_all_units remove_all_units (*per_objfile->per_bfd); signatured_type_set sig_types; @@ -3714,8 +3719,6 @@ create_all_units (dwarf2_per_objfile *per_objfile) if (!dwz->types.empty ()) { - per_objfile->per_bfd->all_units.clear (); - /* See enhancement PR symtab/30838. */ error (_(DWARF_ERROR_PREFIX ".debug_types section not supported in dwz file")); @@ -3725,6 +3728,7 @@ create_all_units (dwarf2_per_objfile *per_objfile) per_objfile->per_bfd->signatured_types = std::move (sig_types); finalize_all_units (per_objfile->per_bfd); + remove_all_units.disable (); } /* Return the initial uleb128 in the die at INFO_PTR. */ diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h index 4e3f8d7d7bc..74ec4204cad 100644 --- a/gdb/dwarf2/read.h +++ b/gdb/dwarf2/read.h @@ -673,6 +673,36 @@ public: std::string captured_debug_dir; }; +/* Scoped object to remove all units from PER_BFD and clear other associated + fields in case of failure. */ + +struct scoped_remove_all_units +{ + explicit scoped_remove_all_units (dwarf2_per_bfd &per_bfd) + : m_per_bfd (&per_bfd) + {} + + DISABLE_COPY_AND_ASSIGN (scoped_remove_all_units); + + ~scoped_remove_all_units () + { + if (m_per_bfd == nullptr) + return; + + m_per_bfd->all_units.clear (); + m_per_bfd->num_comp_units = 0; + m_per_bfd->num_type_units = 0; + } + + /* Disable this object. Call this to keep the units of M_PER_BFD on the + success path. */ + void disable () { m_per_bfd = nullptr; } + +private: + /* This is nullptr if the object is disabled. */ + dwarf2_per_bfd *m_per_bfd; +}; + /* An iterator for all_units that is based on index. This approach makes it possible to iterate over all_units safely, when some caller in the loop may add new units. */