Re: [RFC-v6] Add windows OS Thread Information Block

[Pedro Alves <pedro@codesourcery.com>]

On Monday 12 April 2010 14:51:27, Pierre Muller wrote:
>         gdb.texinfo ($__tlb): Document new automatic convinience variable.

One _ too much?

>   Seems good to me, as I anyhow don't think that
> Windows OS will ever use '0' as a Thread Information Block address.
> Note that this is only a guess, but it might be required 
> if Windows OS guarantees that the null address is not accessible,
> but I don't know if this is indeed written somewhere in the specs. 

Then don't guess, just remove the check, and we'll add
it later if found necessary.  Otherwise, it's just bloat and
adds to confusion (it makes one stop and wonder when
reading that code; I know I have).


On Monday 12 April 2010 14:51:27, Pierre Muller wrote:
> +  return tib_ptr_type;
> +}
> +/* The $_tlb convenience variable is a bit special.  We don't know

Missing newline between previous function and following function's comment.


Something else I just noticed now:

> +/* Display thread information block of a given thread.  */
> +
> +static int
> +display_one_tib (ptid_t ptid)
> +{
> +#define PTID_STRING_SIZE 40
> +  char annex[PTID_STRING_SIZE];
> +  char *annex_end = annex + PTID_STRING_SIZE;
> +  gdb_byte *tib = NULL;
> +  gdb_byte *index;
> +  CORE_ADDR thread_local_base;
...
> +  if (target_read (&current_target, TARGET_OBJECT_MEMORY,
> +                  annex, tib, thread_local_base, tib_size) != tib_size)

This annex handling also seems something used in a previous
patch revision.  I think you meant to drop it.

Also:

> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_qGetTIBAddr],
> +                        "qGetTIBAddr", "get-thread-information-block-address",
> +                        0);

this new command should be documented.  In addition, all new packets
and commands should be mentioned in NEWS.



> +      if (the_target->get_tib_address != NULL)
> +       strcat (own_buf, ";qGetTIBAddr+");

You missed addressing a comment I made to this:

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.)


Also, you have unrelated hunks touching these files:

 > RCS file: /cvs/src/src/gdb/tui/tui-regs.c,v
 > RCS file: /cvs/src/src/gdb/tui/tui-stack.c,v

please make sure these are not included in the patch.

-- 
Pedro Alves