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

[Pedro Alves <palves@redhat.com>]

On 09/03/2013 01:35 PM, Ricard Wanderlof wrote:
> 
> On Mon, 2 Sep 2013, Pedro Alves wrote:
> 
>> (fixing subject)
> 
> Thanks.
> 
>>> 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.
> 
> Thanks for pointing it out. Will fix.
> 
>>> Suggested-by: Edgar Iglisias <edgar.iglesias@gmail.com>
>>
>> (Note: surname/address don't match)
> 
> Thanks for spotting that. Edgar used to work at the same company I do but 
> does not any more so I had to piece this together manually.
> 
>>> 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.
> 
> Sure. That was the intention, like I said the patch as such has been 
> applied locally for some time and I decided to submit it, not really 
> considering all the details.
> 
> I will split this into two patches then.
> 
>> (Note that "fix" is not "what" changed, but "why" you changed it, so
>> it's not the proper ChangeLog description of the change.)
> 
> Ok.
> 
>> 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.
> 
> Ok, I'll do something similar then. I see that MIPS has a similar 
> construct.
> 
>>>   	  (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?
> 
> Virtually all CRIS systems run Linux, and GDB is mostly used for 
> user-space debugging. There is a GDB stub available for the Linux kernel, 
> which I guess would qualify as bare metal debugging, but as far as I know 
> it is virtually never used and the support in the kernel may even be 
> broken by know.

Thanks for the clarification.

> 
> So as you say the file should probably be split, but at this point in time 
> I don't really see the need for it. It doesn't seem like it would be a 
> huge effort (essentially the call to 
> set_gdbarch_fetch_tls_load_module_address and also 
> set_solib_svr4_fetch_link_map_offsets would be put in cris-linux-tdep.c), 
> on the other hand I can't really test that it works as expected, 

I'd be happy enough if the GNU/Linux port keeps working.

> so perhaps the best solution for now is to add a FIXME, and to split the
> files when the need comes up and the result can be tested properly?

It's mostly about code/design/maintenance sanity.  I won't really mind if the
split isn't done, but note how the fact that there's a Linux port here
is being missed often in regular maintenance (because people will look
for *linux-tdep.*) files.  cris-tdep.c doesn't call linux_init_abi anywhere
AFAICT, for example, so the cris port lost the adjustment between v1
and v2 of the gdbarch_gdb_signal_{to,from}_target
patches, just a few weeks back:

 http://sourceware.org/ml/gdb-patches/2013-07/msg00002.html
 http://sourceware.org/ml/gdb-patches/2013-07/msg00651.html

Probably other across-the-board changes have been missed.

> 
>>>
>>> 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,
> 
> Ok, will add that.
> 
>>> +  *base = (void *) ((char *)*base - idx);
>>
>> missing space after '(char *) '.
> 
> Ok, will fix that.
> 
> I'll fix the issues and submit a new patch (two patches).

Thanks.

-- 
Pedro Alves