Remove symbol lookup of arguments

Message ID 20260408141801.3082630-1-tromey@adacore.com
State New
Headers
Series Remove symbol lookup of arguments |

Checks

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

Commit Message

Tom Tromey April 8, 2026, 2:18 p.m. UTC
  gdb has some code that looks up argument variables a second time.  So
for instance in mi-cmd-stack.c:

	      if (sym->is_argument ())
		sym2 = (lookup_symbol_search_name
			(sym->search_name (),
			 block, SEARCH_VAR_DOMAIN).symbol);
	      else
		sym2 = sym;

I believe this is a leftover artifact of stabs.  I'm not really
certain (I never really learned stabs) but I think in stabs a function
argument had two symbols: one for the parameter and a second one that
described the location of the argument after the prologue.

DWARF doesn't do this, and there's no need to keep this code around
any more.

This patch removes these vestiges.  In the Ada code, removing the
special argument handling also left the "found_sym" code unused,
leading to more deletions.

For block_lookup_symbol, while the comment there explains that the
"best symbol" hack isn't needed in this situation, it seemed to me
that (1) it might in fact be (because I think a function block could
very well have locals and types as well), and (2) making this function
simpler and removing the argument hack is the better approach anyway.

Regression tested on x86-64 Fedora 43.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34037
---
 gdb/ada-lang.c        | 84 +++++--------------------------------------
 gdb/block.c           | 35 ++----------------
 gdb/mi/mi-cmd-stack.c | 19 +++-------
 gdb/stack.c           | 82 +-----------------------------------------
 4 files changed, 17 insertions(+), 203 deletions(-)


base-commit: e3998113f9b66cbd0806424ac4700d2aaf8d0f77
  

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 805999e18ee..7f1873d247f 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -5429,26 +5429,10 @@  struct match_data
 
   bool operator() (struct block_symbol *bsym);
 
-  void finish (const block *block);
-
   struct objfile *objfile = nullptr;
   std::vector<struct block_symbol> *resultp;
-  struct symbol *arg_sym = nullptr;
-  bool found_sym = false;
 };
 
-/* Finish iteration over one block.  If a symbol hasn't been found
-   already, add 'arg_sym' to the list of symbols.  */
-
-void
-match_data::finish (const block *block)
-{
-  if (!found_sym && arg_sym != nullptr)
-    add_defn_to_vec (*resultp, arg_sym, block);
-  found_sym = false;
-  arg_sym = nullptr;
-}
-
 /* A callback for add_nonlocal_symbols that adds symbol, found in
    BSYM, to a list of symbols.  */
 
@@ -5460,29 +5444,22 @@  match_data::operator() (struct block_symbol *bsym)
 
   if (sym->loc_class () == LOC_UNRESOLVED)
     return true;
-  else if (sym->is_argument ())
-    arg_sym = sym;
   else
-    {
-      found_sym = true;
-      add_defn_to_vec (*resultp, sym, block);
-    }
+    add_defn_to_vec (*resultp, sym, block);
 
   return true;
 }
 
 /* Helper for add_nonlocal_symbols.  Find symbols in DOMAIN which are
    targeted by renamings matching LOOKUP_NAME in BLOCK.  Add these
-   symbols to RESULT.  Return whether we found such symbols.  */
+   symbols to RESULT.  */
 
-static bool
+static void
 ada_add_block_renamings (std::vector<struct block_symbol> &result,
 			 const struct block *block,
 			 const lookup_name_info &lookup_name,
 			 domain_search_flags domain)
 {
-  int defns_mark = result.size ();
-
   symbol_name_matcher_ftype *name_match
     = ada_get_symbol_name_matcher (lookup_name);
 
@@ -5533,7 +5510,6 @@  ada_add_block_renamings (std::vector<struct block_symbol> &result,
 			       1, NULL);
 	}
     }
-  return result.size () != defns_mark;
 }
 
 /* Convenience function to get at the Ada encoded lookup name for
@@ -5567,7 +5543,6 @@  map_matching_symbols (struct objfile *objfile,
 	 result but assert just to be future-proof.  */
       bool result = iterate_over_symbols (block, lookup_name, domain, data);
       gdb_assert (result);
-      data.finish (block);
       return true;
     };
 
@@ -5599,9 +5574,7 @@  add_nonlocal_symbols (std::vector<struct block_symbol> &result,
 	  const struct block *global_block
 	    = cu.blockvector ()->global_block ();
 
-	  if (ada_add_block_renamings (result, global_block, lookup_name,
-				       domain))
-	    data.found_sym = true;
+	  ada_add_block_renamings (result, global_block, lookup_name, domain);
 	}
     }
 
@@ -6052,44 +6025,18 @@  ada_add_block_symbols (std::vector<struct block_symbol> &result,
 		       const lookup_name_info &lookup_name,
 		       domain_search_flags domain, struct objfile *objfile)
 {
-  /* A matching argument symbol, if any.  */
-  struct symbol *arg_sym;
-  /* Set true when we find a matching non-argument symbol.  */
-  bool found_sym;
-
-  arg_sym = NULL;
-  found_sym = false;
   for (struct symbol *sym : block_iterator_range (block, &lookup_name))
     {
-      if (sym->matches (domain))
-	{
-	  if (sym->loc_class () != LOC_UNRESOLVED)
-	    {
-	      if (sym->is_argument ())
-		arg_sym = sym;
-	      else
-		{
-		  found_sym = true;
-		  add_defn_to_vec (result, sym, block);
-		}
-	    }
-	}
+      if (sym->matches (domain) && sym->loc_class () != LOC_UNRESOLVED)
+	add_defn_to_vec (result, sym, block);
     }
 
   /* Handle renamings.  */
 
-  if (ada_add_block_renamings (result, block, lookup_name, domain))
-    found_sym = true;
-
-  if (!found_sym && arg_sym != NULL)
-    {
-      add_defn_to_vec (result, arg_sym, block);
-    }
+  ada_add_block_renamings (result, block, lookup_name, domain);
 
   if (!lookup_name.ada ().wild_match_p ())
     {
-      arg_sym = NULL;
-      found_sym = false;
       const std::string &ada_lookup_name = lookup_name.ada ().lookup_name ();
       const char *name = ada_lookup_name.c_str ();
       size_t name_len = ada_lookup_name.size ();
@@ -6113,25 +6060,10 @@  ada_add_block_symbols (std::vector<struct block_symbol> &result,
 		&& is_name_suffix (sym->linkage_name () + name_len + 5))
 	      {
 		if (sym->loc_class () != LOC_UNRESOLVED)
-		  {
-		    if (sym->is_argument ())
-		      arg_sym = sym;
-		    else
-		      {
-			found_sym = true;
-			add_defn_to_vec (result, sym, block);
-		      }
-		  }
+		  add_defn_to_vec (result, sym, block);
 	      }
 	  }
       }
-
-      /* NOTE: This really shouldn't be needed for _ada_ symbols.
-	 They aren't parameters, right?  */
-      if (!found_sym && arg_sym != NULL)
-	{
-	  add_defn_to_vec (result, arg_sym, block);
-	}
     }
 }
 
diff --git a/gdb/block.c b/gdb/block.c
index dc00327048f..e8537b96ed4 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -638,38 +638,9 @@  struct symbol *
 block_lookup_symbol (const struct block *block, const lookup_name_info &name,
 		     const domain_search_flags domain)
 {
-  if (!block->function ())
-    {
-      best_symbol_tracker tracker;
-      tracker.search (nullptr, block, name, domain);
-      return tracker.currently_best.symbol;
-    }
-  else
-    {
-      /* Note that parameter symbols do not always show up last in the
-	 list; this loop makes sure to take anything else other than
-	 parameter symbols first; it only uses parameter symbols as a
-	 last resort.  Note that this only takes up extra computation
-	 time on a match.
-	 It's hard to define types in the parameter list (at least in
-	 C/C++) so we don't do the same PR 16253 hack here that is done
-	 for the !BLOCK_FUNCTION case.  */
-
-      struct symbol *sym_found = NULL;
-
-      for (struct symbol *sym : block_iterator_range (block, &name))
-	{
-	  if (sym->matches (domain))
-	    {
-	      sym_found = sym;
-	      if (!sym->is_argument ())
-		{
-		  break;
-		}
-	    }
-	}
-      return (sym_found);	/* Will be NULL if not found.  */
-    }
+  best_symbol_tracker tracker;
+  tracker.search (nullptr, block, name, domain);
+  return tracker.currently_best.symbol;
 }
 
 /* See block.h.  */
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index f2a4b3044b0..3281be2b4dc 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -633,34 +633,25 @@  list_args_or_locals (const frame_print_options &fp_opts,
 	    }
 	  if (print_me)
 	    {
-	      struct symbol *sym2;
 	      struct frame_arg arg, entryarg;
 
-	      if (sym->is_argument ())
-		sym2 = (lookup_symbol_search_name
-			(sym->search_name (),
-			 block, SEARCH_VAR_DOMAIN).symbol);
-	      else
-		sym2 = sym;
-	      gdb_assert (sym2 != NULL);
-
-	      arg.sym = sym2;
+	      arg.sym = sym;
 	      arg.entry_kind = print_entry_values_no;
-	      entryarg.sym = sym2;
+	      entryarg.sym = sym;
 	      entryarg.entry_kind = print_entry_values_no;
 
 	      switch (values)
 		{
 		case PRINT_SIMPLE_VALUES:
-		  if (!mi_simple_type_p (sym2->type ()))
+		  if (!mi_simple_type_p (sym->type ()))
 		    break;
 		  [[fallthrough]];
 
 		case PRINT_ALL_VALUES:
 		  if (sym->is_argument ())
-		    read_frame_arg (fp_opts, sym2, fi, &arg, &entryarg);
+		    read_frame_arg (fp_opts, sym, fi, &arg, &entryarg);
 		  else
-		    read_frame_local (sym2, fi, &arg);
+		    read_frame_local (sym, fi, &arg);
 		  break;
 		}
 
diff --git a/gdb/stack.c b/gdb/stack.c
index 6fcc26417e2..7329430adab 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -801,70 +801,6 @@  print_frame_args (const frame_print_options &fp_opts,
 	      break;
 	    }
 
-	  /* We have to look up the symbol because arguments can have
-	     two entries (one a parameter, one a local) and the one we
-	     want is the local, which lookup_symbol will find for us.
-	     This includes gcc1 (not gcc2) on SPARC when passing a
-	     small structure and gcc2 when the argument type is float
-	     and it is passed as a double and converted to float by
-	     the prologue (in the latter case the type of the LOC_ARG
-	     symbol is double and the type of the LOC_LOCAL symbol is
-	     float).  */
-	  /* But if the parameter name is null, don't try it.  Null
-	     parameter names occur on the RS/6000, for traceback
-	     tables.  FIXME, should we even print them?  */
-
-	  if (*sym->linkage_name ())
-	    {
-	      struct symbol *nsym;
-
-	      nsym = lookup_symbol_search_name (sym->search_name (),
-						b, SEARCH_VAR_DOMAIN).symbol;
-	      gdb_assert (nsym != NULL);
-	      if (nsym->loc_class () == LOC_REGISTER
-		  && !nsym->is_argument ())
-		{
-		  /* There is a LOC_ARG/LOC_REGISTER pair.  This means
-		     that it was passed on the stack and loaded into a
-		     register, or passed in a register and stored in a
-		     stack slot.  GDB 3.x used the LOC_ARG; GDB
-		     4.0-4.11 used the LOC_REGISTER.
-
-		     Reasons for using the LOC_ARG:
-
-		     (1) Because find_saved_registers may be slow for
-			 remote debugging.
-
-		     (2) Because registers are often reused and stack
-			 slots rarely (never?) are.  Therefore using
-			 the stack slot is much less likely to print
-			 garbage.
-
-		     Reasons why we might want to use the LOC_REGISTER:
-
-		     (1) So that the backtrace prints the same value
-			 as "print foo".  I see no compelling reason
-			 why this needs to be the case; having the
-			 backtrace print the value which was passed
-			 in, and "print foo" print the value as
-			 modified within the called function, makes
-			 perfect sense to me.
-
-		     Additional note: It might be nice if "info args"
-		     displayed both values.
-
-		     One more note: There is a case with SPARC
-		     structure passing where we need to use the
-		     LOC_REGISTER, but this is dealt with by creating
-		     a single LOC_REGPARM in symbol reading.  */
-
-		  /* Leave sym (the LOC_ARG) alone.  */
-		  ;
-		}
-	      else
-		sym = nsym;
-	    }
-
 	  /* Print the current arg.  */
 	  if (!first)
 	    uiout->text (", ");
@@ -2446,23 +2382,7 @@  iterate_over_block_arg_vars (const struct block *b,
     {
       /* Don't worry about things which aren't arguments.  */
       if (sym->is_argument ())
-	{
-	  /* We have to look up the symbol because arguments can have
-	     two entries (one a parameter, one a local) and the one we
-	     want is the local, which lookup_symbol will find for us.
-	     This includes gcc1 (not gcc2) on the sparc when passing a
-	     small structure and gcc2 when the argument type is float
-	     and it is passed as a double and converted to float by
-	     the prologue (in the latter case the type of the LOC_ARG
-	     symbol is double and the type of the LOC_LOCAL symbol is
-	     float).  There are also LOC_ARG/LOC_REGISTER pairs which
-	     are not combined in symbol-reading.  */
-
-	  struct symbol *sym2
-	    = lookup_symbol_search_name (sym->search_name (),
-					 b, SEARCH_VAR_DOMAIN).symbol;
-	  cb (sym->print_name (), sym2);
-	}
+	cb (sym->print_name (), sym);
     }
 }