Re: [PATCH][CRISv32] Add support for threaded debugging

[Pedro Alves <palves@redhat.com>]

(fixing subject)

On 08/30/2013 02:29 PM, Ricard Wanderlof wrote:
> 
> This patch adds support for threaded debugging on the CRISv32
> architecture. It's been floating around for several years now in our local
> repository so it's way overdue pushing upstream.
> 
> Patch included inline for review and as attachement for use.
> 
> 
> Suggested-by: Edgar Iglisias <edgar.iglesias@gmail.com>
> Signed-off-by: Ricard Wanderlof <ricardw@axis.com>
> 
> 
> 2013-08-30  Ricard Wanderlof  <ricardw@axis.com>
> 
>   	* cris-tdep.c: Fix typedef for elf_greg_t.
>   	  (cris_gdbarch_init): Add call to
>   	  set_gdbarch_fetch_tls_load_module_address.

Indentation doesn't look right.  Should be indented
with a single tab.

> Suggested-by: Edgar Iglisias <edgar.iglesias@gmail.com>

(Note: surname/address don't match)

> Signed-off-by: Ricard Wanderlof <ricardw@axis.com>
>
>
> 2013-08-30  Ricard Wanderlof  <ricardw@axis.com>
>
>   	* cris-tdep.c: Fix typedef for elf_greg_t.

This seems to be unrelated to threaded debugging support.
It just looks like a host-dependency fix.  Please keep logically unrelated
patches separate, and always send them as separate threads each with their
own rationale.

(Note that "fix" is not "what" changed, but "why" you changed it, so
it's not the proper ChangeLog description of the change.)

In this case, the fix should just go the extra mile and remove the
reliance on host alignment from this type, that is representing an
external structure.  IOW, that typedef, if any, would better
be:

  typedef gdb_byte cris_elf_greg_t[4];

See frv_linux_supply_gregset for example.

>   	  (cris_gdbarch_init): Add call to
>   	  set_gdbarch_fetch_tls_load_module_address.

This part looks OK, though it did raise some eyebrows to have
GNU/Linux-specific code in cris-tdep.c, rather than in a cris-linux-tdep.c
file.  It seems there's no real support for cris bare-metal debugging?

>
> gdbserver
>
>     	* linux-crisv32-low.c (ps_get_thread_area): New function.

This part is OK, though should mention also the addition of
PTRACE_GET_THREAD_AREA, and,

> +  *base = (void *) ((char *)*base - idx);

missing space after '(char *) '.

-- 
Pedro Alves