From: Tom Tromey <tom@tromey.com>
To: gdb-patches@sourceware.org
Subject: [PATCH 5/7] Fix race in background DWARF indexer
Date: Sat, 17 Feb 2024 18:10:06 -0700 [thread overview]
Message-ID: <20240217-dwarf-race-relocate-v1-5-d3d2d908c1e8@tromey.com> (raw)
In-Reply-To: <20240217-dwarf-race-relocate-v1-0-d3d2d908c1e8@tromey.com>
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 --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. */
--
2.43.0
next prev parent reply other threads:[~2024-02-18 1:10 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-18 1:10 [PATCH 0/7] Fix race in DWARF reader Tom Tromey
2024-02-18 1:10 ` [PATCH 1/7] Compare section index in lookup_minimal_symbol_by_pc_section Tom Tromey
2024-02-18 1:10 ` [PATCH 2/7] Remove unnecessary null check " Tom Tromey
2024-02-18 1:10 ` [PATCH 3/7] Hoist a call to frob_address Tom Tromey
2024-02-18 1:10 ` [PATCH 4/7] Add unrelocated overload of lookup_minimal_symbol_by_pc_section Tom Tromey
2024-02-18 1:10 ` Tom Tromey [this message]
2024-02-18 1:10 ` [PATCH 6/7] Use bound_minimal_symbol more in lookup_minimal_symbol_by_pc_section Tom Tromey
2024-02-18 1:10 ` [PATCH 7/7] Fix address comparison " Tom Tromey
2024-04-03 15:16 ` [PATCH 0/7] Fix race in DWARF reader Tom Tromey
2024-04-09 18:16 ` John Baldwin
2024-04-09 22:11 ` Luis Machado
2024-04-10 0:02 ` Maciej W. Rozycki
2024-04-16 17:05 ` Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240217-dwarf-race-relocate-v1-5-d3d2d908c1e8@tromey.com \
--to=tom@tromey.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).