Re: [RFA-v7] Add windows OS Thread Information Block

[Pedro Alves <pedro at codesourcery dot com>]

On Tuesday 13 April 2010 09:38:23, Pierre Muller wrote:

> > > +      if (the_target->get_tib_address != NULL)
> > > +       strcat (own_buf, ";qGetTIBAddr+");
> > 
> > You missed addressing a comment I made to this:
> 
>   I guessed that this is the needed counterpart of the fact
> that qGetTIBAddr is in the list of features that can be disabled.

Not really.  Notice how The `qGetTLSAddr' packet, which you're
basing your new packet on, doesn't have this equivalent qSupported
feature, and you can still disable it with the
`set remote get-thread-local-storage-address' command.

> > I can't seem to find where this new qsupported feature
> > was documented in the manual changes?  (and is this needed
> > at all?  I guess it doesn't hurt.)
> 
>   At least, I tested disabling it with 'set remote get-thread-information-block-address off'
> and that worked OK.

See above.  The way it is in your patch currently, this
isn't doing what you think it is.  It's actually doing nothing.
For it to work, you'd need to add a new entry in the `remote_protocol_features'
array, with PACKED_DISABLE as default, so that only when the feature
is reported would this test:

+  if (remote_protocol_packets[PACKET_qGetTIBAddr].support != PACKET_DISABLE)
+    {

fail.

As is, remote_protocol_packets[PACKET_qGetTIBAddr].support starts as
PACKET_SUPPORT_UNKNOWN, and on first use, the packet_ok call below
updates it depending on the remote stub reply:

+      getpkt (&rs->buf, &rs->buf_size, 0);
+      result = packet_ok (rs->buf,
+                         &remote_protocol_packets[PACKET_qGetTIBAddr]);


Again, what I'm saying is: _new qSupported features need to be
documented in the manual_.  If this stays, it needs to be
documented.  But if it stays, it needs to be made to work correctly.
And hence my earlier question: (is this needed at all?).
Please, do try to fully understand all code you write and propose.
I think these hints should get you a good start.

>   Here is one more version.
>   The main change (apart from the suggestions made by Pedro)
> is the addition of a NEWS entry.
> 

> 	*  windows-tdep.h (info_w32_cmdlist): Declare.

             ^  spurious double space.

> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.369
> diff -u -p -r1.369 NEWS
> --- NEWS	9 Apr 2010 15:26:54 -0000	1.369
> +++ NEWS	13 Apr 2010 07:34:59 -0000
> @@ -3,6 +3,14 @@
>  
>  *** Changes since GDB 7.1
>  
> +* New Windows OS feature: Thread Information Block.
> +
> +  - GDB now supports disaplying of thread specific information for
> +  Windows OS executables. This feature is accessible either as a new command
> +  'info w32 thread-information-block' or as a new computed internal variable
> +  named '$_tlb' a thread-specific pointer to the Thread Information Block.
> +  'qGetTIBAddr' is a new remote packet associated with this feature.
> +

Typo "disaplying".  Double space after period.  Note that the GNU
single-quote style is to open a quote with a back-quote (`).  Also, you're
not adding a new feature to the OS.  :-)  May I suggest this instead ? :

Index: src/gdb/NEWS
===================================================================
--- src.orig/gdb/NEWS   2010-04-13 11:34:17.000000000 +0100
+++ src/gdb/NEWS        2010-04-13 11:37:54.000000000 +0100
@@ -3,6 +3,22 @@

 *** Changes since GDB 7.1

+* Windows Thread Information Block access.
+
+  On Windows targets, GDB now supports displaying the Windows Thread
+  Information Block (TIB) structure.  This structure is visible either
+  by using the new command `info w32 thread-information-block' or, by
+  dereferencing the new convenience variable named `$_tlb', a
+  thread-specific pointer to the TIB.  This feature is also supported
+  when remote debugging using GDBserver.
+
+* New remote packets
+
+qGetTIBAddr
+
+  Return the address of the Windows Thread Information Block of the
+  current thread.
+
 * New features in the GDB remote stub, GDBserver

   - GDBserver now support tracepoints.  The feature is currently


>  /* Support for inferring a target description based on the current
>     architecture and the size of a 'g' packet.  While the 'g' packet
>     can have any size (since optional registers can be left off the
> @@ -9890,6 +9934,8 @@ Specify the serial device it is connecte
>    remote_ops.to_set_circular_trace_buffer = remote_set_circular_trace_buffer;
>    remote_ops.to_core_of_thread = remote_core_of_thread;
>    remote_ops.to_verify_memory = remote_verify_memory;
> +  remote_ops.to_get_tib_address = remote_get_tib_address;
> + 

Spurious new line.

>  }


> +/* Provide thread local base, i.e. Thread Information Block address.
> +   Returns 1 if ptid is found and thread_local_base is non zero.  */

You also need to update the comment...

> +
> +static int
> +windows_get_tib_address (ptid_t ptid, CORE_ADDR *addr)
> +{
> +  thread_info *th;
> +
> +  th = thread_rec (ptid_get_tid (ptid), 0);
> +  if (th == NULL)
> +    return 0;
> +
> +  if (addr != NULL)
> +    *addr = th->thread_local_base;
> +
> +  return 1;
> +}


> +static void
> +tlb_value_write (struct value *v, struct value *fromval)
> +{
> +  error (_("Impossible to change tlb"));

This is really a nit, but I suggest

  error (_("Impossible to change the Thread Local Base."));

Or,

  error (_("Impossible to change the TLB."));


> +  tib = alloca (tib_size);
> +
> +  /* This needs to be changed if multi-process support is added.  */

This comment is still here.  I thought you were going to remove it?


> +/* Display thread information block of a thread specified by ARGS.
> +   If ARGS is empty, display thread information block of current_thread
> +   if current_thread is non NULL.
> +   Otherwise ARGS is parsed and converted to a integer that should
> +   be the windows ThreadID (not the internal GDB thread ID).  */
> +static void

While we're at it, add a new line here too, please.

> +display_tib (char * args, int from_tty)


> +  add_setshow_boolean_cmd ("show-all-tib", class_maintenance,
> +			   &maint_display_all_tib, _("\
> +Set whether to display all non-zero fields of thread information block."), _("\
> +Show whether to display all non-zero fields of thread information block."), _("\
> +Use \"on\" to enable, \"off\" to disable.\n\
> +If enabled, all non-zero fields of thread information block are displayed,\n\
> +even if their meaning is unknown."),
> +			   NULL,
> +			   NULL,

If you would please install a real `show' callback instead of this
second NULL, I'd appreciate it.  The reason is that the default callback
when this is left as NULL can't work correctly in non-English.  A goal
is to have no command left with this callback left as NULL.
But if you don't want to do it now (too many iterations already!), it's
quite fine.

> +@item info w32 thread-information-block
> +This command displays thread specific information stored in the
> +Thread Information Block for x86 CPU family (readable using @code{$fs}
> +selector for 32-bit programs and @code{$gs} for 64-bit programs).

Could you instead say:

+Thread Information Block (readable on the X86 CPU family using @code{$fs}
+selector for 32-bit programs and @code{$gs} for 64-bit programs).


> +@kindex maint set show-all-tib
> +@kindex maint show show-all-tib
> +@item maint set show-all-tib
> +@itemx maint show show-all-tib
> +Control whether to show all non zero areas within a 1k block starting
> +at thread local base, when using @samp{info w32 thread-information-block}
> +command.

when using _the_ ... command.


> +@item qGetTIBAddr:@var{thread-id}:

                                    ^

This `:' is spurious; it isn't a part of the packet.  Please remove it.

-- 
Pedro Alves