diff mbox

[12/15] Add target/symbol.h, update users

Message ID 1404902255-11101-13-git-send-email-gbenson@redhat.com
State Changes Requested, archived
Headers show

Commit Message

Gary Benson July 9, 2014, 10:37 a.m. UTC
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

Tom Tromey July 10, 2014, 5:52 p.m. UTC | #1
>>>>> "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
Doug Evans July 10, 2014, 6:55 p.m. UTC | #2
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.
Tom Tromey July 10, 2014, 7:16 p.m. UTC | #3
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
Gary Benson July 11, 2014, 12:56 p.m. UTC | #4
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
Doug Evans July 11, 2014, 6:29 p.m. UTC | #5
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.
Gary Benson July 16, 2014, 10:23 a.m. UTC | #6
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/
Gary Benson July 16, 2014, 10:37 a.m. UTC | #7
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
Tom Tromey July 17, 2014, 4:49 p.m. UTC | #8
>>>>> "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
Tom Tromey July 17, 2014, 4:52 p.m. UTC | #9
>>>>> "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
diff mbox

Patch

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 0fde2fd..798dd1d 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -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;
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index f93163e..58b3d8c 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -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)
 {
diff --git a/gdb/target.c b/gdb/target.c
index 0796059..047f6ec 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -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.  */
diff --git a/gdb/target.h b/gdb/target.h
index 2a4783c..bb776e4 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -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"
diff --git a/gdb/target/symbol.h b/gdb/target/symbol.h
new file mode 100644
index 0000000..3e4591f
--- /dev/null
+++ b/gdb/target/symbol.h
@@ -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 */