Re: [RFA] make first parameter of to_lookup_symbol const char *

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

> > BTW, it looks like no target defines this operation...
> 
> Unless you use it for some new patch of yours it should be removed instead.

This is what it looks like to remove the target_ops method. It feels
a little like excising a potentially useful feature, so I'm not going
to commit without review, although there is no sign that we'll ever
need it any time soon.  But I added a comment explaining what we used
to do, to give us a clue later on, if we encounter a target where
we might need something of this kind.

The patch is currently only tested on x86_64-linux, with standard
ELF/DWARF, so no real for the dbxread changes, as well as the coffread
ones. I can do a little more testing, for instance with stabs, if
there is agreement on the patch.

-- 
Joel

[-- Attachment #2: target-lookup-symbol.diff --]
[-- Type: text/x-diff, Size: 8375 bytes --]

commit 1c493e43449a507c99664ccf0f3345dccd04d163
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed Mar 16 06:47:25 2011 -0700

    delete target_ops.to_lookup_symbol
    
    gdb/ChangeLog:
    
            * target.h (struct target_ops): Remove to_lookup_symbol field.
            (target_lookup_symbol): Delete macro.
            * target.c (nosymbol, debug_to_lookup_symbol): Delete.
            (update_current_target, setup_target_debug): Remove handling
            of to_lookup_symbol target_ops field.
            * ada-tasks.c (get_known_tasks_addr): Remove use of
            target_lookup_symbol.
            * coffread.c (coff_symtab_read): Likewise.
            * dbxread.c (read_dbx_symtab): Ditto.

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 216902a..2cf62b9 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -300,7 +300,7 @@ read_fat_string_value (char *dest, struct value *val, int max_len)
 }
 
 /* Return the address of the Known_Tasks array maintained in
-   the Ada Runtime.  Return NULL if the array could not be found,
+   the Ada Runtime.  Return zero if the array could not be found,
    meaning that the inferior program probably does not use tasking.
 
    In order to provide a fast response time, this function caches
@@ -317,13 +317,9 @@ get_known_tasks_addr (void)
       struct minimal_symbol *msym;
 
       msym = lookup_minimal_symbol (KNOWN_TASKS_NAME, NULL, NULL);
-      if (msym != NULL)
-        known_tasks_addr = SYMBOL_VALUE_ADDRESS (msym);
-      else
-        {
-          if (target_lookup_symbol (KNOWN_TASKS_NAME, &known_tasks_addr) != 0)
-            return 0;
-        }
+      if (msym == NULL)
+        return 0;
+      known_tasks_addr = SYMBOL_VALUE_ADDRESS (msym);
 
       /* FIXME: brobecker 2003-03-05: Here would be a much better place
          to attach the ada-tasks observers, instead of doing this
diff --git a/gdb/coffread.c b/gdb/coffread.c
index 8ec87c1..b11dd73 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -902,22 +902,14 @@ coff_symtab_read (long symtab_offset, unsigned int nsyms,
 
 	    if (cs->c_secnum == N_UNDEF)
 	      {
-		/* This is a common symbol.  See if the target
-		   environment knows where it has been relocated to.  */
-		CORE_ADDR reladdr;
-
-		if (target_lookup_symbol (cs->c_name, &reladdr))
-		  {
-		    /* Error in lookup; ignore symbol.  */
-		    break;
-		  }
-		tmpaddr = reladdr;
-		/* The address has already been relocated; make sure that
-		   objfile_relocate doesn't relocate it again.  */
-		sec = -2;
-		ms_type = cs->c_sclass == C_EXT
-		  || cs->c_sclass == C_THUMBEXT ?
-		  mst_bss : mst_file_bss;
+		/* This is a common symbol.  We used to rely on
+		   the target to tell us whether it knows where
+		   the symbol has been relocated to, but none of
+		   the target implementations actually provided
+		   that operation.  So we just ignore the symbol,
+		   the same way we would do if we had a target-side
+		   symbol lookup which returned no match.  */
+		break;
 	      }
  	    else if (cs->c_secnum == N_ABS)
  	      {
diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index 2e32f38..957807b 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -1401,24 +1401,16 @@ read_dbx_symtab (struct objfile *objfile)
 	  goto record_it;
 
 	case N_UNDF | N_EXT:
-	  if (nlist.n_value != 0)
-	    {
-	      /* This is a "Fortran COMMON" symbol.  See if the target
-		 environment knows where it has been relocated to.  */
-
-	      CORE_ADDR reladdr;
-
-	      namestring = set_namestring (objfile, &nlist);
-	      if (target_lookup_symbol (namestring, &reladdr))
-		{
-		  continue;	/* Error in lookup; ignore symbol for now.  */
-		}
-	      nlist.n_type ^= (N_BSS ^ N_UNDF);	/* Define it as a
-						   bss-symbol.  */
-	      nlist.n_value = reladdr;
-	      goto bss_ext_symbol;
-	    }
-	  continue;		/* Just undefined, not COMMON.  */
+	  /* The case (nlist.n_value != 0) is a "Fortran COMMON" symbol.
+	     We used to rely on the target to tell us whether it knows
+	     where the symbol has been relocated to, but none of the
+	     target implementations actually provided that operation.
+	     So we just ignore the symbol, the same way we would do if
+	     we had a target-side symbol lookup which returned no match.
+
+	     All other symbols (with nlist.n_value == 0), are really
+	     undefined, and so we ignore them too.  */
+	  continue;
 
 	case N_UNDF:
 	  if (processing_acc_compilation && nlist.n_strx == 1)
diff --git a/gdb/target.c b/gdb/target.c
index aa59920..45259fd 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -54,8 +54,6 @@ static int default_watchpoint_addr_within_range (struct target_ops *,
 
 static int default_region_ok_for_hw_watchpoint (CORE_ADDR, int);
 
-static int nosymbol (char *, CORE_ADDR *);
-
 static void tcomplain (void) ATTRIBUTE_NORETURN;
 
 static int nomemory (CORE_ADDR, char *, int, int, struct target_ops *);
@@ -149,8 +147,6 @@ static void debug_to_terminal_info (char *, int);
 
 static void debug_to_load (char *, int);
 
-static int debug_to_lookup_symbol (char *, CORE_ADDR *);
-
 static int debug_to_can_run (void);
 
 static void debug_to_notice_signals (ptid_t);
@@ -532,12 +528,6 @@ noprocess (void)
   error (_("You can't do that without a process to debug."));
 }
 
-static int
-nosymbol (char *name, CORE_ADDR *addrp)
-{
-  return 1;			/* Symbol does not exist in target env.  */
-}
-
 static void
 default_terminal_info (char *args, int from_tty)
 {
@@ -621,7 +611,6 @@ update_current_target (void)
       INHERIT (to_terminal_info, t);
       /* Do not inherit to_kill.  */
       INHERIT (to_load, t);
-      INHERIT (to_lookup_symbol, t);
       /* Do no inherit to_create_inferior.  */
       INHERIT (to_post_startup_inferior, t);
       INHERIT (to_insert_fork_catchpoint, t);
@@ -774,9 +763,6 @@ update_current_target (void)
   de_fault (to_load,
 	    (void (*) (char *, int))
 	    tcomplain);
-  de_fault (to_lookup_symbol,
-	    (int (*) (char *, CORE_ADDR *))
-	    nosymbol);
   de_fault (to_post_startup_inferior,
 	    (void (*) (ptid_t))
 	    target_ignore);
@@ -3800,18 +3786,6 @@ debug_to_load (char *args, int from_tty)
   fprintf_unfiltered (gdb_stdlog, "target_load (%s, %d)\n", args, from_tty);
 }
 
-static int
-debug_to_lookup_symbol (char *name, CORE_ADDR *addrp)
-{
-  int retval;
-
-  retval = debug_target.to_lookup_symbol (name, addrp);
-
-  fprintf_unfiltered (gdb_stdlog, "target_lookup_symbol (%s, xxx)\n", name);
-
-  return retval;
-}
-
 static void
 debug_to_post_startup_inferior (ptid_t ptid)
 {
@@ -4011,7 +3985,6 @@ setup_target_debug (void)
   current_target.to_terminal_save_ours = debug_to_terminal_save_ours;
   current_target.to_terminal_info = debug_to_terminal_info;
   current_target.to_load = debug_to_load;
-  current_target.to_lookup_symbol = debug_to_lookup_symbol;
   current_target.to_post_startup_inferior = debug_to_post_startup_inferior;
   current_target.to_insert_fork_catchpoint = debug_to_insert_fork_catchpoint;
   current_target.to_remove_fork_catchpoint = debug_to_remove_fork_catchpoint;
diff --git a/gdb/target.h b/gdb/target.h
index e19f7b7..237d1aa 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -479,7 +479,6 @@ struct target_ops
     void (*to_terminal_info) (char *, int);
     void (*to_kill) (struct target_ops *);
     void (*to_load) (char *, int);
-    int (*to_lookup_symbol) (char *, CORE_ADDR *);
     void (*to_create_inferior) (struct target_ops *, 
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);
@@ -1016,17 +1015,6 @@ extern void target_kill (void);
 
 extern void target_load (char *arg, int from_tty);
 
-/* Look up a symbol in the target's symbol table.  NAME is the symbol
-   name.  ADDRP is a CORE_ADDR * pointing to where the value of the
-   symbol should be returned.  The result is 0 if successful, nonzero
-   if the symbol does not exist in the target environment.  This
-   function should not call error() if communication with the target
-   is interrupted, since it is called from symbol reading, but should
-   return nonzero, possibly doing a complain().  */
-
-#define target_lookup_symbol(name, addrp) \
-     (*current_target.to_lookup_symbol) (name, addrp)
-
 /* Start an inferior process and set inferior_ptid to its pid.
    EXEC_FILE is the file to run.
    ALLARGS is a string containing the arguments to the program.