* [RFA] make first parameter of to_lookup_symbol const char * @ 2011-03-14 11:56 Tristan Gingold 2011-03-14 13:53 ` Jan Kratochvil 2011-03-14 14:03 ` Pedro Alves 0 siblings, 2 replies; 9+ messages in thread From: Tristan Gingold @ 2011-03-14 11:56 UTC (permalink / raw) To: gdb-patches@sourceware.org ml 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-14 11:56 [RFA] make first parameter of to_lookup_symbol const char * Tristan Gingold @ 2011-03-14 13:53 ` Jan Kratochvil 2011-03-16 14:02 ` Joel Brobecker 2011-03-14 14:03 ` Pedro Alves 1 sibling, 1 reply; 9+ messages in thread From: Jan Kratochvil @ 2011-03-14 13:53 UTC (permalink / raw) To: Tristan Gingold; +Cc: gdb-patches@sourceware.org ml 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-14 13:53 ` Jan Kratochvil @ 2011-03-16 14:02 ` Joel Brobecker 2011-03-16 15:03 ` Tom Tromey ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Joel Brobecker @ 2011-03-16 14:02 UTC (permalink / raw) To: Jan Kratochvil, Pedro Alves Cc: Tristan Gingold, gdb-patches@sourceware.org ml [-- 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-16 14:02 ` Joel Brobecker @ 2011-03-16 15:03 ` Tom Tromey 2011-03-16 20:03 ` Stan Shebs 2011-03-17 14:58 ` Joel Brobecker 2 siblings, 0 replies; 9+ messages in thread From: Tom Tromey @ 2011-03-16 15:03 UTC (permalink / raw) To: Joel Brobecker Cc: Jan Kratochvil, Pedro Alves, Tristan Gingold, gdb-patches@sourceware.org ml >>>>> "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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-16 14:02 ` Joel Brobecker 2011-03-16 15:03 ` Tom Tromey @ 2011-03-16 20:03 ` Stan Shebs 2011-03-17 14:58 ` Joel Brobecker 2 siblings, 0 replies; 9+ messages in thread From: Stan Shebs @ 2011-03-16 20:03 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-16 14:02 ` Joel Brobecker 2011-03-16 15:03 ` Tom Tromey 2011-03-16 20:03 ` Stan Shebs @ 2011-03-17 14:58 ` Joel Brobecker 2 siblings, 0 replies; 9+ messages in thread From: Joel Brobecker @ 2011-03-17 14:58 UTC (permalink / raw) To: gdb-patches > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-14 11:56 [RFA] make first parameter of to_lookup_symbol const char * Tristan Gingold 2011-03-14 13:53 ` Jan Kratochvil @ 2011-03-14 14:03 ` Pedro Alves 2011-03-14 14:21 ` Tristan Gingold 1 sibling, 1 reply; 9+ messages in thread From: Pedro Alves @ 2011-03-14 14:03 UTC (permalink / raw) To: gdb-patches; +Cc: Tristan Gingold 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-14 14:03 ` Pedro Alves @ 2011-03-14 14:21 ` Tristan Gingold 2011-03-15 8:26 ` Joel Brobecker 0 siblings, 1 reply; 9+ messages in thread From: Tristan Gingold @ 2011-03-14 14:21 UTC (permalink / raw) To: Pedro Alves, Joel Brobecker; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] make first parameter of to_lookup_symbol const char * 2011-03-14 14:21 ` Tristan Gingold @ 2011-03-15 8:26 ` Joel Brobecker 0 siblings, 0 replies; 9+ messages in thread From: Joel Brobecker @ 2011-03-15 8:26 UTC (permalink / raw) To: Tristan Gingold; +Cc: Pedro Alves, gdb-patches > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-03-17 13:21 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-03-14 11:56 [RFA] make first parameter of to_lookup_symbol const char * Tristan Gingold 2011-03-14 13:53 ` Jan Kratochvil 2011-03-16 14:02 ` Joel Brobecker 2011-03-16 15:03 ` Tom Tromey 2011-03-16 20:03 ` Stan Shebs 2011-03-17 14:58 ` Joel Brobecker 2011-03-14 14:03 ` Pedro Alves 2011-03-14 14:21 ` Tristan Gingold 2011-03-15 8:26 ` Joel Brobecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).