[PATCH] Fix CRISv32 compilation

[PATCH] Fix CRISv32 compilation

[Ricard Wanderlof]

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]


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.

gdbserver

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



diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
index 03041e4..dce2724 100644
--- a/gdb/cris-tdep.c
+++ b/gdb/cris-tdep.c
@@ -3820,7 +3820,7 @@ cris_delayed_get_disassembler (bfd_vma addr, struct disassemble_info *info)
   }

   /* Copied from <asm/elf.h>.  */
-typedef unsigned long elf_greg_t;
+typedef uint32_t elf_greg_t;

   /* Same as user_regs_struct struct in <asm/user.h>.  */
   #define CRISV10_ELF_NGREG 35
@@ -4137,6 +4137,9 @@ cris_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
         set_gdbarch_single_step_through_delay
   	(gdbarch, crisv32_single_step_through_delay);

+      set_gdbarch_fetch_tls_load_module_address (gdbarch,
+                                                 svr4_fetch_objfile_link_map);
+
         break;

       default:
diff --git a/gdb/gdbserver/linux-crisv32-low.c b/gdb/gdbserver/linux-crisv32-low.c
index 2849d02..84cb7ff 100644
--- a/gdb/gdbserver/linux-crisv32-low.c
+++ b/gdb/gdbserver/linux-crisv32-low.c
@@ -27,6 +27,10 @@ extern const struct target_desc *tdesc_crisv32;
   /* CRISv32 */
   #define cris_num_regs 49

+#ifndef PTRACE_GET_THREAD_AREA
+#define PTRACE_GET_THREAD_AREA 25
+#endif
+
   /* Note: Ignoring USP (having the stack pointer in two locations causes trouble
      without any significant gain).  */

@@ -339,6 +343,20 @@ cris_stopped_data_address (void)
     return eda;
   }

+ps_err_e
+ps_get_thread_area (const struct ps_prochandle *ph,
+                    lwpid_t lwpid, int idx, void **base)
+{
+  if (ptrace (PTRACE_GET_THREAD_AREA, lwpid, NULL, base) != 0)
+    return PS_ERR;
+
+  /* IDX is the bias from the thread pointer to the beginning of the
+     thread descriptor.  It has to be subtracted due to implementation
+     quirks in libthread_db.  */
+  *base = (void *) ((char *)*base - idx);
+  return PS_OK;
+}
+
   static void
   cris_fill_gregset (struct regcache *regcache, void *buf)
   {


/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch file.patch --]
[-- Type: text/x-diff; name="cris-head-thread.patch", Size: 1934 bytes --]

diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
index 03041e4..dce2724 100644
--- a/gdb/cris-tdep.c
+++ b/gdb/cris-tdep.c
@@ -3820,7 +3820,7 @@ cris_delayed_get_disassembler (bfd_vma addr, struct disassemble_info *info)
 }
 
 /* Copied from <asm/elf.h>.  */
-typedef unsigned long elf_greg_t;
+typedef uint32_t elf_greg_t;
 
 /* Same as user_regs_struct struct in <asm/user.h>.  */
 #define CRISV10_ELF_NGREG 35
@@ -4137,6 +4137,9 @@ cris_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       set_gdbarch_single_step_through_delay 
 	(gdbarch, crisv32_single_step_through_delay);
 
+      set_gdbarch_fetch_tls_load_module_address (gdbarch,
+                                                 svr4_fetch_objfile_link_map);
+
       break;
 
     default:
diff --git a/gdb/gdbserver/linux-crisv32-low.c b/gdb/gdbserver/linux-crisv32-low.c
index 2849d02..84cb7ff 100644
--- a/gdb/gdbserver/linux-crisv32-low.c
+++ b/gdb/gdbserver/linux-crisv32-low.c
@@ -27,6 +27,10 @@ extern const struct target_desc *tdesc_crisv32;
 /* CRISv32 */
 #define cris_num_regs 49
 
+#ifndef PTRACE_GET_THREAD_AREA
+#define PTRACE_GET_THREAD_AREA 25
+#endif
+
 /* Note: Ignoring USP (having the stack pointer in two locations causes trouble
    without any significant gain).  */
 
@@ -339,6 +343,20 @@ cris_stopped_data_address (void)
   return eda;
 }
 
+ps_err_e
+ps_get_thread_area (const struct ps_prochandle *ph,
+                    lwpid_t lwpid, int idx, void **base)
+{
+  if (ptrace (PTRACE_GET_THREAD_AREA, lwpid, NULL, base) != 0)
+    return PS_ERR;
+
+  /* IDX is the bias from the thread pointer to the beginning of the
+     thread descriptor.  It has to be subtracted due to implementation
+     quirks in libthread_db.  */
+  *base = (void *) ((char *)*base - idx);
+  return PS_OK;
+}
+
 static void
 cris_fill_gregset (struct regcache *regcache, void *buf)
 {

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

[Pedro Alves]

(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

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

[Ricard Wanderlof]

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.

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, 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?

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

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

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

[Pedro Alves]

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

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

[Ricard Wanderlof]

On Tue, 3 Sep 2013, Pedro Alves wrote:

>>> 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?
> ...
>> 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,
> ...
> 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.

That's a good point. I'll see if I find some time to make a rudimentary 
split.

Regarding the specific case above, it's a bit odd though that the CRIS 
port was included in the first patch set but not the second, considering 
they were supplied by the same person. Still, it still makes your point.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

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

[Pedro Alves]

On 09/03/2013 03:57 PM, Ricard Wanderlof wrote:
> 
> On Tue, 3 Sep 2013, Pedro Alves wrote:
> 
>>>> 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?
>> ...
>>> 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,
>> ...
>> 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.
> 
> That's a good point. I'll see if I find some time to make a rudimentary 
> split.
> 
> Regarding the specific case above, it's a bit odd though that the CRIS 
> port was included in the first patch set but not the second, considering 
> they were supplied by the same person. Still, it still makes your point.

It's not odd at all.  linux-tdep.c is supposed to be built by all
Linux ports, and it was assumed that all Linux ports are calling
linux-tdep.c:linux_init_abi.  In v2 you see:

"- linux-tdep.c now registers the generic converters itself, thus
  eliminating the need for every target to do so."

Since cris is missing linux-tdep.c, it was left out by accident.

-- 
Pedro Alves