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

Pedro Alves
2013-09-03T14:32:47Z

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