diff mbox series

[1/2] Pre-read .debug_aranges section

Message ID 20231014205755.996771-2-tom@tromey.com
State New
Headers
Series Two .debug_aranges changes |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Tom Tromey Oct. 14, 2023, 8:56 p.m. UTC 
  While working on background DWARF reading, I found a race case that I
tracked down to the handling of the .debug_aranges section.  Currently
the section data is only read in after the CUs have all been created.
However, there's no real reason to do this -- it seems fine to read it
a little earlier, when all the other necessary sections are read in.

This patch makes this change, and updates the
read_addrmap_from_aranges API to assert that the section is read in.

This patch slightly changes the read_addrmap_from_aranges API as well,
to reject an empty section.  This seems better to me than what the
current code does, which is try to read an empty section but then do
no work.

Regression tested on x86-64 Fedora 38.
---
 gdb/dwarf2/read-debug-names.c |  1 +
 gdb/dwarf2/read.c             | 11 ++++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Guinevere Larsen Oct. 26, 2023, 9:54 a.m. UTC | #1 
On 14/10/2023 22:56, Tom Tromey wrote:
> While working on background DWARF reading, I found a race case that I
> tracked down to the handling of the .debug_aranges section.  Currently
> the section data is only read in after the CUs have all been created.
> However, there's no real reason to do this -- it seems fine to read it
> a little earlier, when all the other necessary sections are read in.
>
> This patch makes this change, and updates the
> read_addrmap_from_aranges API to assert that the section is read in.

I tested this as well and see no issues either. My one comment is about 
describing the section as "read in".

It made it very confusing to read (I thought there was a word missing 
somewhere at first), and the description of the "readin" flag says:

     /* True if we have tried to read this section.  */

So I think it would be better to describe that assert as "assert that 
the section has been read" or some other way to make it explicit that 
"read" is the past tense.

With some small rewording around this bit, everything looks good to me, 
Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
Tom Tromey Oct. 29, 2023, 4:34 p.m. UTC | #2 
>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:

Guinevere>     /* True if we have tried to read this section.  */

Guinevere> So I think it would be better to describe that assert as "assert that
Guinevere> the section has been read" or some other way to make it explicit that
Guinevere> "read" is the past tense.

I changed it to

  /* Caller must ensure that the section has already been read.  */

Guinevere> With some small rewording around this bit, everything looks good to
Guinevere> me, Reviewed-By: Guinevere Larsen <blarsen@redhat.com>

I'm going to check these in now.

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/read-debug-names.c b/gdb/dwarf2/read-debug-names.c
index 2e5067efb3d..89f5df6df90 100644
--- a/gdb/dwarf2/read-debug-names.c
+++ b/gdb/dwarf2/read-debug-names.c
@@ -166,6 +166,7 @@  create_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
 
   addrmap_mutable mutable_map;
 
+  section->read (per_objfile->objfile);
   if (read_addrmap_from_aranges (per_objfile, section, &mutable_map))
     per_bfd->index_addrmap
       = new (&per_bfd->obstack) addrmap_fixed (&per_bfd->obstack,
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d4aec19d31d..13ac83395eb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1586,6 +1586,7 @@  dwarf2_per_bfd::map_info_sections (struct objfile *objfile)
   ranges.read (objfile);
   rnglists.read (objfile);
   addr.read (objfile);
+  debug_aranges.read (objfile);
 
   for (auto &section : types)
     section.read (objfile);
@@ -1843,6 +1844,11 @@  read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
 			   dwarf2_section_info *section,
 			   addrmap *mutable_map)
 {
+  /* Caller must ensure this is read in.  */
+  gdb_assert (section->readin);
+  if (section->empty ())
+    return false;
+
   struct objfile *objfile = per_objfile->objfile;
   bfd *abfd = objfile->obfd.get ();
   struct gdbarch *gdbarch = objfile->arch ();
@@ -1870,13 +1876,8 @@  read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
     }
 
   std::set<sect_offset> debug_info_offset_seen;
-
-  section->read (objfile);
-
   const bfd_endian dwarf5_byte_order = gdbarch_byte_order (gdbarch);
-
   const gdb_byte *addr = section->buffer;
-
   while (addr < section->buffer + section->size)
     {
       const gdb_byte *const entry_addr = addr;