[RFC] Generic support for qGetTLSAddr packet

[RFC] Generic support for qGetTLSAddr packet

[Kevin Buettner]

The patch below implements support for the qGetTLSAddr packet.  See:

    http://sources.redhat.com/ml/gdb/2004-11/msg00189.html

This patch also adds a new gdbarch method for fetching the OS / ABI
specific load module parameters.

Still to come are three patches.  They do the following:

    - Add documentation of remote_qGetTLSAddr_load_module_params to
      gdbint.texinfo.
    - Instantiate remote_qGetTLSAddr_load_module_params for GNU/Linux
      running on an i386.
    - Provide an implementation of the qGetTLSAddr packet for
      gdbserver.

The documentation patch for the qGetTLSAddr packet has already been posted.
It may be found at:

    http://sources.redhat.com/ml/gdb-patches/2004-12/msg00168.html

Comments?

	* gdbarch.sh (remote_qGetTLSAddr_load_module_params): New method.
	* gdbarch.c, gdbarch.h: Regenerate.
	* remote.c (remote_protocol_qGetTLSAddr): New static global variable.
	(set_remote_protocol_qGetTLSAddr_packet_cmd)
	(show_remote_protocol_qGetTLSAddr_packet_cmd)
	(remote_get_thread_local_address): New functions.
	(init_all_packet_configs): Initialize remote_protocol_qGetTLSAddr
	variable.
	(init_remote_ops): Initialize ``to_get_thread_local_address'' in
	target vector.
	(show_remote_cmd): Call show_remote_protocol_qGetTLS_Addr_packet_cmd().
	(_initialize_remote):  Add support for the "set remote
	get-thread-local-storage-address-packet' and "show remote
	get-thread-local-address-packet" commands.

Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.352
diff -u -p -r1.352 gdbarch.sh
--- gdbarch.sh	3 Dec 2004 23:59:52 -0000	1.352
+++ gdbarch.sh	6 Dec 2004 20:59:23 -0000
@@ -567,6 +567,9 @@ v:=:CORE_ADDR:decr_pc_after_break:::0:::
 v:=:CORE_ADDR:deprecated_function_start_offset:::0:::0
 
 m::void:remote_translate_xfer_address:struct regcache *regcache, CORE_ADDR gdb_addr, int gdb_len, CORE_ADDR *rem_addr, int *rem_len:regcache, gdb_addr, gdb_len, rem_addr, rem_len::generic_remote_translate_xfer_address::0
+
+# Fill in the additional parameters required to remotely fetch a TLS address
+F:=:int:remote_qGetTLSAddr_load_module_params:ULONGEST **args_ptr, int *argcnt_ptr, struct objfile *objfile:args_ptr, argcnt_ptr, objfile
 #
 v:=:CORE_ADDR:frame_args_skip:::0:::0
 M::CORE_ADDR:unwind_pc:struct frame_info *next_frame:next_frame
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.153
diff -u -p -r1.153 remote.c
--- remote.c	11 Nov 2004 18:59:39 -0000	1.153
+++ remote.c	6 Dec 2004 20:59:24 -0000
@@ -961,6 +961,23 @@ show_remote_protocol_qPart_auxv_packet_c
   show_packet_config_cmd (&remote_protocol_qPart_auxv);
 }
 
+/* Should we try the 'qGetTLSAddr' (Get Thread Local Storage Address) request? */
+static struct packet_config remote_protocol_qGetTLSAddr;
+
+static void
+set_remote_protocol_qGetTLSAddr_packet_cmd (char *args, int from_tty,
+				  struct cmd_list_element *c)
+{
+  update_packet_config (&remote_protocol_qGetTLSAddr);
+}
+
+static void
+show_remote_protocol_qGetTLSAddr_packet_cmd (char *args, int from_tty,
+					 struct cmd_list_element *c)
+{
+  show_packet_config_cmd (&remote_protocol_qGetTLSAddr);
+}
+
 static struct packet_config remote_protocol_p;
 
 static void
@@ -2067,6 +2084,7 @@ init_all_packet_configs (void)
      downloading. */
   update_packet_config (&remote_protocol_binary_download);
   update_packet_config (&remote_protocol_qPart_auxv);
+  update_packet_config (&remote_protocol_qGetTLSAddr);
 }
 
 /* Symbol look-up. */
@@ -5233,6 +5251,52 @@ remote_pid_to_str (ptid_t ptid)
   return buf;
 }
 
+/* Get the address of the thread local variable in OBJFILE which is
+   stored at OFFSET within the thread local storage for thread PTID.  */
+
+static CORE_ADDR
+remote_get_thread_local_address (ptid_t ptid, struct objfile *objfile,
+				 CORE_ADDR offset)
+{
+  if (remote_protocol_qGetTLSAddr.support != PACKET_DISABLE
+      && gdbarch_remote_qGetTLSAddr_load_module_params_p (current_gdbarch))
+    {
+      struct remote_state *rs = get_remote_state ();
+      char *buf = alloca (rs->remote_packet_size);
+      char *p = buf;
+      int status, argcnt;
+      ULONGEST *extra_args;
+
+      strcpy (p, "qGetTLSAddr:");
+      p += strlen (p);
+      p += hexnumstr (p, PIDGET (ptid));
+      *p++ = ',';
+      p += hexnumstr (p, offset);
+      status = gdbarch_remote_qGetTLSAddr_load_module_params
+                 (current_gdbarch, &extra_args, &argcnt, objfile);
+      if (status)
+	{
+	  int i;
+	  for (i = 0; i < argcnt; i++)
+	    {
+	      *p++ = ',';
+	      p += hexnumstr (p, extra_args[i]);
+	    }
+
+	  putpkt (buf);
+	  getpkt (buf, rs->remote_packet_size, 0);
+	  if (packet_ok (buf, &remote_protocol_qGetTLSAddr) == PACKET_OK)
+	    {
+	      ULONGEST result;
+
+	      unpack_varlen_hex (buf, &result);
+	      return result;
+	    }
+	}
+    }
+  error ("Cannot find thread-local values on this target.");
+}
+
 static void
 init_remote_ops (void)
 {
@@ -5272,6 +5336,7 @@ Specify the serial device it is connecte
   remote_ops.to_stop = remote_stop;
   remote_ops.to_xfer_partial = remote_xfer_partial;
   remote_ops.to_rcmd = remote_rcmd;
+  remote_ops.to_get_thread_local_address = remote_get_thread_local_address;
   remote_ops.to_stratum = process_stratum;
   remote_ops.to_has_all_memory = 1;
   remote_ops.to_has_memory = 1;
@@ -5444,6 +5509,7 @@ show_remote_cmd (char *args, int from_tt
   show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_binary_download_cmd (args, from_tty, NULL);
   show_remote_protocol_qPart_auxv_packet_cmd (args, from_tty, NULL);
+  show_remote_protocol_qGetTLSAddr_packet_cmd (args, from_tty, NULL);
 }
 
 static void
@@ -5685,6 +5751,13 @@ in a memory packet.\n",
 			 &remote_set_cmdlist, &remote_show_cmdlist,
 			 0);
 
+  add_packet_config_cmd (&remote_protocol_qGetTLSAddr,
+			 "qGetTLSAddr", "get-thread-local-storage-address",
+			 set_remote_protocol_qGetTLSAddr_packet_cmd,
+			 show_remote_protocol_qGetTLSAddr_packet_cmd,
+			 &remote_set_cmdlist, &remote_show_cmdlist,
+			 0);
+
   /* Keep the old ``set remote Z-packet ...'' working. */
   add_setshow_auto_boolean_cmd ("Z-packet", class_obscure,
 				&remote_Z_packet_detect, "\

Re: [RFC] Generic support for qGetTLSAddr packet

[Mark Kettenis]

   Date: Mon, 6 Dec 2004 14:31:09 -0700
   From: Kevin Buettner <kevinb@redhat.com>

Since you asked for comments ;-):

Yikes!  That qGetTLSAddr in the function name is really ugly.

That said, I don't understand why there's any need for the remote code
to get so deep into the core GDB code.  I don't see the big picture
yet, but my initial reaction is that this must be wrong.  Why does the
remote protocol need to know more than a native GDB?

   The patch below implements support for the qGetTLSAddr packet.  See:

       http://sources.redhat.com/ml/gdb/2004-11/msg00189.html

   This patch also adds a new gdbarch method for fetching the OS / ABI
   specific load module parameters.

I don't think this should be added to the "glibal" gdbbarch vector.
Instead, this probably should be architecture-dependent data that's
only known to the remote protocol module.

Mark

Re: [RFC] Generic support for qGetTLSAddr packet

[Daniel Jacobowitz]

On Mon, Dec 06, 2004 at 11:37:18PM +0100, Mark Kettenis wrote:
> That said, I don't understand why there's any need for the remote code
> to get so deep into the core GDB code.  I don't see the big picture
> yet, but my initial reaction is that this must be wrong.  Why does the
> remote protocol need to know more than a native GDB?

Because shared libraries are handled by GDB, and not by gdbserver. 
Take a look at thread_db_get_thread_local_address; there's a call
to svr4_fetch_objfile_link_map in it, which gdbserver can't do
(unless we were to add ELF header support to it... yuck).

That said, I wonder if this query should be handled uniquely by
remote.c, or by GNU/Linux specific code using an xfer-partial
mechanism.  I haven't thought about the details yet.

-- 
Daniel Jacobowitz

Re: [RFC] Generic support for qGetTLSAddr packet

[Kevin Buettner]

On Mon, 6 Dec 2004 23:37:18 +0100 (CET)
Mark Kettenis <kettenis@gnu.org> wrote:

> Yikes!  That qGetTLSAddr in the function name is really ugly.

Yeah, it is.

Okay, how about something like

    get_remote_TLS_load_module_params

?

> That said, I don't understand why there's any need for the remote code
> to get so deep into the core GDB code.  I don't see the big picture
> yet, but my initial reaction is that this must be wrong.  Why does the
> remote protocol need to know more than a native GDB?

A thread local address is requested via to_get_thread_local_address().
Here's the comment / declaration from target.h:

    /* Return the thread-local address at OFFSET in the
       thread-local storage for the thread PTID and the shared library
       or executable file given by OBJFILE.  If that block of
       thread-local storage hasn't been allocated yet, this function
       may return an error.  */
    CORE_ADDR (*to_get_thread_local_address) (ptid_t ptid,
					      struct objfile *objfile,
					      CORE_ADDR offset);

Abstracting this only a little bit, the following pieces of information
are needed:

    1) The thread id associated with the thread local storage.  Native
       GDB gets this from ``ptid''.
    2) The load module containing the thread local storage.  Native GDB
       gets this by examining ``objfile''.
    3) The offset provided by the debug information.  (Provided by
       ``offset''.)

(1) and (3) are provided to the stub in an unsurprising fashion.

(2), the load module, is represented by an objfile which is a struct
(pointer) The gdbarch method (whose name you object to) provides a
mechanism by which suitable portions of the objfile struct may be
extracted / translated for benefit of the stub.  As Daniel indicates,
for GNU/Linux, the crucial bit of translation needed by the stub is
the mapping from objfile to a linkmap address.  (This is something that
it would be difficult for the stub to do on its own.) However, this
link map address is simply another representation of the load module,
albeit in a form that's readily usable by the stub.

>    The patch below implements support for the qGetTLSAddr packet.  See:
> 
>        http://sources.redhat.com/ml/gdb/2004-11/msg00189.html
> 
>    This patch also adds a new gdbarch method for fetching the OS / ABI
>    specific load module parameters.
> 
> I don't think this should be added to the "glibal" gdbbarch vector.
> Instead, this probably should be architecture-dependent data that's
> only known to the remote protocol module.

Yeah, this could be done.  I'll look to see what the implications are
of doing it this way...

Kevin

Re: [RFC] Generic support for qGetTLSAddr packet

[Kevin Buettner]

On Mon, 6 Dec 2004 18:03:55 -0500
Daniel Jacobowitz <drow@false.org> wrote:

> On Mon, Dec 06, 2004 at 11:37:18PM +0100, Mark Kettenis wrote:
> > That said, I don't understand why there's any need for the remote code
> > to get so deep into the core GDB code.  I don't see the big picture
> > yet, but my initial reaction is that this must be wrong.  Why does the
> > remote protocol need to know more than a native GDB?
> 
> Because shared libraries are handled by GDB, and not by gdbserver. 
> Take a look at thread_db_get_thread_local_address; there's a call
> to svr4_fetch_objfile_link_map in it, which gdbserver can't do
> (unless we were to add ELF header support to it... yuck).

Yes, this is exactly right.

> That said, I wonder if this query should be handled uniquely by
> remote.c, or by GNU/Linux specific code using an xfer-partial
> mechanism.  I haven't thought about the details yet.

I've given this some thought since yesterday.

This approach would define a new enum target_object (maybe
TARGET_OBJECT_TLS_ADDR) and use to_xfer_partial() to handle the
lookup of thread local addresses.  The target method
to_get_thread_local_address() would be eliminated entirely.

As I indicated in my reply to Mark yesterday, we need three
things to fetch a thread-local address.  We need a thread id,
an offset (which is supplied by the debug information), and
some representation of the load module.

The interface for to_xfer_partial is as follows:

    LONGEST (*to_xfer_partial) (struct target_ops *ops,
				enum target_object object, const char *annex,
				void *readbuf, const void *writebuf,
				ULONGEST offset, LONGEST len);

This interface does not make it easy to pass the three TLS related
parameters noted above.  One of them (probably the TLS offset) could
be passed as to_xfer_partial's ``offset'' parameter.  The other two
would have to be encoded in some way and passed as ``annex''.  Each
implementation of to_xfer_partial() would have to implement some
decoding logic in order make use of the two encoded parameters.

Using to_xfer_partial() doesn't really solve the remote protocol
problem either.  remote.c would still need to detect
TARGET_OBJECT_TLS_ADDR and construct a suitable qPart packet.

The problem of the stub not having the load module representation
in an easy to use form would still exist too.

The only advantage that I can think of with this approach is that it
reduces the number of target_ops methods.  (And I don't find this to
be all that compelling.)  The disadvantage is that it complicates the
encoding and decoding of the parameters necessary for fetching a TLS
address.  As noted above, it doesn't help one way or another with
encoding a suitable packet for the remote protocol.  In order to avoid
putting an undue burden on the stub, we'd still need much of the
machinery from my current patch proposal.

So, unless I completely misunderstood what you had in mind, I'm not
in favor of the to_xfer_partial approach.

Kevin

Re: [RFC] Generic support for qGetTLSAddr packet

[Kevin Buettner]

On Tue, 7 Dec 2004 17:00:50 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> On Mon, 6 Dec 2004 18:03:55 -0500
> Daniel Jacobowitz <drow@false.org> wrote:
> 
> > On Mon, Dec 06, 2004 at 11:37:18PM +0100, Mark Kettenis wrote:
> > > That said, I don't understand why there's any need for the remote code
> > > to get so deep into the core GDB code.  I don't see the big picture
> > > yet, but my initial reaction is that this must be wrong.  Why does the
> > > remote protocol need to know more than a native GDB?
> > 
> > Because shared libraries are handled by GDB, and not by gdbserver. 
> > Take a look at thread_db_get_thread_local_address; there's a call
> > to svr4_fetch_objfile_link_map in it, which gdbserver can't do
> > (unless we were to add ELF header support to it... yuck).
> 
> Yes, this is exactly right.
> 
> > That said, I wonder if this query should be handled uniquely by
> > remote.c, or by GNU/Linux specific code using an xfer-partial
> > mechanism.  I haven't thought about the details yet.
> 
> I've given this some thought since yesterday.
> 
> This approach would define a new enum target_object (maybe
> TARGET_OBJECT_TLS_ADDR) and use to_xfer_partial() to handle the
> lookup of thread local addresses.  The target method
> to_get_thread_local_address() would be eliminated entirely.
> 
> As I indicated in my reply to Mark yesterday, we need three
> things to fetch a thread-local address.  We need a thread id,
> an offset (which is supplied by the debug information), and
> some representation of the load module.
> 
> The interface for to_xfer_partial is as follows:
> 
>     LONGEST (*to_xfer_partial) (struct target_ops *ops,
> 				enum target_object object, const char *annex,
> 				void *readbuf, const void *writebuf,
> 				ULONGEST offset, LONGEST len);
> 
> This interface does not make it easy to pass the three TLS related
> parameters noted above.  One of them (probably the TLS offset) could
> be passed as to_xfer_partial's ``offset'' parameter.  The other two
> would have to be encoded in some way and passed as ``annex''.  Each
> implementation of to_xfer_partial() would have to implement some
> decoding logic in order make use of the two encoded parameters.
> 
> Using to_xfer_partial() doesn't really solve the remote protocol
> problem either.  remote.c would still need to detect
> TARGET_OBJECT_TLS_ADDR and construct a suitable qPart packet.
> 
> The problem of the stub not having the load module representation
> in an easy to use form would still exist too.
> 
> The only advantage that I can think of with this approach is that it
> reduces the number of target_ops methods.  (And I don't find this to
> be all that compelling.)  The disadvantage is that it complicates the
> encoding and decoding of the parameters necessary for fetching a TLS
> address.  As noted above, it doesn't help one way or another with
> encoding a suitable packet for the remote protocol.  In order to avoid
> putting an undue burden on the stub, we'd still need much of the
> machinery from my current patch proposal.
> 
> So, unless I completely misunderstood what you had in mind, I'm not
> in favor of the to_xfer_partial approach.

It's just occurred to me that the encoding of thread-id and
load-module into annex could also handle any necessary OS/ABI related
translations of the load module.  This means that that we wouldn't
need two separate code paths which both call
svr4_fetch_objfile_link_map().

Of course, the load module translation could also be done (assuming an
interface change) prior to the call to_get_thread_local_address(). 
I think this would address both Mark's and Andrew's comments about
remote.c doing somewhat more than expected.

I'll mull it over some more...

Kevin

Re: [RFC] Generic support for qGetTLSAddr packet

[Daniel Jacobowitz]

On Tue, Dec 07, 2004 at 05:24:10PM -0700, Kevin Buettner wrote:
> > So, unless I completely misunderstood what you had in mind, I'm not
> > in favor of the to_xfer_partial approach.

I thought about it some more also; I think your analysis was right,
there's not much to gain here.  Except... (see bottom).

> It's just occurred to me that the encoding of thread-id and
> load-module into annex could also handle any necessary OS/ABI related
> translations of the load module.  This means that that we wouldn't
> need two separate code paths which both call
> svr4_fetch_objfile_link_map().
> 
> Of course, the load module translation could also be done (assuming an
> interface change) prior to the call to_get_thread_local_address(). 
> I think this would address both Mark's and Andrew's comments about
> remote.c doing somewhat more than expected.
> 
> I'll mull it over some more...

Another thing I was thinking about was the usefulness of
target-agnostic code for this - I think, very small.  We know what the
get-tls-address operation looks like for GNU/Linux, but not (at least
not I) for any other system.  Presumably there's some way to do this on
Solaris too.

Pending more systems with similar problems, it would be nice if we
could organize all the logic into target-specific code.  Here's where
using target_xfer_partial would be convenient.  I visualize something
like this:

  linux-nat.c provides to_xfer_partial, which can provide X

  linux-tdep.c overrides remote.c's to_xfer_partial (this would
  require a bit of fiddling - I think the best way would be to steer
  clear of target inheritance, and use a gdbarch hook called from
  the remote.c version), with another method for providing X

What the arguments to X should look like, I'm not sure.  It would be
nice to pass the objfile directly, but the to_xfer_partial interface is
not friendly to that; it would be logical to pass the link map address,
but that breaks the abstraction.

Hmm....

-- 
Daniel Jacobowitz

Re: [RFC] Generic support for qGetTLSAddr packet

[Andrew Cagney]

Kevin Buettner wrote:
> The patch below implements support for the qGetTLSAddr packet.  See:
> 
>     http://sources.redhat.com/ml/gdb/2004-11/msg00189.html
> 
> This patch also adds a new gdbarch method for fetching the OS / ABI
> specific load module parameters.
> 
> Still to come are three patches.  They do the following:
> 
>     - Add documentation of remote_qGetTLSAddr_load_module_params to
>       gdbint.texinfo.
>     - Instantiate remote_qGetTLSAddr_load_module_params for GNU/Linux
>       running on an i386.
>     - Provide an implementation of the qGetTLSAddr packet for
>       gdbserver.

Kevin, check my original comments about the protocol.  I identified what 
you've hit here - having to add extra architecture methods.

inferior.get_thread_local_address should take the raw address (to match 
the packet) and not the objfile parameter.  The thread code can then 
extract the necessary address and call the target vector with it.  Of 
course the linux nat code would also need to be adjusted.

Consider the get_thread_local_address interface change pre-approved.

Andrew

PS: I'd stick with the qGetTLSAddr function name, it's consistent with 
the rest of remote.c where the corresponding packet name has been used, 
and makes tracking down the function much simpler.