[PATCH v2 0/5] Cleanups in lookup_minimal_symbol_by_pc_section

[PATCH v2 0/5] Cleanups in lookup_minimal_symbol_by_pc_section

[Tom Tromey]

This series is taken from an earlier approach to fix some races in the
DWARF reader.  It is just a few cleanups and one possible bug fix
related to an invalid (IMO) address comparison.

Regression tested on x86-64 Fedora 38.

---
Changes in v2:
- Removed the DWARF reader changes
- Link to v1: https://inbox.sourceware.org/gdb-patches/20240217-dwarf-race-relocate-v1-0-d3d2d908c1e8@tromey.com

---
Tom Tromey (5):
      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
      Use bound_minimal_symbol more in lookup_minimal_symbol_by_pc_section
      Fix address comparison in lookup_minimal_symbol_by_pc_section

 gdb/minsyms.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)
---
base-commit: f95ecfc6e07f619aaa183716cc3749f32b678d6e
change-id: 20240217-dwarf-race-relocate-287780f3ac82

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

[PATCH v2 1/5] 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.  This is
slightly more efficient.
---
 gdb/minsyms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 38176c4bdcb..ea557d3625f 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -900,8 +900,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.44.0

[PATCH v2 2/5] 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 ea557d3625f..97b1a1c15c8 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -875,13 +875,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.44.0

[PATCH v2 3/5] 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 just a small cleanup.  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 97b1a1c15c8..beafb6ad6dc 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -795,7 +795,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.  */
@@ -826,9 +828,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.44.0

[PATCH v2 4/5] 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 | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index beafb6ad6dc..c8d84344f80 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -762,10 +762,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
   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;
+  struct bound_minimal_symbol best = {};
 
   if (previous != nullptr)
     {
@@ -806,7 +803,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 	{
 	  int best_zero_sized = -1;
 
-	  msymbol = objfile->per_bfd->msymbols.get ();
+	  struct minimal_symbol *msymbol = objfile->per_bfd->msymbols.get ();
 	  lo = 0;
 	  hi = objfile->per_bfd->minimal_symbol_count - 1;
 
@@ -984,20 +981,15 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 		 overall.  */
 
 	      if (hi >= 0
-		  && ((best_symbol == NULL) ||
-		      (best_symbol->unrelocated_address () <
-		       msymbol[hi].unrelocated_address ())))
-		{
-		  best_symbol = &msymbol[hi];
-		  best_objfile = objfile;
-		}
+		  && (best.minsym == nullptr
+		      || (best.minsym->unrelocated_address ()
+			  < msymbol[hi].unrelocated_address ())))
+		best = { &msymbol[hi],  objfile };
 	    }
 	}
     }
 
-  result.minsym = best_symbol;
-  result.objfile = best_objfile;
-  return result;
+  return best;
 }
 
 /* See minsyms.h.  */

-- 
2.44.0

[PATCH v2 5/5] 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.
---
 gdb/minsyms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index c8d84344f80..3482ec7334c 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -982,8 +982,8 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 
 	      if (hi >= 0
 		  && (best.minsym == nullptr
-		      || (best.minsym->unrelocated_address ()
-			  < msymbol[hi].unrelocated_address ())))
+		      || (best.value_address ()
+			  < msymbol[hi].value_address (objfile))))
 		best = { &msymbol[hi],  objfile };
 	    }
 	}

-- 
2.44.0