diff mbox series

[5/7] Fix race in background DWARF indexer

Message ID 20240217-dwarf-race-relocate-v1-5-d3d2d908c1e8@tromey.com
State New
Headers
Series Fix race in DWARF reader |

Checks

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

Commit Message

Tom Tromey Feb. 18, 2024, 1:10 a.m. UTC 
  PR gdb/31261 points out a race in the background DWARF indexer.
Looking into it, the problem is in dwarf2_per_objfile::adjust, which
does:

  CORE_ADDR baseaddr = objfile->text_section_offset ();
  CORE_ADDR tem = (CORE_ADDR) addr + baseaddr;
  tem = gdbarch_adjust_dwarf2_addr (objfile->arch (), tem);
  return (unrelocated_addr) (tem - baseaddr);

This code looks innocuous (or at least, it did to me), but if indexing
is still happening when the objfile is relocated, this causes a data
race when accessing the section offsets.

I considered a couple of fixes for this.  A simple one is to have
relocation wait until indexing is done.  However, it is better to
avoid any blocking like this; and I figured there is no reason for the
DWARF reader to require this information... famous last words.

Adjustment is needed to work around a sort of deficiency in the MIPS
target, where whether a function uses the MIPS16 ABI can apparently
only be determined by examining the minimal symbols.  To handle this,
the patch uses the new lookup_minimal_symbol_by_pc_section overload
that works solely off the per-BFD object -- which only holds
unrelocated data and which is effectively read-only at the time of
DWARF indexing.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31261
---
 gdb/arch-utils.c          |  5 +++--
 gdb/arch-utils.h          |  3 ++-
 gdb/dwarf2/frame.c        | 13 ++++++++++---
 gdb/dwarf2/read.c         | 12 ++++++------
 gdb/gdbarch-gen.h         |  4 ++--
 gdb/gdbarch.c             |  6 +++---
 gdb/gdbarch_components.py |  4 ++--
 gdb/mips-tdep.c           | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 gdb/mips-tdep.h           |  2 ++
 9 files changed, 79 insertions(+), 19 deletions(-)
diff mbox series

Patch

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 1faa013c16f..a5cbd3f06d2 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -222,8 +222,9 @@  default_make_symbol_special (struct symbol *sym, struct objfile *objfile)
 
 /* See arch-utils.h.  */
 
-CORE_ADDR
-default_adjust_dwarf2_addr (CORE_ADDR pc)
+unrelocated_addr
+default_adjust_dwarf2_addr (objfile_per_bfd_storage *per_bfd,
+			    unrelocated_addr pc)
 {
   return pc;
 }
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 0f37aaf20f8..21cdade77b9 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -105,7 +105,8 @@  void default_make_symbol_special (struct symbol *sym, struct objfile *objfile);
 
 /* Do nothing default implementation of gdbarch_adjust_dwarf2_addr.  */
 
-CORE_ADDR default_adjust_dwarf2_addr (CORE_ADDR pc);
+unrelocated_addr default_adjust_dwarf2_addr (objfile_per_bfd_storage *per_bfd,
+					     unrelocated_addr pc);
 
 /* Do nothing default implementation of gdbarch_adjust_dwarf2_line.  */
 
diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
index fc6704f434e..738906a77b9 100644
--- a/gdb/dwarf2/frame.c
+++ b/gdb/dwarf2/frame.c
@@ -145,13 +145,17 @@  typedef std::vector<dwarf2_fde *> dwarf2_fde_table;
 struct comp_unit
 {
   comp_unit (struct objfile *objf)
-    : abfd (objf->obfd.get ())
+    : abfd (objf->obfd.get ()),
+      per_bfd (objf->per_bfd)
   {
   }
 
   /* Keep the bfd convenient.  */
   bfd *abfd;
 
+  /* Also the per-bfd.  */
+  objfile_per_bfd_storage *per_bfd;
+
   /* Pointer to the .debug_frame section loaded into memory.  */
   const gdb_byte *dwarf_frame_buffer = nullptr;
 
@@ -1965,14 +1969,17 @@  decode_frame_entry_1 (struct gdbarch *gdbarch,
 	= read_encoded_value (unit, fde->cie->encoding, fde->cie->ptr_size,
 			      buf, &bytes_read, (unrelocated_addr) 0);
       fde->initial_location
-	= (unrelocated_addr) gdbarch_adjust_dwarf2_addr (gdbarch, init_addr);
+	= gdbarch_adjust_dwarf2_addr (gdbarch, unit->per_bfd,
+				      (unrelocated_addr) init_addr);
       buf += bytes_read;
 
       ULONGEST range
 	= read_encoded_value (unit, fde->cie->encoding & 0x0f,
 			      fde->cie->ptr_size, buf, &bytes_read,
 			      (unrelocated_addr) 0);
-      ULONGEST addr = gdbarch_adjust_dwarf2_addr (gdbarch, init_addr + range);
+      ULONGEST addr
+	= (ULONGEST) gdbarch_adjust_dwarf2_addr (gdbarch, unit->per_bfd,
+						 (unrelocated_addr) (init_addr + range));
       fde->address_range = addr - (ULONGEST) fde->initial_location;
       buf += bytes_read;
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 486be7e4921..27feae20508 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1211,10 +1211,8 @@  dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
 unrelocated_addr
 dwarf2_per_objfile::adjust (unrelocated_addr addr)
 {
-  CORE_ADDR baseaddr = objfile->text_section_offset ();
-  CORE_ADDR tem = (CORE_ADDR) addr + baseaddr;
-  tem = gdbarch_adjust_dwarf2_addr (objfile->arch (), tem);
-  return (unrelocated_addr) (tem - baseaddr);
+  return gdbarch_adjust_dwarf2_addr (objfile->arch (), objfile->per_bfd,
+				     addr);
 }
 
 /* See read.h.  */
@@ -1223,8 +1221,10 @@  CORE_ADDR
 dwarf2_per_objfile::relocate (unrelocated_addr addr)
 {
   CORE_ADDR baseaddr = objfile->text_section_offset ();
-  CORE_ADDR tem = (CORE_ADDR) addr + baseaddr;
-  return gdbarch_adjust_dwarf2_addr (objfile->arch (), tem);
+  unrelocated_addr adj = gdbarch_adjust_dwarf2_addr (objfile->arch (),
+						     objfile->per_bfd,
+						     addr);
+  return (CORE_ADDR) adj + baseaddr;
 }
 
 /* Hash function for line_header_hash.  */
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index 7a57bdcebe2..911547e8ef7 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -862,8 +862,8 @@  extern void set_gdbarch_make_symbol_special (struct gdbarch *gdbarch, gdbarch_ma
    code have the ISA bit set, matching line information and the symbol
    table. */
 
-typedef CORE_ADDR (gdbarch_adjust_dwarf2_addr_ftype) (CORE_ADDR pc);
-extern CORE_ADDR gdbarch_adjust_dwarf2_addr (struct gdbarch *gdbarch, CORE_ADDR pc);
+typedef unrelocated_addr (gdbarch_adjust_dwarf2_addr_ftype) (objfile_per_bfd_storage *per_bfd, unrelocated_addr pc);
+extern unrelocated_addr gdbarch_adjust_dwarf2_addr (struct gdbarch *gdbarch, objfile_per_bfd_storage *per_bfd, unrelocated_addr pc);
 extern void set_gdbarch_adjust_dwarf2_addr (struct gdbarch *gdbarch, gdbarch_adjust_dwarf2_addr_ftype *adjust_dwarf2_addr);
 
 /* Adjust the address updated by a line entry in a backend-specific way.
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index b9be3948d1e..34cd64edae1 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -3541,14 +3541,14 @@  set_gdbarch_make_symbol_special (struct gdbarch *gdbarch,
   gdbarch->make_symbol_special = make_symbol_special;
 }
 
-CORE_ADDR
-gdbarch_adjust_dwarf2_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
+unrelocated_addr
+gdbarch_adjust_dwarf2_addr (struct gdbarch *gdbarch, objfile_per_bfd_storage *per_bfd, unrelocated_addr pc)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->adjust_dwarf2_addr != NULL);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_adjust_dwarf2_addr called\n");
-  return gdbarch->adjust_dwarf2_addr (pc);
+  return gdbarch->adjust_dwarf2_addr (per_bfd, pc);
 }
 
 void
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index d76b820c1b5..2a5d1e544e4 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1497,9 +1497,9 @@  sure addresses in FDE, range records, etc. referring to compressed
 code have the ISA bit set, matching line information and the symbol
 table.
 """,
-    type="CORE_ADDR",
+    type="unrelocated_addr",
     name="adjust_dwarf2_addr",
-    params=[("CORE_ADDR", "pc")],
+    params=[("objfile_per_bfd_storage *", "per_bfd"), ("unrelocated_addr", "pc")],
     predefault="default_adjust_dwarf2_addr",
     invalid=False,
 )
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index bf0b66c4b00..8364cb0d009 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -356,6 +356,12 @@  is_compact_addr (CORE_ADDR addr)
   return ((addr) & 1);
 }
 
+static int
+is_compact_addr (unrelocated_addr addr)
+{
+  return ((CORE_ADDR) addr & 1);
+}
+
 /* Return one iff ADDR denotes standard ISA code.  */
 
 static int
@@ -364,6 +370,12 @@  is_mips_addr (CORE_ADDR addr)
   return !is_compact_addr (addr);
 }
 
+static int
+is_mips_addr (unrelocated_addr addr)
+{
+  return !is_compact_addr (addr);
+}
+
 /* Return one iff ADDR denotes MIPS16 code.  */
 
 static int
@@ -388,6 +400,12 @@  unmake_compact_addr (CORE_ADDR addr)
   return ((addr) & ~(CORE_ADDR) 1);
 }
 
+static unrelocated_addr
+unmake_compact_addr (unrelocated_addr addr)
+{
+  return (unrelocated_addr) (to_underlying (addr) & ~(CORE_ADDR) 1);
+}
+
 /* Add the ISA (compression) bit to ADDR.  */
 
 static CORE_ADDR
@@ -396,6 +414,14 @@  make_compact_addr (CORE_ADDR addr)
   return ((addr) | (CORE_ADDR) 1);
 }
 
+/* Add the ISA (compression) bit to ADDR.  */
+
+static unrelocated_addr
+make_compact_addr (unrelocated_addr addr)
+{
+  return (unrelocated_addr) (to_underlying (addr) | (CORE_ADDR) 1);
+}
+
 /* Extern version of unmake_compact_addr; we use a separate function
    so that unmake_compact_addr can be inlined throughout this file.  */
 
@@ -1225,6 +1251,21 @@  mips_pc_is_mips (CORE_ADDR memaddr)
     return is_mips_addr (memaddr);
 }
 
+int
+mips_pc_is_mips (objfile_per_bfd_storage *per_bfd, unrelocated_addr memaddr)
+{
+  /* Flags indicating that this is a MIPS16 or microMIPS function is
+     stored by elfread.c in the high bit of the info field.  Use this
+     to decide if the function is standard MIPS.  Otherwise if bit 0
+     of the address is clear, then this is a standard MIPS function.  */
+  minimal_symbol *sym
+    = lookup_minimal_symbol_by_pc (per_bfd, make_compact_addr (memaddr));
+  if (sym != nullptr)
+    return msymbol_is_mips (sym);
+  else
+    return is_mips_addr (memaddr);
+}
+
 /* Tell if the program counter value in MEMADDR is in a MIPS16 function.  */
 
 int
@@ -1309,6 +1350,14 @@  mips_adjust_dwarf2_addr (CORE_ADDR pc)
   return mips_pc_is_mips (pc) ? pc : make_compact_addr (pc);
 }
 
+static unrelocated_addr
+mips_adjust_dwarf2_addr (objfile_per_bfd_storage *per_bfd,
+			 unrelocated_addr pc)
+{
+  pc = unmake_compact_addr (pc);
+  return mips_pc_is_mips (per_bfd, pc) ? pc : make_compact_addr (pc);
+}
+
 /* Recalculate the line record requested so that the resulting PC has
    the ISA bit set correctly, used by DWARF-2 machinery.  The need for
    this adjustment comes from some records associated with compressed
diff --git a/gdb/mips-tdep.h b/gdb/mips-tdep.h
index b64f37cebbb..9dd9bf16e7c 100644
--- a/gdb/mips-tdep.h
+++ b/gdb/mips-tdep.h
@@ -173,6 +173,8 @@  extern CORE_ADDR mips_unmake_compact_addr (CORE_ADDR addr);
 /* Tell if the program counter value in MEMADDR is in a standard
    MIPS function.  */
 extern int mips_pc_is_mips (CORE_ADDR memaddr);
+extern int mips_pc_is_mips (objfile_per_bfd_storage *per_bfd,
+			    unrelocated_addr memaddr);
 
 /* Tell if the program counter value in MEMADDR is in a MIPS16
    function.  */