[12/15] Add target/symbol.h, update users
Commit Message
This adds a new "target/symbol.h" file, that declares a symbol-lookup
function used by code in common. It follows the usual pattern, where
clients of "common" have their own definitions of this function. This
simplifies agent.c a bit.
gdb/
2014-07-09 Tom Tromey <tromey@redhat.com>
* target/symbol.h: New file.
* target.h: Include target/symbol.h.
* target.c (target_look_up_symbol): New function.
* common/agent.c: Include target/symbol.h.
(agent_look_up_symbols): Use target_look_up_symbol.
gdb/gdbserver/
2014-07-09 Tom Tromey <tromey@redhat.com>
* target.c: Include target/symbol.h.
(target_look_up_symbol): New function.
---
gdb/ChangeLog | 8 ++++++++
gdb/common/agent.c | 13 ++-----------
gdb/gdbserver/ChangeLog | 5 +++++
gdb/gdbserver/target.c | 9 +++++++++
gdb/target.c | 13 +++++++++++++
gdb/target.h | 1 +
gdb/target/symbol.h | 26 ++++++++++++++++++++++++++
7 files changed, 64 insertions(+), 11 deletions(-)
create mode 100644 gdb/target/symbol.h
Comments
>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
Gary> +/* See target/symbol.h. */
Gary> +
Gary> +int
Gary> +target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)
I never wrote these docs either...
Tom
On Thu, Jul 10, 2014 at 10:52 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
>
> Gary> +/* See target/symbol.h. */
> Gary> +
> Gary> +int
> Gary> +target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)
>
> I never wrote these docs either...
Nit,
Anything with target_ as a prefix I think of as a target.h method.
[There are a few exceptions but as long as it's kept to a minimum it's
manageable.]
IWBN if the different, umm, subsystems of gdb were easily recognizable
in the code.
As target/* scales up, is there a risk of the code becoming harder to
read if target_ is used as a general prefix for things in target/*?
Dunno. Just wondering.
Doug> As target/* scales up, is there a risk of the code becoming harder to
Doug> read if target_ is used as a general prefix for things in target/*?
Doug> Dunno. Just wondering.
My long term goal is that gdb and gdbserver share the entire target
stack. I think these patches further this goal. I don't find the
result harder to read at all.
Tom
Tom Tromey wrote:
> Doug> As target/* scales up, is there a risk of the code becoming
> Doug> harder to read if target_ is used as a general prefix for
> Doug> things in target/*? Dunno. Just wondering.
>
> My long term goal is that gdb and gdbserver share the entire target
> stack. I think these patches further this goal. I don't find the
> result harder to read at all.
Doug, are you ok for me to leave it as it is, or, do you have an
alternative you would like me to implement instead?
Thanks,
Gary
On Fri, Jul 11, 2014 at 5:56 AM, Gary Benson <gbenson@redhat.com> wrote:
> Tom Tromey wrote:
>> Doug> As target/* scales up, is there a risk of the code becoming
>> Doug> harder to read if target_ is used as a general prefix for
>> Doug> things in target/*? Dunno. Just wondering.
>>
>> My long term goal is that gdb and gdbserver share the entire target
>> stack. I think these patches further this goal. I don't find the
>> result harder to read at all.
>
> Doug, are you ok for me to leave it as it is, or, do you have an
> alternative you would like me to implement instead?
Hi. I'm not sure TBH.
I think it might be ok as is, but IWBN to see the full patch with comments.
Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
>
> Gary> +/* See target/symbol.h. */
> Gary> +
> Gary> +int
> Gary> +target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)
>
> I never wrote these docs either...
I've done this:
+struct objfile;
+
+/* Find a symbol that matches NAME. Limit the search to OBJFILE if
+ OBJFILE is non-NULL and the implementation supports limiting the
+ search to specific object files. If a match is found, store the
+ matching symbol's address in ADDR and return nonzero. Return zero
+ if no symbol matching NAME is found. Raise an exception if OBJFILE
+ is non-NULL and the implementation does not support limiting
+ searches to specific object files. */
+
extern int target_look_up_symbol (const char *name, CORE_ADDR *addr,
- void *data);
+ struct objfile *objfile);
Note that I changed the final parameter to struct objfile *objfile and
added a forward declaration which is never filled out in gdbserver and
added an assertion that objfile == NULL in gdbserver. I don't know
whether you will agree with this but I'll change it back if you prefer.
Thanks,
Gary
--
http://gbenson.net/
Doug Evans wrote:
> On Fri, Jul 11, 2014 at 5:56 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Tom Tromey wrote:
> > > Doug> As target/* scales up, is there a risk of the code
> > > Doug> becoming harder to read if target_ is used as a general
> > > Doug> prefix for things in target/*? Dunno. Just wondering.
> > >
> > > My long term goal is that gdb and gdbserver share the entire
> > > target stack. I think these patches further this goal. I don't
> > > find the result harder to read at all.
> >
> > Doug, are you ok for me to leave it as it is, or, do you have an
> > alternative you would like me to implement instead?
>
> Hi. I'm not sure TBH. I think it might be ok as is, but IWBN to
> see the full patch with comments.
In this case, the implementations are in gdb/{,gdbserver/}target.c
so maybe it would make sense to have the prototype in
gdb/target/target.h--though this comes with the caveat that I don't
have a good idea of what sharing the entire target stack will mean
right now. It may be that Tom created the file anticipating other
functions that will go there in future. Doug, Tom, I'd appreciate
both your opinions on this.
Wherever it goes (and whatever it's called) I think there'll always be
a bit of jiggling around with a refactoring of this size and nature.
I think we need to prioritize getting the right subset of code shared
over getting the interface perfect or we risk premature optimization.
Doug, I realize that this paragraph could be read as "I don't share
your concerns" but please don't read it this way! I want a really
nice interface between the shared code (ie common/target/nat) and the
consumers (GDB and gdbserver). I'm anticipating various functions
being moved and/or renamed as the exact subset of code to be shared
becomes clearer.
Thanks,
Gary
>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
Gary> Note that I changed the final parameter to struct objfile *objfile and
Gary> added a forward declaration which is never filled out in gdbserver and
Gary> added an assertion that objfile == NULL in gdbserver. I don't know
Gary> whether you will agree with this but I'll change it back if you prefer.
It seems fine to me.
Tom
>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
Gary> It may be that Tom created the file anticipating other functions
Gary> that will go there in future.
If I remember correctly I think I did it just to try to separate out
different conceptual bits. I don't really care much about it per se, I
just think it would be nice if we could avoid the current problem of
header files full of unsorted goo. Though admittedly a single
declaration in a single file hardly makes a dent.
Tom
@@ -31,6 +31,7 @@
#include "common-types.h"
#include "target/target.h"
+#include "target/symbol.h"
#include "errors.h"
#include "ptid.h"
#include "gdb_locale.h"
@@ -111,18 +112,8 @@ agent_look_up_symbols (void *arg)
{
CORE_ADDR *addrp =
(CORE_ADDR *) ((char *) &ipa_sym_addrs + symbol_list[i].offset);
-#ifdef GDBSERVER
-
- if (look_up_one_symbol (symbol_list[i].name, addrp, 1) == 0)
-#else
- struct bound_minimal_symbol sym =
- lookup_minimal_symbol (symbol_list[i].name, NULL,
- (struct objfile *) arg);
- if (sym.minsym != NULL)
- *addrp = BMSYMBOL_VALUE_ADDRESS (sym);
- else
-#endif
+ if (target_look_up_symbol (symbol_list[i].name, addrp, arg) == 0)
{
DEBUG_AGENT ("symbol `%s' not found\n", symbol_list[i].name);
return -1;
@@ -20,6 +20,7 @@
#include "server.h"
#include "tracepoint.h"
+#include "target/symbol.h"
struct target_ops *the_target;
@@ -168,6 +169,14 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
(*the_target->resume) (&resume_info, 1);
}
+/* See target/symbol.h. */
+
+int
+target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)
+{
+ return look_up_one_symbol (name, addr, 1);
+}
+
int
start_non_stop (int nonstop)
{
@@ -1322,6 +1322,19 @@ target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
return 0;
}
+/* See target/symbol.h. */
+
+int
+target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)
+{
+ struct bound_minimal_symbol sym
+ = lookup_minimal_symbol (name, NULL, (struct objfile *) data);
+
+ if (sym.minsym != NULL)
+ *addr = BMSYMBOL_VALUE_ADDRESS (sym);
+ return sym.minsym != NULL;
+}
+
/* Like target_read_memory, but specify explicitly that this is a read
from the target's raw memory. That is, this read bypasses the
dcache, breakpoint shadowing, etc. */
@@ -62,6 +62,7 @@ struct dcache_struct;
#include "target/resume.h"
#include "target/wait.h"
#include "target/waitstatus.h"
+#include "target/symbol.h"
#include "bfd.h"
#include "symtab.h"
#include "memattr.h"
new file mode 100644
@@ -0,0 +1,26 @@
+/* Declarations of target symbol functions.
+
+ Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef TARGET_SYMBOL_H
+#define TARGET_SYMBOL_H
+
+extern int target_look_up_symbol (const char *name, CORE_ADDR *addr,
+ void *data);
+
+#endif /* TARGET_SYMBOL_H */