[gdb/symtab] Redo "Fix assert in set_length"
Commit Message
Hi,
This reverts commit 1c04f72368c ("[gdb/symtab] Fix assert in set_length"), due
to a regression reported in PR29572, and implements a different fix for PR29453.
The fix is to not use the CU table in a .debug_names section to construct
all_comp_units, but instead use create_all_comp_units, and then verify the CU
table from .debug_names. This also fixes PR25969, so remove the KFAIL.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29572
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25969
Any comments?
Thanks,
- Tom
[gdb/symtab] Redo "Fix assert in set_length"
---
gdb/dwarf2/read.c | 194 ++++++++++-----------
gdb/dwarf2/read.h | 17 ++
gdb/testsuite/gdb.dwarf2/clang-debug-names.exp | 15 +-
.../gdb.dwarf2/debug-names-duplicate-cu.exp | 2 +-
.../gdb.dwarf2/debug-names-missing-cu.exp | 2 +-
.../gdb.dwarf2/debug-names-non-ascending-cu.exp | 2 +-
6 files changed, 113 insertions(+), 119 deletions(-)
Comments
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
Tom> The fix is to not use the CU table in a .debug_names section to construct
Tom> all_comp_units, but instead use create_all_comp_units, and then verify the CU
Tom> table from .debug_names. This also fixes PR25969, so remove the KFAIL.
Note that create_all_comp_units was apparently renamed, so this text
should be changed.
Part of the point of the indices is to avoid reading .debug_info until
needed -- so scanning it should not be necessary. I assume this change
would cause a performance drop.
Checking whether performance suffers would be interesting.
If it doesn't, I'd be more inclined to do this.
Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29572
In this one I see the comment:
This happens fine for CUs, but in this case it's a PU.
Does .debug_names even work with partial units? I thought there was
some hole in this area. Maybe it is just "dwz -m" that can't work with
.debug_names? I don't recall any more.
Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25969
I'm not sure I understand this one, but it says:
AFAIU, to fix this, we'll have to do a top-level scan of the CUs,
and build partial symbols for CUs are not contained in any index.
I think if the index is incomplete, and gdb can detect that, then it
should simply be discarded and a full scan done. Trying to use both
.debug_names and a partial cooked index is going to make everything more
complicated for, IMO, very little gain.
Tom
On 9/23/22 15:45, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Tom> The fix is to not use the CU table in a .debug_names section to construct
> Tom> all_comp_units, but instead use create_all_comp_units, and then verify the CU
> Tom> table from .debug_names. This also fixes PR25969, so remove the KFAIL.
>
> Note that create_all_comp_units was apparently renamed, so this text
> should be changed.
>
Ack.
> Part of the point of the indices is to avoid reading .debug_info until
> needed -- so scanning it should not be necessary.
Right. But the benefit of scanning the CU headers is a way to reduce
complexity of implementation. And if we do the same for .gdb_index, we
can drop the atomic on the length field.
> I assume this change
> would cause a performance drop.
>
> Checking whether performance suffers would be interesting.
> If it doesn't, I'd be more inclined to do this.
>
I did the following experiment: I build gdb with -O2 at current master.
I loaded a "usual suspect" using a cooked index:
...
$ time.sh gdb -q -batch -iex "set language c"
libxul.so-93.0-1.1.x86_64.debug
maxmem: 2817504
real: 4.21
user: 13.01
system: 0.83
...
Then using .debug_names:
...
$ time.sh gdb -q -batch -iex "set language c"
libxul.so-93.0-1.1.x86_64.debug-with-debug-names
maxmem: 267204
real: 0.86
user: 1.55
system: 0.13
...
Then I rebuild with the patch applied and tried again .debug_names:
...
$ time.sh gdb -q -batch -iex "set language c"
libxul.so-93.0-1.1.x86_64.debug-with-debug-names
maxmem: 373516
real: 0.89
user: 1.57
system: 0.13
...
Doesn't look like a noticeable performance drop to me.
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29572
>
> In this one I see the comment:
>
> This happens fine for CUs, but in this case it's a PU.
>
> Does .debug_names even work with partial units? I thought there was
> some hole in this area. Maybe it is just "dwz -m" that can't work with
> .debug_names? I don't recall any more.
>
Sorry, I don't recall such a problem.
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25969
>
> I'm not sure I understand this one, but it says:
>
> AFAIU, to fix this, we'll have to do a top-level scan of the CUs,
> and build partial symbols for CUs are not contained in any index.
>
> I think if the index is incomplete, and gdb can detect that, then it
> should simply be discarded and a full scan done. Trying to use both
> .debug_names and a partial cooked index is going to make everything more
> complicated for, IMO, very little gain.
I guess that's a reasonable approach for an incomplete per-module index.
But is it also for an incomplete per-CU index (that is, per-CU index is
present for some CUs, but missing for other CUs) ?
If so, I guess that's ok. But on openSUSE, the linked-in glibc object
always contain some dwarf, so these will then always add a CU without
per-CU index, causing incompleteness, and gdb will end up ignoring any
clang-added per-CU index (unless we build a -nostdlib exec).
Thanks,
- Tom
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
Tom> Hi,
Tom> This reverts commit 1c04f72368c ("[gdb/symtab] Fix assert in set_length"), due
Tom> to a regression reported in PR29572, and implements a different fix for PR29453.
Tom> The fix is to not use the CU table in a .debug_names section to construct
Tom> all_comp_units, but instead use create_all_comp_units, and then verify the CU
Tom> table from .debug_names. This also fixes PR25969, so remove the KFAIL.
Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29572
Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25969
Tom> Any comments?
I've come around on this patch and I think you should check it in.
My main concern was the time cost, but you showed that this isn't
important; and anyway gdb is probably going to map these sections and do
more stuff with them later anyhow.
So, I think you should check this in.
Approved-By: Tom Tromey <tom@tromey.com>
Tom
On 12/5/23 21:59, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Tom> Hi,
> Tom> This reverts commit 1c04f72368c ("[gdb/symtab] Fix assert in set_length"), due
> Tom> to a regression reported in PR29572, and implements a different fix for PR29453.
>
> Tom> The fix is to not use the CU table in a .debug_names section to construct
> Tom> all_comp_units, but instead use create_all_comp_units, and then verify the CU
> Tom> table from .debug_names. This also fixes PR25969, so remove the KFAIL.
>
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29572
> Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25969
>
> Tom> Any comments?
>
> I've come around on this patch and I think you should check it in.
>
> My main concern was the time cost, but you showed that this isn't
> important; and anyway gdb is probably going to map these sections and do
> more stuff with them later anyhow.
>
> So, I think you should check this in.
>
The patch no longer applied cleanly, so I've updated it.
Also I came across a finalize_all_units call in dwarf2_read_debug_names
that was no longer necessary, so I've removed that one.
Reposted here (
https://sourceware.org/pipermail/gdb-patches/2023-December/204835.html )
and committed.
Thanks,
- Tom
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom
@@ -1201,7 +1201,8 @@ static void prepare_one_comp_unit (struct dwarf2_cu *cu,
static struct type *set_die_type (struct die_info *, struct type *,
struct dwarf2_cu *, bool = false);
-static void create_all_units (dwarf2_per_objfile *per_objfile);
+static void create_all_units (dwarf2_per_objfile *per_objfile,
+ bool pre_read_p = true);
static void load_full_comp_unit (dwarf2_per_cu_data *per_cu,
dwarf2_per_objfile *per_objfile,
@@ -2216,52 +2217,46 @@ create_signatured_type_table_from_index
per_bfd->signatured_types = std::move (sig_types_hash);
}
-/* Create the signatured type hash table from .debug_names. */
+/* Check the signatured type hash table from .debug_names. */
-static void
-create_signatured_type_table_from_debug_names
+static bool
+check_signatured_type_table_from_debug_names
(dwarf2_per_objfile *per_objfile,
const mapped_debug_names &map,
- struct dwarf2_section_info *section,
- struct dwarf2_section_info *abbrev_section)
+ struct dwarf2_section_info *section)
{
struct objfile *objfile = per_objfile->objfile;
+ dwarf2_per_bfd *per_bfd = per_objfile->per_bfd;
+ int nr_cus = per_bfd->all_comp_units.size ();
+ int nr_cus_tus = per_bfd->all_units.size ();
section->read (objfile);
- abbrev_section->read (objfile);
-
- htab_up sig_types_hash = allocate_signatured_type_table ();
+ uint32_t j = nr_cus;
for (uint32_t i = 0; i < map.tu_count; ++i)
{
- signatured_type_up sig_type;
- void **slot;
-
sect_offset sect_off
= (sect_offset) (extract_unsigned_integer
(map.tu_table_reordered + i * map.offset_size,
map.offset_size,
map.dwarf5_byte_order));
- comp_unit_head cu_header;
- read_and_check_comp_unit_head (per_objfile, &cu_header, section,
- abbrev_section,
- section->buffer + to_underlying (sect_off),
- rcuh_kind::TYPE);
-
- sig_type = per_objfile->per_bfd->allocate_signatured_type
- (cu_header.signature);
- sig_type->type_offset_in_tu = cu_header.type_cu_offset_in_tu;
- sig_type->section = section;
- sig_type->sect_off = sect_off;
-
- slot = htab_find_slot (sig_types_hash.get (), sig_type.get (), INSERT);
- *slot = sig_type.get ();
-
- per_objfile->per_bfd->all_units.emplace_back (sig_type.release ());
+ bool found = false;
+ for (; j < nr_cus_tus; j++)
+ if (per_bfd->get_cu (j)->sect_off == sect_off)
+ {
+ found = true;
+ break;
+ }
+ if (!found)
+ {
+ warning (_("Section .debug_names has incorrect entry in TU table,"
+ " ignoring .debug_names."));
+ return false;
+ }
+ per_bfd->all_comp_units_index_tus.push_back (per_bfd->get_cu (j));
}
-
- per_objfile->per_bfd->signatured_types = std::move (sig_types_hash);
+ return true;
}
/* Read the address map data from the mapped index, and use it to
@@ -4603,17 +4598,20 @@ read_debug_names_from_section (struct objfile *objfile,
return true;
}
-/* A helper for create_cus_from_debug_names that handles the MAP's CU
+/* A helper for check_cus_from_debug_names that handles the MAP's CU
list. */
static bool
-create_cus_from_debug_names_list (dwarf2_per_bfd *per_bfd,
- const mapped_debug_names &map,
- dwarf2_section_info §ion,
- bool is_dwz)
+check_cus_from_debug_names_list (dwarf2_per_bfd *per_bfd,
+ const mapped_debug_names &map,
+ dwarf2_section_info §ion,
+ bool is_dwz)
{
+ int nr_cus = per_bfd->all_comp_units.size ();
+
if (!map.augmentation_is_gdb)
{
+ uint32_t j = 0;
for (uint32_t i = 0; i < map.cu_count; ++i)
{
sect_offset sect_off
@@ -4621,56 +4619,44 @@ create_cus_from_debug_names_list (dwarf2_per_bfd *per_bfd,
(map.cu_table_reordered + i * map.offset_size,
map.offset_size,
map.dwarf5_byte_order));
- /* We don't know the length of the CU, because the CU list in a
- .debug_names index can be incomplete, so we can't use the start
- of the next CU as end of this CU. We create the CUs here with
- length 0, and in cutu_reader::cutu_reader we'll fill in the
- actual length. */
- dwarf2_per_cu_data_up per_cu
- = create_cu_from_index_list (per_bfd, §ion, is_dwz,
- sect_off, 0);
- per_bfd->all_units.push_back (std::move (per_cu));
+ bool found = false;
+ for (; j < nr_cus; j++)
+ if (per_bfd->get_cu (j)->sect_off == sect_off)
+ {
+ found = true;
+ break;
+ }
+ if (!found)
+ {
+ warning (_("Section .debug_names has incorrect entry in CU table,"
+ " ignoring .debug_names."));
+ return false;
+ }
+ per_bfd->all_comp_units_index_cus.push_back (per_bfd->get_cu (j));
}
return true;
}
- sect_offset sect_off_prev;
- for (uint32_t i = 0; i <= map.cu_count; ++i)
+ if (map.cu_count != nr_cus)
{
- sect_offset sect_off_next;
- if (i < map.cu_count)
- {
- sect_off_next
- = (sect_offset) (extract_unsigned_integer
- (map.cu_table_reordered + i * map.offset_size,
- map.offset_size,
- map.dwarf5_byte_order));
- }
- else
- sect_off_next = (sect_offset) section.size;
- if (i >= 1)
+ warning (_("Section .debug_names has incorrect number of CUs in CU table,"
+ " ignoring .debug_names."));
+ return false;
+ }
+
+ for (uint32_t i = 0; i < map.cu_count; ++i)
+ {
+ sect_offset sect_off
+ = (sect_offset) (extract_unsigned_integer
+ (map.cu_table_reordered + i * map.offset_size,
+ map.offset_size,
+ map.dwarf5_byte_order));
+ if (sect_off != per_bfd->get_cu (i)->sect_off)
{
- if (sect_off_next == sect_off_prev)
- {
- warning (_("Section .debug_names has duplicate entry in CU table,"
- " ignoring .debug_names."));
- return false;
- }
- if (sect_off_next < sect_off_prev)
- {
- warning (_("Section .debug_names has non-ascending CU table,"
- " ignoring .debug_names."));
- return false;
- }
- /* Note: we're not using length = sect_off_next - sect_off_prev,
- to gracefully handle an incomplete CU list. */
- const ULONGEST length = 0;
- dwarf2_per_cu_data_up per_cu
- = create_cu_from_index_list (per_bfd, §ion, is_dwz,
- sect_off_prev, length);
- per_bfd->all_units.push_back (std::move (per_cu));
+ warning (_("Section .debug_names has incorrect entry in CU table,"
+ " ignoring .debug_names."));
+ return false;
}
- sect_off_prev = sect_off_next;
}
return true;
@@ -4680,23 +4666,20 @@ create_cus_from_debug_names_list (dwarf2_per_bfd *per_bfd,
the CU objects for this dwarf2_per_objfile. */
static bool
-create_cus_from_debug_names (dwarf2_per_bfd *per_bfd,
- const mapped_debug_names &map,
- const mapped_debug_names &dwz_map)
+check_cus_from_debug_names (dwarf2_per_bfd *per_bfd,
+ const mapped_debug_names &map,
+ const mapped_debug_names &dwz_map)
{
- gdb_assert (per_bfd->all_units.empty ());
- per_bfd->all_units.reserve (map.cu_count + dwz_map.cu_count);
-
- if (!create_cus_from_debug_names_list (per_bfd, map, per_bfd->info,
- false /* is_dwz */))
+ if (!check_cus_from_debug_names_list (per_bfd, map, per_bfd->info,
+ false /* is_dwz */))
return false;
if (dwz_map.cu_count == 0)
return true;
dwz_file *dwz = dwarf2_get_dwz_file (per_bfd);
- return create_cus_from_debug_names_list (per_bfd, dwz_map, dwz->info,
- true /* is_dwz */);
+ return check_cus_from_debug_names_list (per_bfd, dwz_map, dwz->info,
+ true /* is_dwz */);
}
/* Read .debug_names. If everything went ok, initialize the "quick"
@@ -4733,7 +4716,8 @@ dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
}
}
- if (!create_cus_from_debug_names (per_bfd, *map, dwz_map))
+ create_all_units (per_objfile, false);
+ if (!check_cus_from_debug_names (per_bfd, *map, dwz_map))
{
per_bfd->all_units.clear ();
return false;
@@ -4754,8 +4738,12 @@ dwarf2_read_debug_names (dwarf2_per_objfile *per_objfile)
? &per_bfd->types[0]
: &per_bfd->info);
- create_signatured_type_table_from_debug_names
- (per_objfile, *map, section, &per_bfd->abbrev);
+ if (!check_signatured_type_table_from_debug_names
+ (per_objfile, *map, section))
+ {
+ per_bfd->all_units.clear ();
+ return false;
+ }
}
finalize_all_units (per_bfd);
@@ -5032,7 +5020,7 @@ dw2_debug_names_iterator::next ()
continue;
}
}
- per_cu = per_bfd->get_cu (ull);
+ per_cu = per_bfd->get_index_cu (ull);
break;
case DW_IDX_type_unit:
/* Don't crash on bad data. */
@@ -5045,15 +5033,14 @@ dw2_debug_names_iterator::next ()
continue;
}
{
- int nr_cus = per_bfd->all_comp_units.size ();
- per_cu = per_bfd->get_cu (nr_cus + ull);
+ per_cu = per_bfd->get_index_tu (ull);
}
break;
case DW_IDX_die_offset:
/* In a per-CU index (as opposed to a per-module index), index
entries without CU attribute implicitly refer to the single CU. */
if (per_cu == NULL)
- per_cu = per_bfd->get_cu (0);
+ per_cu = per_bfd->get_index_cu (0);
break;
case DW_IDX_GNU_internal:
if (!m_map.augmentation_is_gdb)
@@ -7278,7 +7265,7 @@ finalize_all_units (dwarf2_per_bfd *per_bfd)
This is only done for -readnow and building partial symtabs. */
static void
-create_all_units (dwarf2_per_objfile *per_objfile)
+create_all_units (dwarf2_per_objfile *per_objfile, bool pre_read_p)
{
htab_up types_htab;
gdb_assert (per_objfile->per_bfd->all_units.empty ());
@@ -7294,12 +7281,15 @@ create_all_units (dwarf2_per_objfile *per_objfile)
dwz_file *dwz = dwarf2_get_dwz_file (per_objfile->per_bfd);
if (dwz != NULL)
{
- /* Pre-read the sections we'll need to construct an index. */
- struct objfile *objfile = per_objfile->objfile;
- dwz->abbrev.read (objfile);
- dwz->info.read (objfile);
- dwz->str.read (objfile);
- dwz->line.read (objfile);
+ if (pre_read_p)
+ {
+ /* Pre-read the sections we'll need to construct an index. */
+ struct objfile *objfile = per_objfile->objfile;
+ dwz->abbrev.read (objfile);
+ dwz->info.read (objfile);
+ dwz->str.read (objfile);
+ dwz->line.read (objfile);
+ }
read_comp_units_from_section (per_objfile, &dwz->info, &dwz->abbrev, 1,
types_htab, rcuh_kind::COMPILE);
}
@@ -436,6 +436,20 @@ struct dwarf2_per_bfd
return this->all_units[index].get ();
}
+ /* Return the CU given its index in the CU table in the index. */
+ dwarf2_per_cu_data *get_index_cu (int index) const
+ {
+ if (this->all_comp_units_index_cus.empty ())
+ return get_cu (index);
+
+ return this->all_comp_units_index_cus[index];
+ }
+
+ dwarf2_per_cu_data *get_index_tu (int index) const
+ {
+ return this->all_comp_units_index_tus[index];
+ }
+
/* A convenience function to allocate a dwarf2_per_cu_data. The
returned object has its "index" field set properly. The object
is allocated on the dwarf2_per_bfd obstack. */
@@ -496,6 +510,9 @@ struct dwarf2_per_bfd
gdb::array_view<dwarf2_per_cu_data_up> all_comp_units;
gdb::array_view<dwarf2_per_cu_data_up> all_type_units;
+ std::vector<dwarf2_per_cu_data*> all_comp_units_index_cus;
+ std::vector<dwarf2_per_cu_data*> all_comp_units_index_tus;
+
/* Table of struct type_unit_group objects.
The hash key is the DW_AT_stmt_list value. */
htab_up type_unit_groups;
@@ -34,20 +34,7 @@ if { [prepare_for_testing "failed to prepare" ${testfile} \
}
set test "no file command warnings"
-if { [regexp "warning: " $gdb_file_cmd_msg] } {
- set kfail_re \
- [concat \
- "warning: Section .debug_aranges in \[^\r\n\]* entry" \
- "at offset 0 debug_info_offset 0 does not exists," \
- "ignoring \\.debug_aranges\\."]
- if { [regexp $kfail_re $gdb_file_cmd_msg] } {
- kfail symtab/25969 $test
- } else {
- fail $test
- }
-} else {
- pass $test
-}
+gdb_assert { [regexp "warning: " $gdb_file_cmd_msg] == 0 } $test
set cmd "ptype main"
set pass_re \
@@ -68,7 +68,7 @@ if [prepare_for_testing "failed to prepare" $testfile "${asm_file} ${srcfile}" \
set re \
[list \
"warning:" \
- "Section .debug_names has duplicate entry in CU table," \
+ "Section .debug_names has incorrect number of CUs in CU table," \
"ignoring .debug_names."]
set re [join $re]
gdb_assert {[regexp $re $gdb_file_cmd_msg]} "warning"
@@ -69,7 +69,7 @@ if [prepare_for_testing "failed to prepare" $testfile "${asm_file} ${srcfile}" \
# Verify that .debug_names section is not ignored.
set index [have_index $binfile]
-gdb_assert { [string equal $index "debug_names"] } ".debug_names used"
+gdb_assert { [string equal $index ""] } ".debug_names not used"
# Verify that initially no symtab is expanded.
gdb_test_no_output "maint info symtabs"
@@ -73,7 +73,7 @@ if [prepare_for_testing "failed to prepare" $testfile "${asm_file} ${srcfile}" \
set re \
[list \
"warning:" \
- "Section .debug_names has non-ascending CU table," \
+ "Section .debug_names has incorrect entry in CU table," \
"ignoring .debug_names."]
set re [join $re]
gdb_assert {[regexp $re $gdb_file_cmd_msg]} "warning"