[PATCH 0/7] Fix race in DWARF reader

[PATCH 0/7] Fix race in DWARF reader

[Tom Tromey]

The background DWARF reader turns out to have a few races.  This
series fixes one that occurs when the indexer runs at the same time as
DWARF relocation.

Most of the series is just cleanup / preparation.

The main patch affects MIPS16.  I can't test this -- I tried on a MIPS
machine in the GCC compile farm, but unfortunately the relevant
gdb.arch test says that the processor doesn't support MIPS16.  It's
possible this code is simply dead; I do not know.

Regression tested on x86-64 Fedora 38.

---
Tom Tromey (7):
      Compare section index in lookup_minimal_symbol_by_pc_section
      Remove unnecessary null check in lookup_minimal_symbol_by_pc_section
      Hoist a call to frob_address
      Add unrelocated overload of lookup_minimal_symbol_by_pc_section
      Fix race in background DWARF indexer
      Use bound_minimal_symbol more in lookup_minimal_symbol_by_pc_section
      Fix address comparison in lookup_minimal_symbol_by_pc_section

 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/minsyms.c             | 449 +++++++++++++++++++++++++---------------------
 gdb/minsyms.h             |   7 +
 gdb/mips-tdep.c           |  49 +++++
 gdb/mips-tdep.h           |   2 +
 11 files changed, 330 insertions(+), 224 deletions(-)
---
base-commit: 989aa9b8e8e340ba65f386cbfd239009a3aba68f
change-id: 20240217-dwarf-race-relocate-287780f3ac82

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH 1/7] Compare section index in lookup_minimal_symbol_by_pc_section

[Tom Tromey]

This changes lookup_minimal_symbol_by_pc_section to compare the
section index when comparing two symbols.  This is ok because the
symbols in question always come from the same objfile.  The motivation
for this is to simplify a refactoring in a subsequent patch; but it is
also more efficient.
---
 gdb/minsyms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 1b85424586f..ffded3781c4 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -901,8 +901,8 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 		      && (msymbol[hi].size () == msymbol[hi - 1].size ())
 		      && (msymbol[hi].unrelocated_address ()
 			  == msymbol[hi - 1].unrelocated_address ())
-		      && (msymbol[hi].obj_section (objfile)
-			  == msymbol[hi - 1].obj_section (objfile)))
+		      && (msymbol[hi].section_index ()
+			  == msymbol[hi - 1].section_index ()))
 		    {
 		      hi--;
 		      continue;

-- 
2.43.0

[PATCH 2/7] Remove unnecessary null check in lookup_minimal_symbol_by_pc_section

[Tom Tromey]

lookup_minimal_symbol_by_pc_section asserts that section != NULL early
in the function, rendering a later check (and comment) unnecessary.
---
 gdb/minsyms.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index ffded3781c4..f63d0f4af61 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -876,13 +876,11 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 		      continue;
 		    }
 
-		  /* If SECTION was specified, skip any symbol from
-		     wrong section.  */
-		  if (section
-		      /* Some types of debug info, such as COFF,
+		  /* Skip any symbol from wrong section.  */
+		  if (/* Some types of debug info, such as COFF,
 			 don't fill the bfd_section member, so don't
 			 throw away symbols on those platforms.  */
-		      && msymbol[hi].obj_section (objfile) != nullptr
+		      msymbol[hi].obj_section (objfile) != nullptr
 		      && (!matching_obj_sections
 			  (msymbol[hi].obj_section (objfile),
 			   section)))

-- 
2.43.0

[PATCH 3/7] Hoist a call to frob_address

[Tom Tromey]

This hoists the call to frob_address in
lookup_minimal_symbol_by_pc_section, so that it now appears earlier in
the loop.  This is useful to help with a refactoring in the next
patch.  Making this change also made it clear that 'pc' is not used,
so this removes the local variable as well.
---
 gdb/minsyms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index f63d0f4af61..36ca11a0b94 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -796,7 +796,9 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 
   for (objfile *objfile : section->objfile->separate_debug_objfiles ())
     {
-      CORE_ADDR pc = pc_in;
+      unrelocated_addr unrel_pc;
+      if (!frob_address (objfile, pc_in, &unrel_pc))
+	continue;
 
       /* If this objfile has a minimal symbol table, go search it
 	 using a binary search.  */
@@ -827,9 +829,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 
 	     Warning: this code is trickier than it would appear at first.  */
 
-	  unrelocated_addr unrel_pc;
-	  if (frob_address (objfile, pc, &unrel_pc)
-	      && unrel_pc >= msymbol[lo].unrelocated_address ())
+	  if (unrel_pc >= msymbol[lo].unrelocated_address ())
 	    {
 	      while (msymbol[hi].unrelocated_address () > unrel_pc)
 		{

-- 
2.43.0

[PATCH 4/7] Add unrelocated overload of lookup_minimal_symbol_by_pc_section

[Tom Tromey]

This refactors lookup_minimal_symbol_by_pc_section, pulling out the
inner loop into a separate function.  This function also does all its
work using only the per-BFD object -- not the objfile.

This overload is used in the next patch, but it seemed simpler to
review as its own patch.
---
 gdb/minsyms.c | 437 ++++++++++++++++++++++++++++++++--------------------------
 gdb/minsyms.h |   7 +
 2 files changed, 250 insertions(+), 194 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 36ca11a0b94..2c8bf2a9f0b 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -747,6 +747,214 @@ msym_prefer_to_msym_type (lookup_msym_prefer prefer)
   gdb_assert_not_reached ("unhandled lookup_msym_prefer");
 }
 
+/* Worker function for the various public overloads.  Looks in PER_BFD
+   for a symbol at UNREL_PC.  PREVIOUS is an out parameter that is set
+   to the closest symbol appearing before UNREL_PC.  WANT_TYPE
+   indicates which sections to look for.  WRONG_SECTION is a predicate
+   that is passed a symbol to decide if it is in the wrong section for
+   some reason other than not matching WANT_TYPE.  */
+
+static minimal_symbol *
+lookup_minimal_symbol_by_pc
+     (objfile_per_bfd_storage *per_bfd,
+      unrelocated_addr unrel_pc,
+      minimal_symbol **previous,
+      minimal_symbol_type want_type,
+      gdb::function_view<bool (minimal_symbol *)> wrong_section)
+{
+  /* If there is a minimal symbol table, go search it using a binary
+     search.  */
+  if (per_bfd->minimal_symbol_count == 0)
+    return nullptr;
+
+  int best_zero_sized = -1;
+  minimal_symbol *best_symbol = nullptr;
+
+  minimal_symbol *msymbol = per_bfd->msymbols.get ();
+  int lo = 0;
+  int hi = per_bfd->minimal_symbol_count - 1;
+
+  /* This code assumes that the minimal symbols are sorted by
+     ascending address values.  If the pc value is greater than or
+     equal to the first symbol's address, then some symbol in this
+     minimal symbol table is a suitable candidate for being the
+     "best" symbol.  This includes the last real symbol, for cases
+     where the pc value is larger than any address in this vector.
+
+     By iterating until the address associated with the current
+     hi index (the endpoint of the test interval) is less than
+     or equal to the desired pc value, we accomplish two things:
+     (1) the case where the pc value is larger than any minimal
+     symbol address is trivially solved, (2) the address associated
+     with the hi index is always the one we want when the iteration
+     terminates.  In essence, we are iterating the test interval
+     down until the pc value is pushed out of it from the high end.
+
+     Warning: this code is trickier than it would appear at first.  */
+
+  if (unrel_pc >= msymbol[lo].unrelocated_address ())
+    {
+      while (msymbol[hi].unrelocated_address () > unrel_pc)
+	{
+	  /* pc is still strictly less than highest address.  */
+	  /* Note "new" will always be >= lo.  */
+	  int newobj = (lo + hi) / 2;
+	  if ((msymbol[newobj].unrelocated_address () >= unrel_pc)
+	      || (lo == newobj))
+	    {
+	      hi = newobj;
+	    }
+	  else
+	    {
+	      lo = newobj;
+	    }
+	}
+
+      /* If we have multiple symbols at the same address, we want
+	 hi to point to the last one.  That way we can find the
+	 right symbol if it has an index greater than hi.  */
+      while (hi < per_bfd->minimal_symbol_count - 1
+	     && (msymbol[hi].unrelocated_address ()
+		 == msymbol[hi + 1].unrelocated_address ()))
+	hi++;
+
+      /* Skip various undesirable symbols.  */
+      while (hi >= 0)
+	{
+	  /* Skip any absolute symbols.  This is apparently
+	     what adb and dbx do, and is needed for the CM-5.
+	     There are two known possible problems: (1) on
+	     ELF, apparently end, edata, etc. are absolute.
+	     Not sure ignoring them here is a big deal, but if
+	     we want to use them, the fix would go in
+	     elfread.c.  (2) I think shared library entry
+	     points on the NeXT are absolute.  If we want
+	     special handling for this it probably should be
+	     triggered by a special mst_abs_or_lib or some
+	     such.  */
+
+	  if (msymbol[hi].type () == mst_abs)
+	    {
+	      hi--;
+	      continue;
+	    }
+
+	  /* Skip any symbol from the wrong section.  */
+	  if (wrong_section (&msymbol[hi]))
+	    {
+	      hi--;
+	      continue;
+	    }
+
+	  /* If we are looking for a trampoline and this is a
+	     text symbol, or the other way around, check the
+	     preceding symbol too.  If they are otherwise
+	     identical prefer that one.  */
+	  if (hi > 0
+	      && msymbol[hi].type () != want_type
+	      && msymbol[hi - 1].type () == want_type
+	      && (msymbol[hi].size () == msymbol[hi - 1].size ())
+	      && (msymbol[hi].unrelocated_address ()
+		  == msymbol[hi - 1].unrelocated_address ())
+	      && (msymbol[hi].section_index ()
+		  == msymbol[hi - 1].section_index ()))
+	    {
+	      hi--;
+	      continue;
+	    }
+
+	  /* If the minimal symbol has a zero size, save it
+	     but keep scanning backwards looking for one with
+	     a non-zero size.  A zero size may mean that the
+	     symbol isn't an object or function (e.g. a
+	     label), or it may just mean that the size was not
+	     specified.  */
+	  if (msymbol[hi].size () == 0)
+	    {
+	      if (best_zero_sized == -1)
+		best_zero_sized = hi;
+	      hi--;
+	      continue;
+	    }
+
+	  /* If we are past the end of the current symbol, try
+	     the previous symbol if it has a larger overlapping
+	     size.  This happens on i686-pc-linux-gnu with glibc;
+	     the nocancel variants of system calls are inside
+	     the cancellable variants, but both have sizes.  */
+	  if (hi > 0
+	      && msymbol[hi].size () != 0
+	      && unrel_pc >= msymbol[hi].unrelocated_end_address ()
+	      && unrel_pc < msymbol[hi - 1].unrelocated_end_address ())
+	    {
+	      hi--;
+	      continue;
+	    }
+
+	  /* Otherwise, this symbol must be as good as we're going
+	     to get.  */
+	  break;
+	}
+
+      /* If HI has a zero size, and best_zero_sized is set,
+	 then we had two or more zero-sized symbols; prefer
+	 the first one we found (which may have a higher
+	 address).  Also, if we ran off the end, be sure
+	 to back up.  */
+      if (best_zero_sized != -1
+	  && (hi < 0 || msymbol[hi].size () == 0))
+	hi = best_zero_sized;
+
+      /* If the minimal symbol has a non-zero size, and this
+	 PC appears to be outside the symbol's contents, then
+	 refuse to use this symbol.  If we found a zero-sized
+	 symbol with an address greater than this symbol's,
+	 use that instead.  We assume that if symbols have
+	 specified sizes, they do not overlap.  */
+
+      if (hi >= 0
+	  && msymbol[hi].size () != 0
+	  && unrel_pc >= msymbol[hi].unrelocated_end_address ())
+	{
+	  if (best_zero_sized != -1)
+	    hi = best_zero_sized;
+	  else
+	    {
+	      /* If needed record this symbol as the closest
+		 previous symbol.  */
+	      if (*previous == nullptr
+		  || (msymbol[hi].unrelocated_address ()
+		      > (*previous)->unrelocated_address ()))
+		*previous = &msymbol[hi];
+	    }
+	  /* Done.  */
+	  return best_symbol;
+	}
+
+      if (hi >= 0
+	  && (best_symbol == nullptr
+	      || (best_symbol->unrelocated_address ()
+		  < msymbol[hi].unrelocated_address ())))
+	best_symbol = &msymbol[hi];
+    }
+
+  return best_symbol;
+}
+
+/* See minsyms.h.  */
+
+minimal_symbol *
+lookup_minimal_symbol_by_pc (objfile_per_bfd_storage *per_bfd,
+			     unrelocated_addr addr)
+{
+  minimal_symbol *ignore = nullptr;
+  return lookup_minimal_symbol_by_pc (per_bfd, addr, &ignore, mst_text,
+				      [] (minimal_symbol *msym)
+    {
+      return msym->type () != mst_text;
+    });
+}
+
 /* See minsyms.h.
 
    Note that we need to look through ALL the minimal symbol tables
@@ -760,10 +968,6 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 				     lookup_msym_prefer prefer,
 				     bound_minimal_symbol *previous)
 {
-  int lo;
-  int hi;
-  int newobj;
-  struct minimal_symbol *msymbol;
   struct minimal_symbol *best_symbol = NULL;
   struct objfile *best_objfile = NULL;
   struct bound_minimal_symbol result;
@@ -800,199 +1004,44 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
       if (!frob_address (objfile, pc_in, &unrel_pc))
 	continue;
 
-      /* If this objfile has a minimal symbol table, go search it
-	 using a binary search.  */
-
-      if (objfile->per_bfd->minimal_symbol_count > 0)
+      minimal_symbol *this_previous = nullptr;
+      minimal_symbol *minsym
+	= lookup_minimal_symbol_by_pc (objfile->per_bfd, unrel_pc,
+				       &this_previous, want_type,
+				       [&] (minimal_symbol *check_sym)
+	{
+	  /* Some types of debug info, such as COFF, don't fill the
+	     bfd_section member, so don't throw away symbols on those
+	     platforms.  */
+	  return (check_sym->obj_section (objfile) != nullptr
+		  && !matching_obj_sections (check_sym->obj_section (objfile),
+					     section));
+	});
+
+      /* If needed record this symbol as the closest previous
+	 symbol.  */
+      if (previous != nullptr)
 	{
-	  int best_zero_sized = -1;
-
-	  msymbol = objfile->per_bfd->msymbols.get ();
-	  lo = 0;
-	  hi = objfile->per_bfd->minimal_symbol_count - 1;
-
-	  /* This code assumes that the minimal symbols are sorted by
-	     ascending address values.  If the pc value is greater than or
-	     equal to the first symbol's address, then some symbol in this
-	     minimal symbol table is a suitable candidate for being the
-	     "best" symbol.  This includes the last real symbol, for cases
-	     where the pc value is larger than any address in this vector.
-
-	     By iterating until the address associated with the current
-	     hi index (the endpoint of the test interval) is less than
-	     or equal to the desired pc value, we accomplish two things:
-	     (1) the case where the pc value is larger than any minimal
-	     symbol address is trivially solved, (2) the address associated
-	     with the hi index is always the one we want when the iteration
-	     terminates.  In essence, we are iterating the test interval
-	     down until the pc value is pushed out of it from the high end.
-
-	     Warning: this code is trickier than it would appear at first.  */
-
-	  if (unrel_pc >= msymbol[lo].unrelocated_address ())
+	  if (previous->minsym == nullptr
+	      || (this_previous->unrelocated_address ()
+		  > previous->minsym->unrelocated_address ()))
 	    {
-	      while (msymbol[hi].unrelocated_address () > unrel_pc)
-		{
-		  /* pc is still strictly less than highest address.  */
-		  /* Note "new" will always be >= lo.  */
-		  newobj = (lo + hi) / 2;
-		  if ((msymbol[newobj].unrelocated_address () >= unrel_pc)
-		      || (lo == newobj))
-		    {
-		      hi = newobj;
-		    }
-		  else
-		    {
-		      lo = newobj;
-		    }
-		}
-
-	      /* If we have multiple symbols at the same address, we want
-		 hi to point to the last one.  That way we can find the
-		 right symbol if it has an index greater than hi.  */
-	      while (hi < objfile->per_bfd->minimal_symbol_count - 1
-		     && (msymbol[hi].unrelocated_address ()
-			 == msymbol[hi + 1].unrelocated_address ()))
-		hi++;
-
-	      /* Skip various undesirable symbols.  */
-	      while (hi >= 0)
-		{
-		  /* Skip any absolute symbols.  This is apparently
-		     what adb and dbx do, and is needed for the CM-5.
-		     There are two known possible problems: (1) on
-		     ELF, apparently end, edata, etc. are absolute.
-		     Not sure ignoring them here is a big deal, but if
-		     we want to use them, the fix would go in
-		     elfread.c.  (2) I think shared library entry
-		     points on the NeXT are absolute.  If we want
-		     special handling for this it probably should be
-		     triggered by a special mst_abs_or_lib or some
-		     such.  */
-
-		  if (msymbol[hi].type () == mst_abs)
-		    {
-		      hi--;
-		      continue;
-		    }
-
-		  /* Skip any symbol from wrong section.  */
-		  if (/* Some types of debug info, such as COFF,
-			 don't fill the bfd_section member, so don't
-			 throw away symbols on those platforms.  */
-		      msymbol[hi].obj_section (objfile) != nullptr
-		      && (!matching_obj_sections
-			  (msymbol[hi].obj_section (objfile),
-			   section)))
-		    {
-		      hi--;
-		      continue;
-		    }
-
-		  /* If we are looking for a trampoline and this is a
-		     text symbol, or the other way around, check the
-		     preceding symbol too.  If they are otherwise
-		     identical prefer that one.  */
-		  if (hi > 0
-		      && msymbol[hi].type () != want_type
-		      && msymbol[hi - 1].type () == want_type
-		      && (msymbol[hi].size () == msymbol[hi - 1].size ())
-		      && (msymbol[hi].unrelocated_address ()
-			  == msymbol[hi - 1].unrelocated_address ())
-		      && (msymbol[hi].section_index ()
-			  == msymbol[hi - 1].section_index ()))
-		    {
-		      hi--;
-		      continue;
-		    }
-
-		  /* If the minimal symbol has a zero size, save it
-		     but keep scanning backwards looking for one with
-		     a non-zero size.  A zero size may mean that the
-		     symbol isn't an object or function (e.g. a
-		     label), or it may just mean that the size was not
-		     specified.  */
-		  if (msymbol[hi].size () == 0)
-		    {
-		      if (best_zero_sized == -1)
-			best_zero_sized = hi;
-		      hi--;
-		      continue;
-		    }
-
-		  /* If we are past the end of the current symbol, try
-		     the previous symbol if it has a larger overlapping
-		     size.  This happens on i686-pc-linux-gnu with glibc;
-		     the nocancel variants of system calls are inside
-		     the cancellable variants, but both have sizes.  */
-		  if (hi > 0
-		      && msymbol[hi].size () != 0
-		      && unrel_pc >= msymbol[hi].unrelocated_end_address ()
-		      && unrel_pc < msymbol[hi - 1].unrelocated_end_address ())
-		    {
-		      hi--;
-		      continue;
-		    }
-
-		  /* Otherwise, this symbol must be as good as we're going
-		     to get.  */
-		  break;
-		}
-
-	      /* If HI has a zero size, and best_zero_sized is set,
-		 then we had two or more zero-sized symbols; prefer
-		 the first one we found (which may have a higher
-		 address).  Also, if we ran off the end, be sure
-		 to back up.  */
-	      if (best_zero_sized != -1
-		  && (hi < 0 || msymbol[hi].size () == 0))
-		hi = best_zero_sized;
-
-	      /* If the minimal symbol has a non-zero size, and this
-		 PC appears to be outside the symbol's contents, then
-		 refuse to use this symbol.  If we found a zero-sized
-		 symbol with an address greater than this symbol's,
-		 use that instead.  We assume that if symbols have
-		 specified sizes, they do not overlap.  */
-
-	      if (hi >= 0
-		  && msymbol[hi].size () != 0
-		  && unrel_pc >= msymbol[hi].unrelocated_end_address ())
-		{
-		  if (best_zero_sized != -1)
-		    hi = best_zero_sized;
-		  else
-		    {
-		      /* If needed record this symbol as the closest
-			 previous symbol.  */
-		      if (previous != nullptr)
-			{
-			  if (previous->minsym == nullptr
-			      || (msymbol[hi].unrelocated_address ()
-				  > previous->minsym->unrelocated_address ()))
-			    {
-			      previous->minsym = &msymbol[hi];
-			      previous->objfile = objfile;
-			    }
-			}
-		      /* Go on to the next object file.  */
-		      continue;
-		    }
-		}
-
-	      /* The minimal symbol indexed by hi now is the best one in this
-		 objfile's minimal symbol table.  See if it is the best one
-		 overall.  */
-
-	      if (hi >= 0
-		  && ((best_symbol == NULL) ||
-		      (best_symbol->unrelocated_address () <
-		       msymbol[hi].unrelocated_address ())))
-		{
-		  best_symbol = &msymbol[hi];
-		  best_objfile = objfile;
-		}
+	      previous->minsym = this_previous;
+	      previous->objfile = objfile;
 	    }
+	  /* Go on to the next object file.  */
+	  continue;
+	}
+
+      /* MINSYM now is the best one in this objfile's minimal symbol
+	 table.  See if it is the best one overall.  */
+      if (minsym != nullptr
+	  && ((best_symbol == NULL) ||
+	      (best_symbol->unrelocated_address ()
+	       < minsym->unrelocated_address ())))
+	{
+	  best_symbol = minsym;
+	  best_objfile = objfile;
 	}
     }
 
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index d44f281939b..df77977538f 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -301,6 +301,13 @@ struct bound_minimal_symbol lookup_minimal_symbol_by_pc_section
 
 struct bound_minimal_symbol lookup_minimal_symbol_by_pc (CORE_ADDR);
 
+/* Overload of lookup_minimal_symbol_by_pc that only references the
+   (unrelocated) per-BFD object and only searches the text
+   section.  */
+
+minimal_symbol *lookup_minimal_symbol_by_pc (objfile_per_bfd_storage *per_bfd,
+					     unrelocated_addr addr);
+
 /* Iterate over all the minimal symbols in the objfile OBJF which
    match NAME.  Both the ordinary and demangled names of each symbol
    are considered.  The caller is responsible for canonicalizing NAME,

-- 
2.43.0

[PATCH 5/7] Fix race in background DWARF indexer

[Tom Tromey]

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

[PATCH 6/7] Use bound_minimal_symbol more in lookup_minimal_symbol_by_pc_section

[Tom Tromey]

lookup_minimal_symbol_by_pc_section tracks the best symbol and
objfile.  This patch changes it to use a bound_minimal_symbol rather
than two separate variables.  This simplifies the code a little.
---
 gdb/minsyms.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 2c8bf2a9f0b..64f085a0314 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -968,9 +968,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 				     lookup_msym_prefer prefer,
 				     bound_minimal_symbol *previous)
 {
-  struct minimal_symbol *best_symbol = NULL;
-  struct objfile *best_objfile = NULL;
-  struct bound_minimal_symbol result;
+  struct bound_minimal_symbol best = {};
 
   if (previous != nullptr)
     {
@@ -1036,18 +1034,13 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
       /* MINSYM now is the best one in this objfile's minimal symbol
 	 table.  See if it is the best one overall.  */
       if (minsym != nullptr
-	  && ((best_symbol == NULL) ||
-	      (best_symbol->unrelocated_address ()
-	       < minsym->unrelocated_address ())))
-	{
-	  best_symbol = minsym;
-	  best_objfile = objfile;
-	}
+	  && (best.minsym == nullptr
+	      || (best.minsym->unrelocated_address ()
+		  < minsym->unrelocated_address ())))
+	best = { minsym, objfile };
     }
 
-  result.minsym = best_symbol;
-  result.objfile = best_objfile;
-  return result;
+  return best;
 }
 
 /* See minsyms.h.  */

-- 
2.43.0

[PATCH 7/7] Fix address comparison in lookup_minimal_symbol_by_pc_section

[Tom Tromey]

I noticed that lookup_minimal_symbol_by_pc_section is comparing
unrelocated addresses across objfiles when trying to find the best
symbol.  However, this seems incorrect to me, as the unrelocated
addresses aren't of interest here.  This patch changes this code to
compare the true addresses instead.

This does make me wonder whether this code is necessary at all.
---
 gdb/minsyms.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 64f085a0314..06a87865c13 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1035,8 +1035,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 	 table.  See if it is the best one overall.  */
       if (minsym != nullptr
 	  && (best.minsym == nullptr
-	      || (best.minsym->unrelocated_address ()
-		  < minsym->unrelocated_address ())))
+	      || best.value_address () < minsym->value_address (objfile)))
 	best = { minsym, objfile };
     }
 

-- 
2.43.0

Re: [PATCH 0/7] Fix race in DWARF reader

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> The background DWARF reader turns out to have a few races.  This
Tom> series fixes one that occurs when the indexer runs at the same time as
Tom> DWARF relocation.

Tom> Most of the series is just cleanup / preparation.

Tom> The main patch affects MIPS16.  I can't test this -- I tried on a MIPS
Tom> machine in the GCC compile farm, but unfortunately the relevant
Tom> gdb.arch test says that the processor doesn't support MIPS16.  It's
Tom> possible this code is simply dead; I do not know.

Elsewhere I mentioned that I had a different idea for this series.

It seems to me that most (or maybe even all) the calls to
dwarf2_per_objfile::adjust aren't really needed.  Many of them only
affect lookup tables, where the adjustment isn't needed.  This includes
all calls made by the indexer.

Some of the calls (like the one in read_attribute_value) even seem to be
wrong.

So, I wrote a short series to remove these.  Unfortunately, though, it's
hard to know for sure if the result is correct, given that I don't know
how to test MIPS16.

I could probably test some simple things ("break") by debugging gdb
while examining (but not running) a MIPS16 program.  I'm not sure if
that's really sufficient though.

I'd appreciate some insight if you have any.

thanks,
Tom

Re: [PATCH 0/7] Fix race in DWARF reader

[John Baldwin]

On 4/3/24 11:16 AM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> The background DWARF reader turns out to have a few races.  This
> Tom> series fixes one that occurs when the indexer runs at the same time as
> Tom> DWARF relocation.
> 
> Tom> Most of the series is just cleanup / preparation.
> 
> Tom> The main patch affects MIPS16.  I can't test this -- I tried on a MIPS
> Tom> machine in the GCC compile farm, but unfortunately the relevant
> Tom> gdb.arch test says that the processor doesn't support MIPS16.  It's
> Tom> possible this code is simply dead; I do not know.
> 
> Elsewhere I mentioned that I had a different idea for this series.
> 
> It seems to me that most (or maybe even all) the calls to
> dwarf2_per_objfile::adjust aren't really needed.  Many of them only
> affect lookup tables, where the adjustment isn't needed.  This includes
> all calls made by the indexer.
> 
> Some of the calls (like the one in read_attribute_value) even seem to be
> wrong.
> 
> So, I wrote a short series to remove these.  Unfortunately, though, it's
> hard to know for sure if the result is correct, given that I don't know
> how to test MIPS16.
> 
> I could probably test some simple things ("break") by debugging gdb
> while examining (but not running) a MIPS16 program.  I'm not sure if
> that's really sufficient though.
> 
> I'd appreciate some insight if you have any.

I haven't seen anyone active with submitting MIPS patches in several years.
I no longer make use of MIPS myself (and we've removed it from FreeBSD
entirely, though I know it's still present in Linux distros).  Even when I
was working with MIPS I never tested microMIPS / MIPS16.

OTOH, I think MIPS16 is similar to Thumb on ARM, and it might even be
using a similar trick from reading your series (setting the LSB to enter
"compressed decoding mode" vs "regular decoding mode").  I think ARM uses
special mapping symbols to mark Thumb vs non-Thumb code though instead of
depending on the LSB?  That is, I wonder why Thumb doesn't trip over this
issue the way MIPS16 does?

-- 
John Baldwin

Re: [PATCH 0/7] Fix race in DWARF reader

[Luis Machado]

On 4/9/24 19:16, John Baldwin wrote:
> On 4/3/24 11:16 AM, Tom Tromey wrote:
>>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>>
>> Tom> The background DWARF reader turns out to have a few races.  This
>> Tom> series fixes one that occurs when the indexer runs at the same time as
>> Tom> DWARF relocation.
>>
>> Tom> Most of the series is just cleanup / preparation.
>>
>> Tom> The main patch affects MIPS16.  I can't test this -- I tried on a MIPS
>> Tom> machine in the GCC compile farm, but unfortunately the relevant
>> Tom> gdb.arch test says that the processor doesn't support MIPS16.  It's
>> Tom> possible this code is simply dead; I do not know.
>>
>> Elsewhere I mentioned that I had a different idea for this series.
>>
>> It seems to me that most (or maybe even all) the calls to
>> dwarf2_per_objfile::adjust aren't really needed.  Many of them only
>> affect lookup tables, where the adjustment isn't needed.  This includes
>> all calls made by the indexer.
>>
>> Some of the calls (like the one in read_attribute_value) even seem to be
>> wrong.
>>
>> So, I wrote a short series to remove these.  Unfortunately, though, it's
>> hard to know for sure if the result is correct, given that I don't know
>> how to test MIPS16.
>>
>> I could probably test some simple things ("break") by debugging gdb
>> while examining (but not running) a MIPS16 program.  I'm not sure if
>> that's really sufficient though.
>>
>> I'd appreciate some insight if you have any.
> 
> I haven't seen anyone active with submitting MIPS patches in several years.
> I no longer make use of MIPS myself (and we've removed it from FreeBSD
> entirely, though I know it's still present in Linux distros).  Even when I
> was working with MIPS I never tested microMIPS / MIPS16.
> 
> OTOH, I think MIPS16 is similar to Thumb on ARM, and it might even be
> using a similar trick from reading your series (setting the LSB to enter
> "compressed decoding mode" vs "regular decoding mode").  I think ARM uses
> special mapping symbols to mark Thumb vs non-Thumb code though instead of
> depending on the LSB?  That is, I wonder why Thumb doesn't trip over this
> issue the way MIPS16 does?
> 

cc-ing Maciej, who might have a better idea on MIPS bits.

Re: [PATCH 0/7] Fix race in DWARF reader

[Maciej W. Rozycki]

On Tue, 9 Apr 2024, Luis Machado wrote:

> >> Tom> The main patch affects MIPS16.  I can't test this -- I tried on a MIPS
> >> Tom> machine in the GCC compile farm, but unfortunately the relevant
> >> Tom> gdb.arch test says that the processor doesn't support MIPS16.  It's
> >> Tom> possible this code is simply dead; I do not know.
> >>
> >> Elsewhere I mentioned that I had a different idea for this series.
> >>
> >> It seems to me that most (or maybe even all) the calls to
> >> dwarf2_per_objfile::adjust aren't really needed.  Many of them only
> >> affect lookup tables, where the adjustment isn't needed.  This includes
> >> all calls made by the indexer.
> >>
> >> Some of the calls (like the one in read_attribute_value) even seem to be
> >> wrong.
> >>
> >> So, I wrote a short series to remove these.  Unfortunately, though, it's
> >> hard to know for sure if the result is correct, given that I don't know
> >> how to test MIPS16.
> >>
> >> I could probably test some simple things ("break") by debugging gdb
> >> while examining (but not running) a MIPS16 program.  I'm not sure if
> >> that's really sufficient though.
> >>
> >> I'd appreciate some insight if you have any.
> > 
> > I haven't seen anyone active with submitting MIPS patches in several years.
> > I no longer make use of MIPS myself (and we've removed it from FreeBSD
> > entirely, though I know it's still present in Linux distros).  Even when I
> > was working with MIPS I never tested microMIPS / MIPS16.

 Support for the MIPS target in Linux is certainly far from being dead and 
I believe new MIPS hardware continues being made.  Also a substantial MIPS 
patch for GDB for R6 ISA support is being pinged for review right now.

> > OTOH, I think MIPS16 is similar to Thumb on ARM, and it might even be
> > using a similar trick from reading your series (setting the LSB to enter
> > "compressed decoding mode" vs "regular decoding mode").  I think ARM uses
> > special mapping symbols to mark Thumb vs non-Thumb code though instead of
> > depending on the LSB?  That is, I wonder why Thumb doesn't trip over this
> > issue the way MIPS16 does?
> > 
> 
> cc-ing Maciej, who might have a better idea on MIPS bits.

 Thanks, Luis!

 I can certainly run MIPS16 GDB verification right away with actual 
hardware:

macro@malta(1)~$ uname -a
Linux malta 5.18.0-rc2-00254-gfb649bda6f56-dirty #2 Sat Nov 12 20:14:53 GMT 2022 mips unknown unknown GNU/Linux
macro@malta(2)~$ grep mips16 /proc/cpuinfo
ASEs implemented	: mips16 dsp dsp2
macro@malta(3)~$ 

however to understand the impact I'd have to go through the code changes, 
which I can't guarantee any specific timeframe for.

 Indeed at the machine level it is the LSB of the PC that tells compressed 
and regular code apart: you just flip the bit as required either by using 
special instructions with direct calls/jumps or explicitly with indirect 
ones; it's also correctly set according to the execution mode in effect in 
PC values recorded by hardware, such as the return address for function 
calls or the exception PC for kernel traps.

 Then whether compressed code uses the MIPS16 instruction encoding or the 
microMIPS instruction encoding it is the property of the implementation.

 Offhand I recall for compressed functions DWARF line information has the 
LSB of the PC set according to compressed vs regular encoding, however 
other DWARF records or the static ELF symbol table do not.  Compressed 
function symbols have appropriate flags set in `st_other' to tell them 
apart from regular function symbols.  BFD uses that information to set the 
LSB appropriately in relocation processing where applicable.

 To call a compressed function by hand or for function pointer comparison 
(e.g. against a datum stored in a program's variable or in a CPU register) 
in expression evaluation GDB has to recreate the LSB from information 
available and apply it to symbol values obtained from the symbol table, 
and it's a bit messy due to how things happened in the past.  Conversely, 
in certain contexts the LSB has to be removed instead, such as `x /i $pc'.  
I made all this at least work at one point, not without shortcomings (e.g. 
broken hex instruction dumps in `disassemble /r' output), which is what we 
have now.

 Later on Yao Qi came up with a better proposal building on a generalised 
property of some psABIs where a function pointer is not the function's 
address: <https://sourceware.org/ml/gdb-patches/2016-10/msg00430.html>.  
The proposal got stuck on an issue with the PPC64 target which got never 
resolved due to the shortage of time and higher priority tasks combined: 
<https://sourceware.org/ml/gdb-patches/2017-10/msg00096.html>.

 Maybe someone can pick it up from there?  I could do the necessary MIPS 
bits then myself, I certainly find it important enough to preempt other 
stuff I might otherwise want doing instead.

  Maciej

Re: [PATCH 0/7] Fix race in DWARF reader

[Tom Tromey]

>>>>> "Maciej" == Maciej W Rozycki <macro@orcam.me.uk> writes:

Maciej>  Support for the MIPS target in Linux is certainly far from being dead and 
Maciej> I believe new MIPS hardware continues being made.  Also a substantial MIPS 
Maciej> patch for GDB for R6 ISA support is being pinged for review right now.

Thanks.  In terms of dead-ness I was really referring just to MIPS16.

Anyway, I'm sending a different & much simpler variant of this fix.
Any testing you could provide on that would be appreciated.

Tom