[RFA] make first parameter of to_lookup_symbol const char *

[RFA] make first parameter of to_lookup_symbol const char *

[Tristan Gingold]

Hi,

is there any good reason why the NAME parameter is 'char *' instead of 'const char *' ?  I can't see any of them.

This patch was tested only by recompiling gdb for powerpc-elf.

BTW, it looks like no target defines this operation...

Tristan.

2011-03-14  Tristan Gingold  <gingold@adacore.com>

	* target.h (target_ops): Make NAME parameter of to_lookup_symbol
	const char*
	* target.c (debug_to_lookup_symbol): Constify NAME parameter.
	Adjust prototype.
	(update_current_target): Adjust.

diff --git a/gdb/target.c b/gdb/target.c
index c155716..0e6d652 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -151,7 +151,7 @@ static void debug_to_load (char *, int);
 
 static void debug_to_unload (char *, int);
 
-static int debug_to_lookup_symbol (char *, CORE_ADDR *);
+static int debug_to_lookup_symbol (const char *, CORE_ADDR *);
 
 static int debug_to_can_run (void);
 
@@ -781,7 +781,7 @@ update_current_target (void)
 	    (void (*) (char *, int))
 	    tcomplain);
   de_fault (to_lookup_symbol,
-	    (int (*) (char *, CORE_ADDR *))
+	    (int (*) (const char *, CORE_ADDR *))
 	    nosymbol);
   de_fault (to_post_startup_inferior,
 	    (void (*) (ptid_t))
@@ -3679,7 +3679,7 @@ debug_to_unload (char *args, int from_tty)
 }
 
 static int
-debug_to_lookup_symbol (char *name, CORE_ADDR *addrp)
+debug_to_lookup_symbol (const char *name, CORE_ADDR *addrp)
 {
   int retval;
 
diff --git a/gdb/target.h b/gdb/target.h
index b8db4bc..2318b9d 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -478,7 +478,7 @@ struct target_ops
     void (*to_kill) (struct target_ops *);
     void (*to_load) (char *, int);
     void (*to_unload) (char *, int);
-    int (*to_lookup_symbol) (char *, CORE_ADDR *);
+    int (*to_lookup_symbol) (const char *, CORE_ADDR *);
     void (*to_create_inferior) (struct target_ops *, 
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);

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

[Jan Kratochvil]

Hi Tristan.,

On Mon, 14 Mar 2011 11:13:43 +0100, Tristan Gingold wrote:
> is there any good reason why the NAME parameter is 'char *' instead of
> 'const char *' ?  I can't see any of them.

there were many similar patches.


> BTW, it looks like no target defines this operation...

Unless you use it for some new patch of yours it should be removed instead.


Thanks,
Jan

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

[Pedro Alves]

On Monday 14 March 2011 10:13:43, Tristan Gingold wrote:

> is there any good reason why the NAME parameter is 'char *' instead of 'const char *' ?  I can't see any of them.

Old code.  Original K&R didn't know about const.  We have a lot of places
that could/should be const but aren't.  It's an ongoing slow cleanup.

> 
> This patch was tested only by recompiling gdb for powerpc-elf.
> 
> BTW, it looks like no target defines this operation...

Are you going to add a use of it?  Otherwise, I'd rather
just getting rid of it.

> 
> Tristan.
> 
> 2011-03-14  Tristan Gingold  <gingold@adacore.com>
> 
> 	* target.h (target_ops): Make NAME parameter of to_lookup_symbol
> 	const char*
> 	* target.c (debug_to_lookup_symbol): Constify NAME parameter.
> 	Adjust prototype.
> 	(update_current_target): Adjust.
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index c155716..0e6d652 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -151,7 +151,7 @@ static void debug_to_load (char *, int);
>  
>  static void debug_to_unload (char *, int);
>  
> -static int debug_to_lookup_symbol (char *, CORE_ADDR *);
> +static int debug_to_lookup_symbol (const char *, CORE_ADDR *);
>  
>  static int debug_to_can_run (void);
>  
> @@ -781,7 +781,7 @@ update_current_target (void)
>  	    (void (*) (char *, int))
>  	    tcomplain);
>    de_fault (to_lookup_symbol,
> -	    (int (*) (char *, CORE_ADDR *))
> +	    (int (*) (const char *, CORE_ADDR *))
>  	    nosymbol);
>    de_fault (to_post_startup_inferior,
>  	    (void (*) (ptid_t))
> @@ -3679,7 +3679,7 @@ debug_to_unload (char *args, int from_tty)
>  }
>  
>  static int
> -debug_to_lookup_symbol (char *name, CORE_ADDR *addrp)
> +debug_to_lookup_symbol (const char *name, CORE_ADDR *addrp)
>  {
>    int retval;
>  
> diff --git a/gdb/target.h b/gdb/target.h
> index b8db4bc..2318b9d 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -478,7 +478,7 @@ struct target_ops
>      void (*to_kill) (struct target_ops *);
>      void (*to_load) (char *, int);
>      void (*to_unload) (char *, int);
> -    int (*to_lookup_symbol) (char *, CORE_ADDR *);
> +    int (*to_lookup_symbol) (const char *, CORE_ADDR *);
>      void (*to_create_inferior) (struct target_ops *, 
>  				char *, char *, char **, int);
>      void (*to_post_startup_inferior) (ptid_t);
> 
> 

-- 
Pedro Alves

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

[Tristan Gingold]

On Mar 14, 2011, at 2:53 PM, Pedro Alves wrote:

> On Monday 14 March 2011 10:13:43, Tristan Gingold wrote:
> 
>> is there any good reason why the NAME parameter is 'char *' instead of 'const char *' ?  I can't see any of them.
> 
> Old code.  Original K&R didn't know about const.  We have a lot of places
> that could/should be const but aren't.  It's an ongoing slow cleanup.
> 
>> 
>> This patch was tested only by recompiling gdb for powerpc-elf.
>> 
>> BTW, it looks like no target defines this operation...
> 
> Are you going to add a use of it?  Otherwise, I'd rather
> just getting rid of it.

No.

Joel, can we remove that ?  I thought we were using it for dfw, but we don't anymore.

Tristan.

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

[Joel Brobecker]

> Joel, can we remove that ?  I thought we were using it for dfw, but we
> don't anymore.

I did a quick grep, and I don't see any users in our code either.

-- 
Joel

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

[Joel Brobecker]

[-- 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.

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

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

I think it is a good idea to remove unused code.  If it is needed in the
future, it is simple enough to resurrect or rewrite this code.

The patch looks fine to me.

Tom

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

[Stan Shebs]

On 3/16/11 6:58 AM, Joel Brobecker wrote:
>>> 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.

Yeah, it looks like the last use quietly evaporated with the deletion of 
remote-vx.c in 2004.

Features without any means of exercise are likely to bitrot semantically 
even if they continue to compile/run (witness tracepoints), so it's in 
our interest to be ruthless.

I used to have fun using gcov with the GDB testsuite to find segments of 
code that were never exercised - gcov results are cumulative, so the 
post-testing coverage display is good for ideas as to what test cases 
ought to be written, and also suggests code that might turn out to be 
intrinsically unreachable.

Perhaps msnyder can play with that when he gets tired of coverity. :-)

Stan

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

[Joel Brobecker]

> 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.

Thanks to everyone for the review and feedback.  I checked this patch in.

-- 
Joel