[RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Luis Machado]

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

Hi,

uClibc-based targets can load their programs in an offset in memory, and 
this information has historically been communicated to gdbserver via 
ptrace with the following options: PT_TEXT_ADDR, PT_DATA_ADDR and 
PT_TEXT_END_ADDR.

We have a target that uses loadmaps as opposed to the above mechanism. 
It is just another ptrace request, but it doesn't use linux_read_offsets 
at all.

linux_read_offsets is always defined (for uClibc-based targets) though, 
so gdbserver eventually calls it and it obviously returns an error. This 
error is propagated all the way to GDB, displaying an alarming and 
cryptic warning on the host's side.

"warning: Remote failure reply: E01"

Though the warning is harmless, the handling of this scenario could be 
improved a little.

The following patch conditionally defines linux_read_offsets only for 
targets that are uClibc-based and that define PT_TEXT_ADDR, PT_DATA_ADDR 
and PT_TEXT_END_ADDR.

Targets using other mechanisms won't define this function then, making 
gdbserver return an empty response to GDB (meaning packet not 
supported). GDB is happy again.

Additionally, i see 3 different archs defining local constants. coldfire 
and c6x still seem to lack definitions in the kernel, but blackfin 
already has those.

I've asked Mike Frysinger whether these can be removed. If so, i'll 
update the patch.

Alternativelly, we could forward the burden of fetching offsets to the 
target backends, though the number of targets that would use this is 
quite limited.

Luis

[-- Attachment #2: 0001-qoffsets_uclinux.diff --]
[-- Type: text/x-patch, Size: 3672 bytes --]

2013-05-15  Luis Machado  <lgustavo@codesourcery.com>

	* linux-low.c: Move definition checks upwards for PT_TEXT_ADDR,
	PT_DATA_ADDR and PT_TEXT_END_ADDR. Update comments.
	(linux_read_offsets): Remove PT_TEXT_ADDR, PT_DATA_ADDR and
	PT_TEXT_END_ADDR guards. Update comments.
	(linux_target_op) <read_offsets): Conditionally define to
	linux_read_offsets if the target is UCLIBC and if it defines
	PT_TEXT_ADDR, PT_DATA_ADDR and PT_TEXT_END_ADDR.

Index: gdb-head/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-head.orig/gdb/gdbserver/linux-low.c	2013-04-17 10:24:52.859499119 +0200
+++ gdb-head/gdb/gdbserver/linux-low.c	2013-05-15 12:22:04.141660168 +0200
@@ -84,6 +84,30 @@
 #endif
 #endif
 
+/* Some targets did not define these ptrace constants from the start,
+   so gdbserver defines them locally here.  In the future, these may
+   be removed after they are added to asm/ptrace.h.  */
+#if !(defined(PT_TEXT_ADDR) \
+      || defined(PT_DATA_ADDR) \
+      || defined(PT_TEXT_END_ADDR))
+#if defined(__mcoldfire__)
+/* These are still undefined in recent (3.10) kernels.  */
+#define PT_TEXT_ADDR 49*4
+#define PT_DATA_ADDR 50*4
+#define PT_TEXT_END_ADDR  51*4
+/* BFIN already defines these constants in recent (3.10) kernels.  */
+#elif defined(BFIN)
+#define PT_TEXT_ADDR 220
+#define PT_TEXT_END_ADDR 224
+#define PT_DATA_ADDR 228
+/* These are still undefined in recent (3.10) kernels.  */
+#elif defined(__TMS320C6X__)
+#define PT_TEXT_ADDR     (0x10000*4)
+#define PT_DATA_ADDR     (0x10004*4)
+#define PT_TEXT_END_ADDR (0x10008*4)
+#endif
+#endif
+
 #ifdef HAVE_LINUX_BTRACE
 # include "linux-btrace.h"
 #endif
@@ -4833,25 +4857,14 @@ linux_stopped_data_address (void)
   return lwp->stopped_data_address;
 }
 
-#if defined(__UCLIBC__) && defined(HAS_NOMMU)
-#if ! (defined(PT_TEXT_ADDR) \
-       || defined(PT_DATA_ADDR) \
-       || defined(PT_TEXT_END_ADDR))
-#if defined(__mcoldfire__)
-/* These should really be defined in the kernel's ptrace.h header.  */
-#define PT_TEXT_ADDR 49*4
-#define PT_DATA_ADDR 50*4
-#define PT_TEXT_END_ADDR  51*4
-#elif defined(BFIN)
-#define PT_TEXT_ADDR 220
-#define PT_TEXT_END_ADDR 224
-#define PT_DATA_ADDR 228
-#elif defined(__TMS320C6X__)
-#define PT_TEXT_ADDR     (0x10000*4)
-#define PT_DATA_ADDR     (0x10004*4)
-#define PT_TEXT_END_ADDR (0x10008*4)
-#endif
-#endif
+#if defined(__UCLIBC__) && defined(HAS_NOMMU)	      \
+    && defined(PT_TEXT_ADDR) && defined(PT_DATA_ADDR) \
+    && defined(PT_TEXT_END_ADDR)
+
+/* This is only used for targets that define PT_TEXT_ADDR,
+   PT_DATA_ADDR and PT_TEXT_END_ADDR.  If those are not defined, supposedly
+   the target has different ways of acquiring this information, like
+   loadmaps.  */
 
 /* Under uClinux, programs are loaded at non-zero offsets, which we need
    to tell gdb about.  */
@@ -4859,7 +4872,6 @@ linux_stopped_data_address (void)
 static int
 linux_read_offsets (CORE_ADDR *text_p, CORE_ADDR *data_p)
 {
-#if defined(PT_TEXT_ADDR) && defined(PT_DATA_ADDR) && defined(PT_TEXT_END_ADDR)
   unsigned long text, text_end, data;
   int pid = lwpid_of (get_thread_lwp (current_inferior));
 
@@ -4888,7 +4900,6 @@ linux_read_offsets (CORE_ADDR *text_p, C
 
       return 1;
     }
-#endif
  return 0;
 }
 #endif
@@ -5887,7 +5898,9 @@ static struct target_ops linux_target_op
   linux_remove_point,
   linux_stopped_by_watchpoint,
   linux_stopped_data_address,
-#if defined(__UCLIBC__) && defined(HAS_NOMMU)
+#if defined(__UCLIBC__) && defined(HAS_NOMMU)	      \
+    && defined(PT_TEXT_ADDR) && defined(PT_DATA_ADDR) \
+    && defined(PT_TEXT_END_ADDR)
   linux_read_offsets,
 #else
   NULL,

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Pedro Alves]

On 05/15/2013 12:25 PM, Luis Machado wrote:

> 	* linux-low.c: Move definition checks upwards for PT_TEXT_ADDR,
> 	PT_DATA_ADDR and PT_TEXT_END_ADDR. Update comments.
> 	(linux_read_offsets): Remove PT_TEXT_ADDR, PT_DATA_ADDR and
> 	PT_TEXT_END_ADDR guards. Update comments.
> 	(linux_target_op) <read_offsets): Conditionally define to
> 	linux_read_offsets if the target is UCLIBC and if it defines
> 	PT_TEXT_ADDR, PT_DATA_ADDR and PT_TEXT_END_ADDR.

This is OK.

> +#if defined(__mcoldfire__)
> +/* These are still undefined in recent (3.10) kernels.  */
> +#define PT_TEXT_ADDR 49*4
> +#define PT_DATA_ADDR 50*4
> +#define PT_TEXT_END_ADDR  51*4
> +/* BFIN already defines these constants in recent (3.10) kernels.  */
> +#elif defined(BFIN)
> +#define PT_TEXT_ADDR 220
> +#define PT_TEXT_END_ADDR 224
> +#define PT_DATA_ADDR 228
> +/* These are still undefined in recent (3.10) kernels.  */

But please avoid "recent" "new", etc. in code.  Those get old fast.

Thanks,
-- 
Pedro Alves

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Yao Qi]

Luis,
I went through the patch, and didn't see something wrong.  Some nits on 
changelog.

On 05/15/2013 07:25 PM, Luis Machado wrote:
>
> 2013-05-15  Luis Machado<lgustavo@codesourcery.com>
>
> 	* linux-low.c: Move definition checks upwards for PT_TEXT_ADDR,
> 	PT_DATA_ADDR and PT_TEXT_END_ADDR. Update comments.
                                           ^^  Two spaces after ".".

> 	(linux_read_offsets): Remove PT_TEXT_ADDR, PT_DATA_ADDR and
> 	PT_TEXT_END_ADDR guards. Update comments.
                                 ^ Likewise.

> 	(linux_target_op) <read_offsets): Conditionally define to
                                        ^ ")" -> ">"

> 	linux_read_offsets if the target is UCLIBC and if it defines
> 	PT_TEXT_ADDR, PT_DATA_ADDR and PT_TEXT_END_ADDR.


-- 
Yao (齐尧)

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 881 bytes --]

On Wednesday 15 May 2013 07:25:34 Luis Machado wrote:
> uClibc-based targets can load their programs in an offset in memory, and
> this information has historically been communicated to gdbserver via
> ptrace with the following options: PT_TEXT_ADDR, PT_DATA_ADDR and
> PT_TEXT_END_ADDR.

well, not to be pedantic, but this is for FLAT programs, not uClibc

> We have a target that uses loadmaps as opposed to the above mechanism.
> It is just another ptrace request, but it doesn't use linux_read_offsets
> at all.

you mean FDPIC ?  gdb already supports that and uses a different set of ptrace 
requests for that.  ideally, gdb nor gdbserver should not be tied to a specific 
file format (what format it happened to be compiled for).  instead, gdbserver 
should support all formats and then gdb detects the format and changes its 
requests based on that.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Luis Machado]

On 05/15/2013 06:06 PM, Mike Frysinger wrote:
> On Wednesday 15 May 2013 07:25:34 Luis Machado wrote:
>> uClibc-based targets can load their programs in an offset in memory, and
>> this information has historically been communicated to gdbserver via
>> ptrace with the following options: PT_TEXT_ADDR, PT_DATA_ADDR and
>> PT_TEXT_END_ADDR.
>
> well, not to be pedantic, but this is for FLAT programs, not uClibc

Ok. uClibc has been used here due to its gdbserver-specific #if guard 
explicitly checking for UCLIBC and mmu-lessness.

>> We have a target that uses loadmaps as opposed to the above mechanism.
>> It is just another ptrace request, but it doesn't use linux_read_offsets
>> at all.
>
> you mean FDPIC ?  gdb already supports that and uses a different set of ptrace
> requests for that.  ideally, gdb nor gdbserver should not be tied to a specific
> file format (what format it happened to be compiled for).  instead, gdbserver
> should support all formats and then gdb detects the format and changes its
> requests based on that.

Not FDPIC, but DSBT. I agree gdb/gdbserver should be format-agnostic, 
but it grew like this. Let's not extend the uglyness though.

I have a change coming that will deal with some of the differences when 
handling FDPIC and DSBT, hopefully making things more generic.

Luis

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1501 bytes --]

On Wednesday 15 May 2013 12:26:06 Luis Machado wrote:
> On 05/15/2013 06:06 PM, Mike Frysinger wrote:
> > On Wednesday 15 May 2013 07:25:34 Luis Machado wrote:
> >> uClibc-based targets can load their programs in an offset in memory, and
> >> this information has historically been communicated to gdbserver via
> >> ptrace with the following options: PT_TEXT_ADDR, PT_DATA_ADDR and
> >> PT_TEXT_END_ADDR.
> > 
> > well, not to be pedantic, but this is for FLAT programs, not uClibc
> 
> Ok. uClibc has been used here due to its gdbserver-specific #if guard
> explicitly checking for UCLIBC and mmu-lessness.

because no other C lib supports FLAT currently :)

> >> We have a target that uses loadmaps as opposed to the above mechanism.
> >> It is just another ptrace request, but it doesn't use linux_read_offsets
> >> at all.
> > 
> > you mean FDPIC ?  gdb already supports that and uses a different set of
> > ptrace requests for that.  ideally, gdb nor gdbserver should not be tied
> > to a specific file format (what format it happened to be compiled for). 
> > instead, gdbserver should support all formats and then gdb detects the
> > format and changes its requests based on that.
> 
> Not FDPIC, but DSBT. I agree gdb/gdbserver should be format-agnostic,
> but it grew like this. Let's not extend the uglyness though.

i thought someone already committed support for DSBT, and i helped merge some 
of the FDPIC differences.  it was for the c6x port iirc.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Luis Machado]

On 05/15/2013 07:12 PM, Mike Frysinger wrote:
> On Wednesday 15 May 2013 12:26:06 Luis Machado wrote:
>> On 05/15/2013 06:06 PM, Mike Frysinger wrote:
>>> On Wednesday 15 May 2013 07:25:34 Luis Machado wrote:
>>>> uClibc-based targets can load their programs in an offset in memory, and
>>>> this information has historically been communicated to gdbserver via
>>>> ptrace with the following options: PT_TEXT_ADDR, PT_DATA_ADDR and
>>>> PT_TEXT_END_ADDR.
>>>
>>> well, not to be pedantic, but this is for FLAT programs, not uClibc
>>
>> Ok. uClibc has been used here due to its gdbserver-specific #if guard
>> explicitly checking for UCLIBC and mmu-lessness.
>
> because no other C lib supports FLAT currently :)

Ah, that explains it. :-)

>>>> We have a target that uses loadmaps as opposed to the above mechanism.
>>>> It is just another ptrace request, but it doesn't use linux_read_offsets
>>>> at all.
>>>
>>> you mean FDPIC ?  gdb already supports that and uses a different set of
>>> ptrace requests for that.  ideally, gdb nor gdbserver should not be tied
>>> to a specific file format (what format it happened to be compiled for).
>>> instead, gdbserver should support all formats and then gdb detects the
>>> format and changes its requests based on that.
>>
>> Not FDPIC, but DSBT. I agree gdb/gdbserver should be format-agnostic,
>> but it grew like this. Let's not extend the uglyness though.
>
> i thought someone already committed support for DSBT, and i helped merge some
> of the FDPIC differences.  it was for the c6x port iirc.

That is correct, but there are a few differences in the loadmap format 
between targets.  My idea is to clean that up and make it more generic 
without having to use #if blocks inside linux-low.c.

Luis

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 2054 bytes --]

On Wednesday 15 May 2013 14:07:31 Luis Machado wrote:
> On 05/15/2013 07:12 PM, Mike Frysinger wrote:
> > On Wednesday 15 May 2013 12:26:06 Luis Machado wrote:
> >> On 05/15/2013 06:06 PM, Mike Frysinger wrote:
> >>> On Wednesday 15 May 2013 07:25:34 Luis Machado wrote:
> >>>> We have a target that uses loadmaps as opposed to the above mechanism.
> >>>> It is just another ptrace request, but it doesn't use
> >>>> linux_read_offsets at all.
> >>> 
> >>> you mean FDPIC ?  gdb already supports that and uses a different set of
> >>> ptrace requests for that.  ideally, gdb nor gdbserver should not be
> >>> tied to a specific file format (what format it happened to be compiled
> >>> for). instead, gdbserver should support all formats and then gdb
> >>> detects the format and changes its requests based on that.
> >> 
> >> Not FDPIC, but DSBT. I agree gdb/gdbserver should be format-agnostic,
> >> but it grew like this. Let's not extend the uglyness though.
> > 
> > i thought someone already committed support for DSBT, and i helped merge
> > some of the FDPIC differences.  it was for the c6x port iirc.
> 
> That is correct, but there are a few differences in the loadmap format
> between targets.  My idea is to clean that up and make it more generic
> without having to use #if blocks inside linux-low.c.

as long as the functionality isn't based on the format gdbserver itself is 
compiled as, sounds fine.  i believe we cleaned up all the DSBT logic so that 
it isn't fighting with FLAT (i know on Blackfin we have a single gdbserver that 
can support FLAT or FDPIC).

glancing at the code, we do fail slightly in that it's currently DSBT||FDPIC, 
but in practice that hasn't mattered as no one has implemented both formats 
for the same architecture.

personally, i'd be interested in why DSBT was conceived in the first place 
instead of using the existing FDPIC.  are the minor differences on purpose ?  
by accident ?  or why people are using a different format from either DSBT or 
FDPIC ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC, gdbserver] Avoid defining linux_read_offsets when the target does not need it

[Luis Machado]

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

On 05/15/2013 04:14 PM, Pedro Alves wrote:
> On 05/15/2013 12:25 PM, Luis Machado wrote:
>
>> 	* linux-low.c: Move definition checks upwards for PT_TEXT_ADDR,
>> 	PT_DATA_ADDR and PT_TEXT_END_ADDR. Update comments.
>> 	(linux_read_offsets): Remove PT_TEXT_ADDR, PT_DATA_ADDR and
>> 	PT_TEXT_END_ADDR guards. Update comments.
>> 	(linux_target_op) <read_offsets): Conditionally define to
>> 	linux_read_offsets if the target is UCLIBC and if it defines
>> 	PT_TEXT_ADDR, PT_DATA_ADDR and PT_TEXT_END_ADDR.
>
> This is OK.
>
>> +#if defined(__mcoldfire__)
>> +/* These are still undefined in recent (3.10) kernels.  */
>> +#define PT_TEXT_ADDR 49*4
>> +#define PT_DATA_ADDR 50*4
>> +#define PT_TEXT_END_ADDR  51*4
>> +/* BFIN already defines these constants in recent (3.10) kernels.  */
>> +#elif defined(BFIN)
>> +#define PT_TEXT_ADDR 220
>> +#define PT_TEXT_END_ADDR 224
>> +#define PT_DATA_ADDR 228
>> +/* These are still undefined in recent (3.10) kernels.  */
>
> But please avoid "recent" "new", etc. in code.  Those get old fast.
>
> Thanks,
>

Thanks. Here is what i checked in.

Luis



[-- Attachment #2: 0001-qoffsets_uclinux.diff --]
[-- Type: text/x-patch, Size: 3651 bytes --]

2013-05-16  Luis Machado  <lgustavo@codesourcery.com>

	* linux-low.c: Move definition checks upwards for PT_TEXT_ADDR,
	PT_DATA_ADDR and PT_TEXT_END_ADDR.  Update comments.
	(linux_read_offsets): Remove PT_TEXT_ADDR, PT_DATA_ADDR and
	PT_TEXT_END_ADDR guards.  Update comments.
	(linux_target_op) <read_offsets>: Conditionally define to
	linux_read_offsets if the target is UCLIBC and if it defines
	PT_TEXT_ADDR, PT_DATA_ADDR and PT_TEXT_END_ADDR.

Index: gdb-head/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-head.orig/gdb/gdbserver/linux-low.c	2013-04-17 10:24:52.859499119 +0200
+++ gdb-head/gdb/gdbserver/linux-low.c	2013-05-16 12:29:32.822525610 +0200
@@ -84,6 +84,30 @@
 #endif
 #endif
 
+/* Some targets did not define these ptrace constants from the start,
+   so gdbserver defines them locally here.  In the future, these may
+   be removed after they are added to asm/ptrace.h.  */
+#if !(defined(PT_TEXT_ADDR) \
+      || defined(PT_DATA_ADDR) \
+      || defined(PT_TEXT_END_ADDR))
+#if defined(__mcoldfire__)
+/* These are still undefined in 3.10 kernels.  */
+#define PT_TEXT_ADDR 49*4
+#define PT_DATA_ADDR 50*4
+#define PT_TEXT_END_ADDR  51*4
+/* BFIN already defines these since at least 2.6.32 kernels.  */
+#elif defined(BFIN)
+#define PT_TEXT_ADDR 220
+#define PT_TEXT_END_ADDR 224
+#define PT_DATA_ADDR 228
+/* These are still undefined in 3.10 kernels.  */
+#elif defined(__TMS320C6X__)
+#define PT_TEXT_ADDR     (0x10000*4)
+#define PT_DATA_ADDR     (0x10004*4)
+#define PT_TEXT_END_ADDR (0x10008*4)
+#endif
+#endif
+
 #ifdef HAVE_LINUX_BTRACE
 # include "linux-btrace.h"
 #endif
@@ -4833,25 +4857,14 @@ linux_stopped_data_address (void)
   return lwp->stopped_data_address;
 }
 
-#if defined(__UCLIBC__) && defined(HAS_NOMMU)
-#if ! (defined(PT_TEXT_ADDR) \
-       || defined(PT_DATA_ADDR) \
-       || defined(PT_TEXT_END_ADDR))
-#if defined(__mcoldfire__)
-/* These should really be defined in the kernel's ptrace.h header.  */
-#define PT_TEXT_ADDR 49*4
-#define PT_DATA_ADDR 50*4
-#define PT_TEXT_END_ADDR  51*4
-#elif defined(BFIN)
-#define PT_TEXT_ADDR 220
-#define PT_TEXT_END_ADDR 224
-#define PT_DATA_ADDR 228
-#elif defined(__TMS320C6X__)
-#define PT_TEXT_ADDR     (0x10000*4)
-#define PT_DATA_ADDR     (0x10004*4)
-#define PT_TEXT_END_ADDR (0x10008*4)
-#endif
-#endif
+#if defined(__UCLIBC__) && defined(HAS_NOMMU)	      \
+    && defined(PT_TEXT_ADDR) && defined(PT_DATA_ADDR) \
+    && defined(PT_TEXT_END_ADDR)
+
+/* This is only used for targets that define PT_TEXT_ADDR,
+   PT_DATA_ADDR and PT_TEXT_END_ADDR.  If those are not defined, supposedly
+   the target has different ways of acquiring this information, like
+   loadmaps.  */
 
 /* Under uClinux, programs are loaded at non-zero offsets, which we need
    to tell gdb about.  */
@@ -4859,7 +4872,6 @@ linux_stopped_data_address (void)
 static int
 linux_read_offsets (CORE_ADDR *text_p, CORE_ADDR *data_p)
 {
-#if defined(PT_TEXT_ADDR) && defined(PT_DATA_ADDR) && defined(PT_TEXT_END_ADDR)
   unsigned long text, text_end, data;
   int pid = lwpid_of (get_thread_lwp (current_inferior));
 
@@ -4888,7 +4900,6 @@ linux_read_offsets (CORE_ADDR *text_p, C
 
       return 1;
     }
-#endif
  return 0;
 }
 #endif
@@ -5887,7 +5898,9 @@ static struct target_ops linux_target_op
   linux_remove_point,
   linux_stopped_by_watchpoint,
   linux_stopped_data_address,
-#if defined(__UCLIBC__) && defined(HAS_NOMMU)
+#if defined(__UCLIBC__) && defined(HAS_NOMMU)	      \
+    && defined(PT_TEXT_ADDR) && defined(PT_DATA_ADDR) \
+    && defined(PT_TEXT_END_ADDR)
   linux_read_offsets,
 #else
   NULL,