[PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Luis Machado]

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

Hi,

At some point, c6x used different data structures for its DSBT-based 
loadmap.

DSBT-based

struct target_loadmap
{
   /* Protocol version number, must be zero.  */
   Elf32_Word version;
   /* Pointer to the DSBT table, its size, and the DSBT index.  */
   unsigned *dsbt_table;
   unsigned dsbt_size, dsbt_index;
   /* Number of segments in this map.  */
   Elf32_Word nsegs;
   /* The actual memory map.  */
   struct target_loadseg segs[/*nsegs*/];
};

FDPIC-based

struct target_loadmap
{
   /* Protocol version number, must be zero.  */
   Elf32_Half version;
   /* Number of segments in this map.  */
   Elf32_Half nsegs;
   /* The actual memory map.  */
   struct target_loadseg segs[/*nsegs*/];
};

We shared a little bit of code with FDPIC-based targets though...

struct target_loadseg
{
   /* Core address to which the segment is mapped.  */
   Elf32_Addr addr;
   /* VMA recorded in the program header.  */
   Elf32_Addr p_vaddr;
   /* Size of this segment in memory.  */
   Elf32_Word p_memsz;
};

Things have changed, and c6x is now using the exact same data structures 
as FDPIC-based targets in uClibc. Please refer to 
http://lists.uclibc.org/pipermail/uclibc/2013-May/047789.html for the 
uClibc changes that led to this.

Mark Salter, the author of the uClibc change, has agreed with the 
solution i proposed: 
http://lists.uclibc.org/pipermail/uclibc/2013-May/047790.html.

It is all good, but we've been conditionalizing the c6x-specific 
target_loadmap data structure based on the presence of PT_GETDSBT. This 
has always been defined in uClibc and, since Mark's change, it doesn't 
work as a hint of whether to use the new or the old target_loadmap data 
structure anymore. Therefore we will/already have a potential problem 
with backwards compatibility.

Bernhard has stated that backwards compatibility on uClibc's side is not 
a problem: http://lists.uclibc.org/pipermail/uclibc/2013-June/047801.html.

With all that exposed, my proposed change to gdbserver is to drop all 
the DSBT-specific bits, remove their definitions and explicitly use 
FDPIC definitions instead, making things a little bit cleaner.

In the following patch i also changed the code slightly to stop defining 
linux_read_loadmap to NULL and i switched to explicitly setting the 
target hook to NULL in the absence of the required definition.

What do you think? Yao? Mike?

Luis

[-- Attachment #2: dsbt_fdpic.diff --]
[-- Type: text/x-patch, Size: 3290 bytes --]

2013-06-20  Luis Machado  <lgustavo@codesourcery.com>

	* linux-low.c: Remove check for PT_GETDSBT.
	(target_loadmap): Remove data structure conditionalized by
	the presence of PT_GETDSBT.
	(LINUX_LOADMAP, LINUX_LOADMAP_EXEC,
	LINUX_LOADMAP_INTERP): Remove definitions.
	(linux_read_loadmap): Replace LINUX_LOADMAP_EXEC with
	PTRACE_GETFDPIC_EXEC, LINUX_LOADMAP_INTERP with
	PTRACE_GETFDPIC_INTERP and LINUX_LOADMAP with PTRACE_GETFDPIC.
	Do not set linux_read_loadmap to NULL in the absence of
	PTRACE_GETFDPIC.
	(linux_target_ops) <read_loadmap>: Only set to linux_read_loadmap
	in the presence of PTRACE_GETFDPIC.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index bb7298a..df53775 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5263,7 +5263,7 @@ linux_qxfer_spu (const char *annex, unsigned char *readbuf,
   return ret;
 }
 
-#if defined PT_GETDSBT || defined PTRACE_GETFDPIC
+#if defined PTRACE_GETFDPIC
 struct target_loadseg
 {
   /* Core address to which the segment is mapped.  */
@@ -5274,23 +5274,6 @@ struct target_loadseg
   Elf32_Word p_memsz;
 };
 
-# if defined PT_GETDSBT
-struct target_loadmap
-{
-  /* Protocol version number, must be zero.  */
-  Elf32_Word version;
-  /* Pointer to the DSBT table, its size, and the DSBT index.  */
-  unsigned *dsbt_table;
-  unsigned dsbt_size, dsbt_index;
-  /* Number of segments in this map.  */
-  Elf32_Word nsegs;
-  /* The actual memory map.  */
-  struct target_loadseg segs[/*nsegs*/];
-};
-#  define LINUX_LOADMAP		PT_GETDSBT
-#  define LINUX_LOADMAP_EXEC	PTRACE_GETDSBT_EXEC
-#  define LINUX_LOADMAP_INTERP	PTRACE_GETDSBT_INTERP
-# else
 struct target_loadmap
 {
   /* Protocol version number, must be zero.  */
@@ -5300,10 +5283,6 @@ struct target_loadmap
   /* The actual memory map.  */
   struct target_loadseg segs[/*nsegs*/];
 };
-#  define LINUX_LOADMAP		PTRACE_GETFDPIC
-#  define LINUX_LOADMAP_EXEC	PTRACE_GETFDPIC_EXEC
-#  define LINUX_LOADMAP_INTERP	PTRACE_GETFDPIC_INTERP
-# endif
 
 static int
 linux_read_loadmap (const char *annex, CORE_ADDR offset,
@@ -5315,13 +5294,13 @@ linux_read_loadmap (const char *annex, CORE_ADDR offset,
   unsigned int actual_length, copy_length;
 
   if (strcmp (annex, "exec") == 0)
-    addr = (int) LINUX_LOADMAP_EXEC;
+    addr = (int) PTRACE_GETFDPIC_EXEC;
   else if (strcmp (annex, "interp") == 0)
-    addr = (int) LINUX_LOADMAP_INTERP;
+    addr = (int) PTRACE_GETFDPIC_INTERP;
   else
     return -1;
 
-  if (ptrace (LINUX_LOADMAP, pid, addr, &data) != 0)
+  if (ptrace (PTRACE_GETFDPIC, pid, addr, &data) != 0)
     return -1;
 
   if (data == NULL)
@@ -5337,9 +5316,7 @@ linux_read_loadmap (const char *annex, CORE_ADDR offset,
   memcpy (myaddr, (char *) data + offset, copy_length);
   return copy_length;
 }
-#else
-# define linux_read_loadmap NULL
-#endif /* defined PT_GETDSBT || defined PTRACE_GETFDPIC */
+#endif /* defined PTRACE_GETFDPIC */
 
 static void
 linux_process_qsupported (const char *query)
@@ -6037,7 +6014,11 @@ static struct target_ops linux_target_ops = {
   NULL,
 #endif
   linux_common_core_of_thread,
+#if defined PTRACE_GETFDPIC
   linux_read_loadmap,
+#else
+  NULL,
+#endif /* defined PTRACE_GETFDPIC */
   linux_process_qsupported,
   linux_supports_tracepoints,
   linux_read_pc,

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Yao Qi]

On 06/21/2013 02:51 AM, Luis Machado wrote:
> Things have changed, and c6x is now using the exact same data structures
> as FDPIC-based targets in uClibc. Please refer to
> http://lists.uclibc.org/pipermail/uclibc/2013-May/047789.html  for the
> uClibc changes that led to this.
>
> Mark Salter, the author of the uClibc change, has agreed with the
> solution i proposed:
> http://lists.uclibc.org/pipermail/uclibc/2013-May/047790.html.
>
> It is all good, but we've been conditionalizing the c6x-specific
> target_loadmap data structure based on the presence of PT_GETDSBT. This
> has always been defined in uClibc and, since Mark's change, it doesn't
> work as a hint of whether to use the new or the old target_loadmap data
> structure anymore. Therefore we will/already have a potential problem
> with backwards compatibility.
>
> Bernhard has stated that backwards compatibility on uClibc's side is not
> a problem:http://lists.uclibc.org/pipermail/uclibc/2013-June/047801.html.
>
> With all that exposed, my proposed change to gdbserver is to drop all
> the DSBT-specific bits, remove their definitions and explicitly use
> FDPIC definitions instead, making things a little bit cleaner.
>
> In the following patch i also changed the code slightly to stop defining
> linux_read_loadmap to NULL and i switched to explicitly setting the
> target hook to NULL in the absence of the required definition.
>
> What do you think? Yao? Mike?

Luis,
Looks Mark S. proposed using FDPIC in tic6x port in kernel, instead of 
DSBT which was used when we did the tic6x port in GDB.  I checked the 
kernel log, and found that DSBT constants are never used in the official 
kernel.  They only appeared in the linux-c6x.org git tree temporarily. 
Since kernel and uclibc has migrated to the new scheme, I don't worry 
about the compatibility issue here.

>
> 2013-06-20  Luis Machado<lgustavo@codesourcery.com>
>
> 	* linux-low.c: Remove check for PT_GETDSBT.
> 	(target_loadmap): Remove data structure conditionalized by
> 	the presence of PT_GETDSBT.
> 	(LINUX_LOADMAP, LINUX_LOADMAP_EXEC,
> 	LINUX_LOADMAP_INTERP): Remove definitions.

Not sure we can break words in parentheses into multiple lines.  I suggest:

	(LINUX_LOADMAP, LINUX_LOADMAP_EXEC): Remove definitions.
	(LINUX_LOADMAP_INTERP): Likewise.

the patch is OK to me, but we also need adjustments in solib-dsbt.c. 
This patch should go in together with the changes in solib-dsbt.c.

-- 
Yao (齐尧)

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Luis Machado]

On 06/24/2013 10:42 AM, Yao Qi wrote:
> On 06/21/2013 02:51 AM, Luis Machado wrote:
>> Things have changed, and c6x is now using the exact same data structures
>> as FDPIC-based targets in uClibc. Please refer to
>> http://lists.uclibc.org/pipermail/uclibc/2013-May/047789.html  for the
>> uClibc changes that led to this.
>>
>> Mark Salter, the author of the uClibc change, has agreed with the
>> solution i proposed:
>> http://lists.uclibc.org/pipermail/uclibc/2013-May/047790.html.
>>
>> It is all good, but we've been conditionalizing the c6x-specific
>> target_loadmap data structure based on the presence of PT_GETDSBT. This
>> has always been defined in uClibc and, since Mark's change, it doesn't
>> work as a hint of whether to use the new or the old target_loadmap data
>> structure anymore. Therefore we will/already have a potential problem
>> with backwards compatibility.
>>
>> Bernhard has stated that backwards compatibility on uClibc's side is not
>> a problem:http://lists.uclibc.org/pipermail/uclibc/2013-June/047801.html.
>>
>> With all that exposed, my proposed change to gdbserver is to drop all
>> the DSBT-specific bits, remove their definitions and explicitly use
>> FDPIC definitions instead, making things a little bit cleaner.
>>
>> In the following patch i also changed the code slightly to stop defining
>> linux_read_loadmap to NULL and i switched to explicitly setting the
>> target hook to NULL in the absence of the required definition.
>>
>> What do you think? Yao? Mike?
>
> Luis,
> Looks Mark S. proposed using FDPIC in tic6x port in kernel, instead of
> DSBT which was used when we did the tic6x port in GDB.  I checked the
> kernel log, and found that DSBT constants are never used in the official
> kernel.  They only appeared in the linux-c6x.org git tree temporarily.
> Since kernel and uclibc has migrated to the new scheme, I don't worry
> about the compatibility issue here.
>
>>
>> 2013-06-20  Luis Machado<lgustavo@codesourcery.com>
>>
>>     * linux-low.c: Remove check for PT_GETDSBT.
>>     (target_loadmap): Remove data structure conditionalized by
>>     the presence of PT_GETDSBT.
>>     (LINUX_LOADMAP, LINUX_LOADMAP_EXEC,
>>     LINUX_LOADMAP_INTERP): Remove definitions.
>
> Not sure we can break words in parentheses into multiple lines.  I suggest:
>
>      (LINUX_LOADMAP, LINUX_LOADMAP_EXEC): Remove definitions.
>      (LINUX_LOADMAP_INTERP): Likewise.
>
> the patch is OK to me, but we also need adjustments in solib-dsbt.c.
> This patch should go in together with the changes in solib-dsbt.c.
>

Thanks Yao. You are correct. Let me address the solib-dsbt.c bits as well.

Luis

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Mike Frysinger]

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

On Thursday 20 June 2013 14:51:00 Luis Machado wrote:
> What do you think? Yao? Mike?

bloop, accidentally deleted this before i got a chance to respond :)

this looks really good.  i wondered why DSBT was written the way it was 
(slightly different from FDPIC).  the fact that it can work with the FDPIC 
structure is awesome.  i have no opinion on the backwards compat issue, but it 
doesn't seem like it should be a problem as the official kernel hasn't 
supported.
-mike

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

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Pedro Alves]

On 06/20/2013 07:51 PM, Luis Machado wrote:
> Hi,
> 
> At some point, c6x used different data structures for its DSBT-based 
> loadmap.
> 
> DSBT-based
> 
> struct target_loadmap
> {
>    /* Protocol version number, must be zero.  */
>    Elf32_Word version;
>    /* Pointer to the DSBT table, its size, and the DSBT index.  */
>    unsigned *dsbt_table;
>    unsigned dsbt_size, dsbt_index;
>    /* Number of segments in this map.  */
>    Elf32_Word nsegs;
>    /* The actual memory map.  */
>    struct target_loadseg segs[/*nsegs*/];
> };
> 
> FDPIC-based
> 
> struct target_loadmap
> {
>    /* Protocol version number, must be zero.  */
>    Elf32_Half version;
>    /* Number of segments in this map.  */
>    Elf32_Half nsegs;
>    /* The actual memory map.  */
>    struct target_loadseg segs[/*nsegs*/];
> };
> 
> We shared a little bit of code with FDPIC-based targets though...
> 
> struct target_loadseg
> {
>    /* Core address to which the segment is mapped.  */
>    Elf32_Addr addr;
>    /* VMA recorded in the program header.  */
>    Elf32_Addr p_vaddr;
>    /* Size of this segment in memory.  */
>    Elf32_Word p_memsz;
> };
> 
> Things have changed, and c6x is now using the exact same data structures 
> as FDPIC-based targets in uClibc. Please refer to 
> http://lists.uclibc.org/pipermail/uclibc/2013-May/047789.html for the 
> uClibc changes that led to this.
> 
> Mark Salter, the author of the uClibc change, has agreed with the 
> solution i proposed: 
> http://lists.uclibc.org/pipermail/uclibc/2013-May/047790.html.
> 
> It is all good, but we've been conditionalizing the c6x-specific 
> target_loadmap data structure based on the presence of PT_GETDSBT. This 
> has always been defined in uClibc and, since Mark's change, it doesn't 
> work as a hint of whether to use the new or the old target_loadmap data 
> structure anymore. Therefore we will/already have a potential problem 
> with backwards compatibility.
> 
> Bernhard has stated that backwards compatibility on uClibc's side is not 
> a problem: http://lists.uclibc.org/pipermail/uclibc/2013-June/047801.html.
> 
> With all that exposed, my proposed change to gdbserver is to drop all 
> the DSBT-specific bits, remove their definitions and explicitly use 
> FDPIC definitions instead, making things a little bit cleaner.
> 
> In the following patch i also changed the code slightly to stop defining 
> linux_read_loadmap to NULL and i switched to explicitly setting the 
> target hook to NULL in the absence of the required definition.

This is fine with me.

Does this mean that solib-frv.c / solib-dsbt.c and the
solib-fdpic.c from
http://www.sourceware.org/ml/gdb-patches/2010-12/msg00291.html

?

can all be combined?  I'm confused on the current state
of bfin solib debugging -- it is still depending on out of
tree patches?

-- 
Pedro Alves

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Luis Machado]

On 06/25/2013 12:01 PM, Pedro Alves wrote:
> On 06/20/2013 07:51 PM, Luis Machado wrote:
>> Hi,
>>
>> At some point, c6x used different data structures for its DSBT-based
>> loadmap.
>>
>> DSBT-based
>>
>> struct target_loadmap
>> {
>>     /* Protocol version number, must be zero.  */
>>     Elf32_Word version;
>>     /* Pointer to the DSBT table, its size, and the DSBT index.  */
>>     unsigned *dsbt_table;
>>     unsigned dsbt_size, dsbt_index;
>>     /* Number of segments in this map.  */
>>     Elf32_Word nsegs;
>>     /* The actual memory map.  */
>>     struct target_loadseg segs[/*nsegs*/];
>> };
>>
>> FDPIC-based
>>
>> struct target_loadmap
>> {
>>     /* Protocol version number, must be zero.  */
>>     Elf32_Half version;
>>     /* Number of segments in this map.  */
>>     Elf32_Half nsegs;
>>     /* The actual memory map.  */
>>     struct target_loadseg segs[/*nsegs*/];
>> };
>>
>> We shared a little bit of code with FDPIC-based targets though...
>>
>> struct target_loadseg
>> {
>>     /* Core address to which the segment is mapped.  */
>>     Elf32_Addr addr;
>>     /* VMA recorded in the program header.  */
>>     Elf32_Addr p_vaddr;
>>     /* Size of this segment in memory.  */
>>     Elf32_Word p_memsz;
>> };
>>
>> Things have changed, and c6x is now using the exact same data structures
>> as FDPIC-based targets in uClibc. Please refer to
>> http://lists.uclibc.org/pipermail/uclibc/2013-May/047789.html for the
>> uClibc changes that led to this.
>>
>> Mark Salter, the author of the uClibc change, has agreed with the
>> solution i proposed:
>> http://lists.uclibc.org/pipermail/uclibc/2013-May/047790.html.
>>
>> It is all good, but we've been conditionalizing the c6x-specific
>> target_loadmap data structure based on the presence of PT_GETDSBT. This
>> has always been defined in uClibc and, since Mark's change, it doesn't
>> work as a hint of whether to use the new or the old target_loadmap data
>> structure anymore. Therefore we will/already have a potential problem
>> with backwards compatibility.
>>
>> Bernhard has stated that backwards compatibility on uClibc's side is not
>> a problem: http://lists.uclibc.org/pipermail/uclibc/2013-June/047801.html.
>>
>> With all that exposed, my proposed change to gdbserver is to drop all
>> the DSBT-specific bits, remove their definitions and explicitly use
>> FDPIC definitions instead, making things a little bit cleaner.
>>
>> In the following patch i also changed the code slightly to stop defining
>> linux_read_loadmap to NULL and i switched to explicitly setting the
>> target hook to NULL in the absence of the required definition.
>
> This is fine with me.
>
> Does this mean that solib-frv.c / solib-dsbt.c and the
> solib-fdpic.c from
> http://www.sourceware.org/ml/gdb-patches/2010-12/msg00291.html
>
> ?
>
> can all be combined?  I'm confused on the current state
> of bfin solib debugging -- it is still depending on out of
> tree patches?
>

I wouldn't discard the possibility, but i still need to check if those 
are fully compatible with each other. Would be great to merge all those 
into a single file.

Luis

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Mike Frysinger]

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

On Tuesday 25 June 2013 11:01:14 Pedro Alves wrote:
> can all be combined?  I'm confused on the current state
> of bfin solib debugging -- it is still depending on out of
> tree patches?

yes :/.  i tried to rewrite solib-frv.c to a solib-fdpic.c which frv & bfin 
then used, but haven't had a chance to ever really get back to finishing it.  i 
don't really want to merge the change the Blackfin port uses since it just 
copies solib-frv.c to solib-bfin.c then runs like s/FRV/BFIN/g on it.

since i no longer work at ADI, it's Blackfin stuff is a hobby now, so "boring" 
mechanical work like this isn't high on my list :x.
-mike

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

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Luis Machado]

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

Hi,

On 06/24/2013 10:47 AM, Luis Machado wrote:
> On 06/24/2013 10:42 AM, Yao Qi wrote:
>> On 06/21/2013 02:51 AM, Luis Machado wrote:
>>> Things have changed, and c6x is now using the exact same data structures
>>> as FDPIC-based targets in uClibc. Please refer to
>>> http://lists.uclibc.org/pipermail/uclibc/2013-May/047789.html  for the
>>> uClibc changes that led to this.
>>>
>>> Mark Salter, the author of the uClibc change, has agreed with the
>>> solution i proposed:
>>> http://lists.uclibc.org/pipermail/uclibc/2013-May/047790.html.
>>>
>>> It is all good, but we've been conditionalizing the c6x-specific
>>> target_loadmap data structure based on the presence of PT_GETDSBT. This
>>> has always been defined in uClibc and, since Mark's change, it doesn't
>>> work as a hint of whether to use the new or the old target_loadmap data
>>> structure anymore. Therefore we will/already have a potential problem
>>> with backwards compatibility.
>>>
>>> Bernhard has stated that backwards compatibility on uClibc's side is not
>>> a
>>> problem:http://lists.uclibc.org/pipermail/uclibc/2013-June/047801.html.
>>>
>>> With all that exposed, my proposed change to gdbserver is to drop all
>>> the DSBT-specific bits, remove their definitions and explicitly use
>>> FDPIC definitions instead, making things a little bit cleaner.
>>>
>>> In the following patch i also changed the code slightly to stop defining
>>> linux_read_loadmap to NULL and i switched to explicitly setting the
>>> target hook to NULL in the absence of the required definition.
>>>
>>> What do you think? Yao? Mike?
>>
>> Luis,
>> Looks Mark S. proposed using FDPIC in tic6x port in kernel, instead of
>> DSBT which was used when we did the tic6x port in GDB.  I checked the
>> kernel log, and found that DSBT constants are never used in the official
>> kernel.  They only appeared in the linux-c6x.org git tree temporarily.
>> Since kernel and uclibc has migrated to the new scheme, I don't worry
>> about the compatibility issue here.
>>
>>>
>>> 2013-06-20  Luis Machado<lgustavo@codesourcery.com>
>>>
>>>     * linux-low.c: Remove check for PT_GETDSBT.
>>>     (target_loadmap): Remove data structure conditionalized by
>>>     the presence of PT_GETDSBT.
>>>     (LINUX_LOADMAP, LINUX_LOADMAP_EXEC,
>>>     LINUX_LOADMAP_INTERP): Remove definitions.
>>
>> Not sure we can break words in parentheses into multiple lines.  I
>> suggest:
>>
>>      (LINUX_LOADMAP, LINUX_LOADMAP_EXEC): Remove definitions.
>>      (LINUX_LOADMAP_INTERP): Likewise.
>>
>> the patch is OK to me, but we also need adjustments in solib-dsbt.c.
>> This patch should go in together with the changes in solib-dsbt.c.
>>
>
> Thanks Yao. You are correct. Let me address the solib-dsbt.c bits as well.
>
> Luis

Here is an updated patch that deals with solib-dsbt.c as well. The 
additional tic6x-specific fields from the loadmap were removed and 
dsbt_index is now fetched from the shared library file on disk instead 
of grabbing such information from the runtime loadmap. That information 
does not change anyway.

Since hardware for this platform is scarce, i could not test this change 
for real, only build it and certify it looks sane.

How does it look?

Luis


[-- Attachment #2: dsbt.diff --]
[-- Type: text/x-patch, Size: 10574 bytes --]

2013-08-08  Luis Machado  <lgustavo@codesourcery.com>

	gdb/gdbserver
	* linux-low.c: Remove check for PT_GETDSBT.
	(target_loadmap): Remove data structure conditionalized by
	the presence of PT_GETDSBT.
	(LINUX_LOADMAP, LINUX_LOADMAP_EXEC,
	LINUX_LOADMAP_INTERP): Remove definitions.
	(linux_read_loadmap): Replace LINUX_LOADMAP_EXEC with
	PTRACE_GETFDPIC_EXEC, LINUX_LOADMAP_INTERP with
	PTRACE_GETFDPIC_INTERP and LINUX_LOADMAP with PTRACE_GETFDPIC.
	Do not set linux_read_loadmap to NULL in the absence of
	PTRACE_GETFDPIC.
	(linux_target_ops) <read_loadmap>: Only set to linux_read_loadmap
	in the presence of PTRACE_GETFDPIC.

	gdb/
	* solib-dsbt.c (DT_TIC6X_DSBT_INDEX): New definition.
	(ext_elf32_dsbt_loadseg) <dsbt_table_ptr>: Remove.
	(ext_elf32_dsbt_loadseg) <dsbt_size, dsbt_index>: Remove.
	(int_elf32_dsbt_loadmap) <dsbt_table_ptr>: Remove.
	(int_elf32_dsbt_loadmap) <dsbt_size, dsbt_index>: Remove.
	(scan_dyntag_in_bfd): New function.
	(fetch_solib_dsbt_index): New function.
	(dsbt_current_sos): Extract dsbt_index based on the
	shared library filename.

 gdb/gdbserver/linux-low.c |   37 +++--------
 gdb/solib-dsbt.c          |  160 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 143 insertions(+), 54 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 47ea76d..4013829 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5262,7 +5262,7 @@ linux_qxfer_spu (const char *annex, unsigned char *readbuf,
   return ret;
 }
 
-#if defined PT_GETDSBT || defined PTRACE_GETFDPIC
+#if defined PTRACE_GETFDPIC
 struct target_loadseg
 {
   /* Core address to which the segment is mapped.  */
@@ -5273,23 +5273,6 @@ struct target_loadseg
   Elf32_Word p_memsz;
 };
 
-# if defined PT_GETDSBT
-struct target_loadmap
-{
-  /* Protocol version number, must be zero.  */
-  Elf32_Word version;
-  /* Pointer to the DSBT table, its size, and the DSBT index.  */
-  unsigned *dsbt_table;
-  unsigned dsbt_size, dsbt_index;
-  /* Number of segments in this map.  */
-  Elf32_Word nsegs;
-  /* The actual memory map.  */
-  struct target_loadseg segs[/*nsegs*/];
-};
-#  define LINUX_LOADMAP		PT_GETDSBT
-#  define LINUX_LOADMAP_EXEC	PTRACE_GETDSBT_EXEC
-#  define LINUX_LOADMAP_INTERP	PTRACE_GETDSBT_INTERP
-# else
 struct target_loadmap
 {
   /* Protocol version number, must be zero.  */
@@ -5299,10 +5282,6 @@ struct target_loadmap
   /* The actual memory map.  */
   struct target_loadseg segs[/*nsegs*/];
 };
-#  define LINUX_LOADMAP		PTRACE_GETFDPIC
-#  define LINUX_LOADMAP_EXEC	PTRACE_GETFDPIC_EXEC
-#  define LINUX_LOADMAP_INTERP	PTRACE_GETFDPIC_INTERP
-# endif
 
 static int
 linux_read_loadmap (const char *annex, CORE_ADDR offset,
@@ -5314,13 +5293,13 @@ linux_read_loadmap (const char *annex, CORE_ADDR offset,
   unsigned int actual_length, copy_length;
 
   if (strcmp (annex, "exec") == 0)
-    addr = (int) LINUX_LOADMAP_EXEC;
+    addr = (int) PTRACE_GETFDPIC_EXEC;
   else if (strcmp (annex, "interp") == 0)
-    addr = (int) LINUX_LOADMAP_INTERP;
+    addr = (int) PTRACE_GETFDPIC_INTERP;
   else
     return -1;
 
-  if (ptrace (LINUX_LOADMAP, pid, addr, &data) != 0)
+  if (ptrace (PTRACE_GETFDPIC, pid, addr, &data) != 0)
     return -1;
 
   if (data == NULL)
@@ -5336,9 +5315,7 @@ linux_read_loadmap (const char *annex, CORE_ADDR offset,
   memcpy (myaddr, (char *) data + offset, copy_length);
   return copy_length;
 }
-#else
-# define linux_read_loadmap NULL
-#endif /* defined PT_GETDSBT || defined PTRACE_GETFDPIC */
+#endif /* defined PTRACE_GETFDPIC */
 
 static void
 linux_process_qsupported (const char *query)
@@ -6036,7 +6013,11 @@ static struct target_ops linux_target_ops = {
   NULL,
 #endif
   linux_common_core_of_thread,
+#if defined PTRACE_GETFDPIC
   linux_read_loadmap,
+#else
+  NULL,
+#endif /* defined PTRACE_GETFDPIC */
   linux_process_qsupported,
   linux_supports_tracepoints,
   linux_read_pc,
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 4fe24f8..4fd3eca 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -33,6 +33,7 @@
 #include "gdb_bfd.h"
 
 #define GOT_MODULE_OFFSET 4
+#define DT_TIC6X_DSBT_INDEX 0x70000003
 
 /* Flag which indicates whether internal debug messages should be printed.  */
 static unsigned int solib_dsbt_debug = 0;
@@ -62,11 +63,6 @@ struct ext_elf32_dsbt_loadseg
 struct ext_elf32_dsbt_loadmap {
   /* Protocol version number, must be zero.  */
   ext_Elf32_Word version;
-  /* A pointer to the DSBT table; the DSBT size and the index of this
-     module.  */
-  ext_Elf32_Word dsbt_table_ptr;
-  ext_Elf32_Word dsbt_size;
-  ext_Elf32_Word dsbt_index;
   /* Number of segments in this map.  */
   ext_Elf32_Word nsegs;
   /* The actual memory map.  */
@@ -90,10 +86,6 @@ struct int_elf32_dsbt_loadmap
 {
   /* Protocol version number, must be zero.  */
   int version;
-  CORE_ADDR dsbt_table_ptr;
-  /* A pointer to the DSBT table; the DSBT size and the index of this
-     module.  */
-  int dsbt_size, dsbt_index;
   /* Number of segments in this map.  */
   int nsegs;
   /* The actual memory map.  */
@@ -619,6 +611,123 @@ lm_base (void)
   return info->lm_base_cache;
 }
 
+/* Scan for DYNTAG in .dynamic section of ABFD. If DYNTAG is found 1
+   is returned and the corresponding PTR is set.  We only search in
+   the BFD, not in the target's memory.  */
+
+static int
+scan_dyntag_in_bfd (int dyntag, bfd *abfd, CORE_ADDR *ptr)
+{
+  int step, sect_size;
+  long dyn_tag;
+  CORE_ADDR dyn_ptr, dyn_addr;
+  gdb_byte *bufend, *bufstart, *buf;
+  Elf64_External_Dyn *x_dynp_64;
+  struct bfd_section *sect;
+  struct target_section *target_section;
+
+  if (abfd == NULL)
+    return 0;
+
+  if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
+    return 0;
+
+  /* Make sure the size is sane.  */
+  if (bfd_get_arch_size (abfd) == -1)
+    return 0;
+
+  /* Find the start address of the .dynamic section.  */
+  sect = bfd_get_section_by_name (abfd, ".dynamic");
+  if (sect == NULL)
+    return 0;
+
+  for (target_section = current_target_sections->sections;
+       target_section < current_target_sections->sections_end;
+       target_section++)
+    if (sect == target_section->the_bfd_section)
+      break;
+  if (target_section < current_target_sections->sections_end)
+    dyn_addr = target_section->addr;
+  else
+    {
+      /* ABFD may come from OBJFILE acting only as a symbol file
+	 without being loaded into the target
+	 (see add_symbol_file_command).  This case is such fallback
+	 to the file VMA address without the possibility of having
+	 the section relocated to its actual in-memory address.  */
+
+      dyn_addr = bfd_section_vma (abfd, sect);
+    }
+
+  /* Read in .dynamic from the BFD.  */
+  sect_size = bfd_section_size (abfd, sect);
+  buf = bufstart = alloca (sect_size);
+  if (!bfd_get_section_contents (abfd, sect,
+				 buf, 0, sect_size))
+    return 0;
+
+  /* Iterate over BUF and scan for DYNTAG.  If found, set PTR and
+     return.  */
+  step = sizeof (Elf64_External_Dyn);
+
+  for (bufend = buf + sect_size;
+       buf < bufend;
+       buf += step)
+  {
+    x_dynp_64 = (Elf64_External_Dyn *) buf;
+    dyn_tag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
+    dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
+
+    if (dyn_tag == DT_NULL)
+      return 0;
+    if (dyn_tag == dyntag)
+      {
+	if (ptr)
+	  {
+	    /* We don't want to read this information from memory.
+	       If a relocation is needed, it should be done
+	       elsewhere.  */
+	    *ptr = dyn_ptr;
+	  }
+	return 1;
+      }
+  }
+
+  return 0;
+}
+
+/* Given a shared library filename, load it up and find
+   out what is its dsbt index.  */
+
+static int
+fetch_solib_dsbt_index (char *filename)
+{
+  unsigned long dsbt_index;
+  CORE_ADDR addr;
+  bfd *solib_bfd = NULL;
+  volatile struct gdb_exception ex;
+
+  if (filename == NULL)
+    return 0;
+
+  /* Open the shared library.  */
+  TRY_CATCH (ex, RETURN_MASK_ALL)
+    {
+      solib_bfd = solib_bfd_open (filename);
+    }
+  if (solib_bfd == NULL)
+    {
+      return 0;
+    }
+
+  /* Fetch the DSBT_INDEX from the shared library file on disk.  */
+  if (scan_dyntag_in_bfd (DT_TIC6X_DSBT_INDEX, solib_bfd, &addr) == 0)
+    return -1;
+
+  bfd_close (solib_bfd);
+
+  return addr;
+}
 
 /* Build a list of `struct so_list' objects describing the shared
    objects currently loaded in the inferior.  This list does not
@@ -661,10 +770,12 @@ dsbt_current_sos (void)
   while (lm_addr)
     {
       struct ext_link_map lm_buf;
-      ext_Elf32_Word indexword;
       CORE_ADDR map_addr;
       int dsbt_index;
       int ret;
+      CORE_ADDR addr;
+      int errcode;
+      char *name_buf = NULL;
 
       if (solib_dsbt_debug)
 	fprintf_unfiltered (gdb_stdlog,
@@ -684,24 +795,26 @@ dsbt_current_sos (void)
 					   sizeof lm_buf.l_addr.map,
 					   byte_order);
 
-      ret = target_read_memory (map_addr + 12, (gdb_byte *) &indexword,
-				sizeof indexword);
-      if (ret)
+      /* Fetch the name.  */
+      addr = extract_unsigned_integer (lm_buf.l_name,
+				       sizeof (lm_buf.l_name),
+				       byte_order);
+      target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
+			  &errcode);
+
+      if (errcode != 0)
+	dsbt_index = 0;
+      else
 	{
-	  warning (_("dsbt_current_sos: Unable to read dsbt index."
-		     "  Shared object chain may be incomplete."));
-	  break;
+	  /* Fetch the DSBT index of this load module.  */
+	  dsbt_index = fetch_solib_dsbt_index (name_buf);
 	}
-      dsbt_index = extract_unsigned_integer (indexword, sizeof indexword,
-					     byte_order);
 
       /* If the DSBT index is zero, then we're looking at the entry
 	 for the main executable.  By convention, we don't include
 	 this in the list of shared objects.  */
       if (dsbt_index != 0)
 	{
-	  int errcode;
-	  char *name_buf;
 	  struct int_elf32_dsbt_loadmap *loadmap;
 	  struct so_list *sop;
 	  CORE_ADDR addr;
@@ -717,12 +830,6 @@ dsbt_current_sos (void)
 	  sop = xcalloc (1, sizeof (struct so_list));
 	  sop->lm_info = xcalloc (1, sizeof (struct lm_info));
 	  sop->lm_info->map = loadmap;
-	  /* Fetch the name.  */
-	  addr = extract_unsigned_integer (lm_buf.l_name,
-					   sizeof (lm_buf.l_name),
-					   byte_order);
-	  target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
-			      &errcode);
 
 	  if (errcode != 0)
 	    warning (_("Can't read pathname for link map entry: %s."),
@@ -744,6 +851,7 @@ dsbt_current_sos (void)
 	}
       else
 	{
+	  xfree (name_buf);
 	  info->main_lm_addr = lm_addr;
 	}
 

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Pedro Alves]

On 08/08/2013 05:41 PM, Luis Machado wrote:

> Here is an updated patch that deals with solib-dsbt.c as well. The
> additional tic6x-specific fields from the loadmap were removed and
> dsbt_index is now fetched from the shared library file on disk instead
> of grabbing such information from the runtime loadmap. That information
> does not change anyway.

Is there no DT_DEBUG in auvx in FDPIC/DSBT we could consult first?

>
> +/* Scan for DYNTAG in .dynamic section of ABFD. If DYNTAG is found 1

Double space after period.

> +   is returned and the corresponding PTR is set.  We only search in
> +   the BFD, not in the target's memory.  */
> +
> +static int
> +scan_dyntag_in_bfd (int dyntag, bfd *abfd, CORE_ADDR *ptr)
> +{
> +  int step, sect_size;
> +  long dyn_tag;
> +  CORE_ADDR dyn_ptr, dyn_addr;
> +  gdb_byte *bufend, *bufstart, *buf;
> +  Elf64_External_Dyn *x_dynp_64;
> +  struct bfd_section *sect;
> +  struct target_section *target_section;
> +
> +  if (abfd == NULL)
> +    return 0;
> +
> +  if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
> +    return 0;
> +
> +  /* Make sure the size is sane.  */
> +  if (bfd_get_arch_size (abfd) == -1)
> +    return 0;
> +
> +  /* Find the start address of the .dynamic section.  */
> +  sect = bfd_get_section_by_name (abfd, ".dynamic");
> +  if (sect == NULL)
> +    return 0;
> +
> +  for (target_section = current_target_sections->sections;
> +       target_section < current_target_sections->sections_end;
> +       target_section++)
> +    if (sect == target_section->the_bfd_section)
> +      break;
> +  if (target_section < current_target_sections->sections_end)
> +    dyn_addr = target_section->addr;
> +  else
> +    {
> +      /* ABFD may come from OBJFILE acting only as a symbol file
> +	 without being loaded into the target
> +	 (see add_symbol_file_command).  This case is such fallback
> +	 to the file VMA address without the possibility of having
> +	 the section relocated to its actual in-memory address.  */
> +
> +      dyn_addr = bfd_section_vma (abfd, sect);
> +    }
> +
> +  /* Read in .dynamic from the BFD.  */
> +  sect_size = bfd_section_size (abfd, sect);
> +  buf = bufstart = alloca (sect_size);
> +  if (!bfd_get_section_contents (abfd, sect,
> +				 buf, 0, sect_size))
> +    return 0;
> +
> +  /* Iterate over BUF and scan for DYNTAG.  If found, set PTR and
> +     return.  */
> +  step = sizeof (Elf64_External_Dyn);
> +
> +  for (bufend = buf + sect_size;
> +       buf < bufend;
> +       buf += step)
> +  {
> +    x_dynp_64 = (Elf64_External_Dyn *) buf;
> +    dyn_tag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
> +    dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
> +
> +    if (dyn_tag == DT_NULL)
> +      return 0;
> +    if (dyn_tag == dyntag)
> +      {
> +	if (ptr)

	if (ptr != NULL)

> +	  {
> +	    /* We don't want to read this information from memory.
> +	       If a relocation is needed, it should be done
> +	       elsewhere.  */
> +	    *ptr = dyn_ptr;
> +	  }
> +	return 1;
> +      }
> +  }
> +
> +  return 0;
> +}
> +

Would solib-svr4.c's scan_dyntag work pristine here?
This duplication is making me like Maciej's idea of a
shared-between-elf-ish-solib-backends solib-elf.c file more.

> +/* Given a shared library filename, load it up and find
> +   out what is its dsbt index.  */
> +
> +static int
> +fetch_solib_dsbt_index (char *filename)

could be const?

> +      target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
> +			  &errcode);
> +
> +      if (errcode != 0)
> +	dsbt_index = 0;

Is this an error path?  Should it still have the warning removed below?

> +      else
>  	{
> -	  warning (_("dsbt_current_sos: Unable to read dsbt index."
> -		     "  Shared object chain may be incomplete."));
> -	  break;
> +	  /* Fetch the DSBT index of this load module.  */
> +	  dsbt_index = fetch_solib_dsbt_index (name_buf);
>  	}
> -      dsbt_index = extract_unsigned_integer (indexword, sizeof indexword,
> -					     byte_order);
>

the bit where that above was moved from:

>  	  sop->lm_info->map = loadmap;
> -	  /* Fetch the name.  */
> -	  addr = extract_unsigned_integer (lm_buf.l_name,
> -					   sizeof (lm_buf.l_name),
> -					   byte_order);
> -	  target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
> -			      &errcode);
>
>  	  if (errcode != 0)
>  	    warning (_("Can't read pathname for link map entry: %s."),

had the warning, but it isn't reachable, due to

  if (dsbt_index != 0)

above.

Otherwise this is fine with me, if fine with Yao.

-- 
Pedro Alves

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Luis Machado]

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

Hi,

On 08/09/2013 12:08 PM, Pedro Alves wrote:
> On 08/08/2013 05:41 PM, Luis Machado wrote:
>
>> Here is an updated patch that deals with solib-dsbt.c as well. The
>> additional tic6x-specific fields from the loadmap were removed and
>> dsbt_index is now fetched from the shared library file on disk instead
>> of grabbing such information from the runtime loadmap. That information
>> does not change anyway.
>
> Is there no DT_DEBUG in auvx in FDPIC/DSBT we could consult first?
>

Possibly. The main problem here is making sure whatever change we make 
works reliably without access to real hardware for testing. The code in 
question, to read data from the file, is a bit simpler.

>>
>> +/* Scan for DYNTAG in .dynamic section of ABFD. If DYNTAG is found 1
>
> Double space after period.
>

Silly mistake always there.

>> +   is returned and the corresponding PTR is set.  We only search in
>> +   the BFD, not in the target's memory.  */
>> +
>> +static int
>> +scan_dyntag_in_bfd (int dyntag, bfd *abfd, CORE_ADDR *ptr)
>> +{
>> +  int step, sect_size;
>> +  long dyn_tag;
>> +  CORE_ADDR dyn_ptr, dyn_addr;
>> +  gdb_byte *bufend, *bufstart, *buf;
>> +  Elf64_External_Dyn *x_dynp_64;
>> +  struct bfd_section *sect;
>> +  struct target_section *target_section;
>> +
>> +  if (abfd == NULL)
>> +    return 0;
>> +
>> +  if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
>> +    return 0;
>> +
>> +  /* Make sure the size is sane.  */
>> +  if (bfd_get_arch_size (abfd) == -1)
>> +    return 0;
>> +
>> +  /* Find the start address of the .dynamic section.  */
>> +  sect = bfd_get_section_by_name (abfd, ".dynamic");
>> +  if (sect == NULL)
>> +    return 0;
>> +
>> +  for (target_section = current_target_sections->sections;
>> +       target_section < current_target_sections->sections_end;
>> +       target_section++)
>> +    if (sect == target_section->the_bfd_section)
>> +      break;
>> +  if (target_section < current_target_sections->sections_end)
>> +    dyn_addr = target_section->addr;
>> +  else
>> +    {
>> +      /* ABFD may come from OBJFILE acting only as a symbol file
>> +	 without being loaded into the target
>> +	 (see add_symbol_file_command).  This case is such fallback
>> +	 to the file VMA address without the possibility of having
>> +	 the section relocated to its actual in-memory address.  */
>> +
>> +      dyn_addr = bfd_section_vma (abfd, sect);
>> +    }
>> +
>> +  /* Read in .dynamic from the BFD.  */
>> +  sect_size = bfd_section_size (abfd, sect);
>> +  buf = bufstart = alloca (sect_size);
>> +  if (!bfd_get_section_contents (abfd, sect,
>> +				 buf, 0, sect_size))
>> +    return 0;
>> +
>> +  /* Iterate over BUF and scan for DYNTAG.  If found, set PTR and
>> +     return.  */
>> +  step = sizeof (Elf64_External_Dyn);
>> +
>> +  for (bufend = buf + sect_size;
>> +       buf < bufend;
>> +       buf += step)
>> +  {
>> +    x_dynp_64 = (Elf64_External_Dyn *) buf;
>> +    dyn_tag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
>> +    dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
>> +
>> +    if (dyn_tag == DT_NULL)
>> +      return 0;
>> +    if (dyn_tag == dyntag)
>> +      {
>> +	if (ptr)
>
> 	if (ptr != NULL)
>
>> +	  {
>> +	    /* We don't want to read this information from memory.
>> +	       If a relocation is needed, it should be done
>> +	       elsewhere.  */
>> +	    *ptr = dyn_ptr;
>> +	  }
>> +	return 1;
>> +      }
>> +  }
>> +
>> +  return 0;
>> +}
>> +
>
> Would solib-svr4.c's scan_dyntag work pristine here?
> This duplication is making me like Maciej's idea of a
> shared-between-elf-ish-solib-backends solib-elf.c file more.
>

It needs some tweaking to take into account relocations and it needs 
some help to locate the shared library's .dynamic section. From there we 
can extract the DSBT_INDEX. But, once again, the problem here is making 
sure such a change works reliably without hardware to test on.

>> +/* Given a shared library filename, load it up and find
>> +   out what is its dsbt index.  */
>> +
>> +static int
>> +fetch_solib_dsbt_index (char *filename)
>
> could be const?
>

Aye.

>> +      target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
>> +			  &errcode);
>> +
>> +      if (errcode != 0)
>> +	dsbt_index = 0;
>
> Is this an error path?  Should it still have the warning removed below?
>

Yeah. I've pulled the path warning from the inner block to the outer 
one. It also warns about the lack of a dsbt_index. The default index in 
case of error is -1 now. Only the main executable can have dsbt index 0, 
and there can be only one.

>> +      else
>>   	{
>> -	  warning (_("dsbt_current_sos: Unable to read dsbt index."
>> -		     "  Shared object chain may be incomplete."));
>> -	  break;
>> +	  /* Fetch the DSBT index of this load module.  */
>> +	  dsbt_index = fetch_solib_dsbt_index (name_buf);
>>   	}
>> -      dsbt_index = extract_unsigned_integer (indexword, sizeof indexword,
>> -					     byte_order);
>>
>
> the bit where that above was moved from:
>
>>   	  sop->lm_info->map = loadmap;
>> -	  /* Fetch the name.  */
>> -	  addr = extract_unsigned_integer (lm_buf.l_name,
>> -					   sizeof (lm_buf.l_name),
>> -					   byte_order);
>> -	  target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
>> -			      &errcode);
>>
>>   	  if (errcode != 0)
>>   	    warning (_("Can't read pathname for link map entry: %s."),
>
> had the warning, but it isn't reachable, due to
>
>    if (dsbt_index != 0)
>
> above.
>
> Otherwise this is fine with me, if fine with Yao.
>

How does this version look?

Thanks for reviewing.

[-- Attachment #2: dsbt.diff --]
[-- Type: text/x-patch, Size: 11460 bytes --]

2013-08-09  Luis Machado  <lgustavo@codesourcery.com>

	gdb/gdbserver
	* linux-low.c: Remove check for PT_GETDSBT.
	(target_loadmap): Remove data structure conditionalized by
	the presence of PT_GETDSBT.
	(LINUX_LOADMAP, LINUX_LOADMAP_EXEC,
	LINUX_LOADMAP_INTERP): Remove definitions.
	(linux_read_loadmap): Replace LINUX_LOADMAP_EXEC with
	PTRACE_GETFDPIC_EXEC, LINUX_LOADMAP_INTERP with
	PTRACE_GETFDPIC_INTERP and LINUX_LOADMAP with PTRACE_GETFDPIC.
	Do not set linux_read_loadmap to NULL in the absence of
	PTRACE_GETFDPIC.
	(linux_target_ops) <read_loadmap>: Only set to linux_read_loadmap
	in the presence of PTRACE_GETFDPIC.

	gdb/
	* solib-dsbt.c (DT_TIC6X_DSBT_INDEX): New definition.
	(ext_elf32_dsbt_loadseg) <dsbt_table_ptr>: Remove.
	(ext_elf32_dsbt_loadseg) <dsbt_size, dsbt_index>: Remove.
	(int_elf32_dsbt_loadmap) <dsbt_table_ptr>: Remove.
	(int_elf32_dsbt_loadmap) <dsbt_size, dsbt_index>: Remove.
	(scan_dyntag_in_bfd): New function.
	(fetch_solib_dsbt_index): New function.
	(dsbt_current_sos): Extract dsbt_index based on the
	shared library filename.

 gdb/gdbserver/linux-low.c |   37 +++------
 gdb/solib-dsbt.c          |  188 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 167 insertions(+), 58 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 47ea76d..4013829 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5262,7 +5262,7 @@ linux_qxfer_spu (const char *annex, unsigned char *readbuf,
   return ret;
 }
 
-#if defined PT_GETDSBT || defined PTRACE_GETFDPIC
+#if defined PTRACE_GETFDPIC
 struct target_loadseg
 {
   /* Core address to which the segment is mapped.  */
@@ -5273,23 +5273,6 @@ struct target_loadseg
   Elf32_Word p_memsz;
 };
 
-# if defined PT_GETDSBT
-struct target_loadmap
-{
-  /* Protocol version number, must be zero.  */
-  Elf32_Word version;
-  /* Pointer to the DSBT table, its size, and the DSBT index.  */
-  unsigned *dsbt_table;
-  unsigned dsbt_size, dsbt_index;
-  /* Number of segments in this map.  */
-  Elf32_Word nsegs;
-  /* The actual memory map.  */
-  struct target_loadseg segs[/*nsegs*/];
-};
-#  define LINUX_LOADMAP		PT_GETDSBT
-#  define LINUX_LOADMAP_EXEC	PTRACE_GETDSBT_EXEC
-#  define LINUX_LOADMAP_INTERP	PTRACE_GETDSBT_INTERP
-# else
 struct target_loadmap
 {
   /* Protocol version number, must be zero.  */
@@ -5299,10 +5282,6 @@ struct target_loadmap
   /* The actual memory map.  */
   struct target_loadseg segs[/*nsegs*/];
 };
-#  define LINUX_LOADMAP		PTRACE_GETFDPIC
-#  define LINUX_LOADMAP_EXEC	PTRACE_GETFDPIC_EXEC
-#  define LINUX_LOADMAP_INTERP	PTRACE_GETFDPIC_INTERP
-# endif
 
 static int
 linux_read_loadmap (const char *annex, CORE_ADDR offset,
@@ -5314,13 +5293,13 @@ linux_read_loadmap (const char *annex, CORE_ADDR offset,
   unsigned int actual_length, copy_length;
 
   if (strcmp (annex, "exec") == 0)
-    addr = (int) LINUX_LOADMAP_EXEC;
+    addr = (int) PTRACE_GETFDPIC_EXEC;
   else if (strcmp (annex, "interp") == 0)
-    addr = (int) LINUX_LOADMAP_INTERP;
+    addr = (int) PTRACE_GETFDPIC_INTERP;
   else
     return -1;
 
-  if (ptrace (LINUX_LOADMAP, pid, addr, &data) != 0)
+  if (ptrace (PTRACE_GETFDPIC, pid, addr, &data) != 0)
     return -1;
 
   if (data == NULL)
@@ -5336,9 +5315,7 @@ linux_read_loadmap (const char *annex, CORE_ADDR offset,
   memcpy (myaddr, (char *) data + offset, copy_length);
   return copy_length;
 }
-#else
-# define linux_read_loadmap NULL
-#endif /* defined PT_GETDSBT || defined PTRACE_GETFDPIC */
+#endif /* defined PTRACE_GETFDPIC */
 
 static void
 linux_process_qsupported (const char *query)
@@ -6036,7 +6013,11 @@ static struct target_ops linux_target_ops = {
   NULL,
 #endif
   linux_common_core_of_thread,
+#if defined PTRACE_GETFDPIC
   linux_read_loadmap,
+#else
+  NULL,
+#endif /* defined PTRACE_GETFDPIC */
   linux_process_qsupported,
   linux_supports_tracepoints,
   linux_read_pc,
diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 4fe24f8..95a0c09 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -33,6 +33,7 @@
 #include "gdb_bfd.h"
 
 #define GOT_MODULE_OFFSET 4
+#define DT_TIC6X_DSBT_INDEX 0x70000003
 
 /* Flag which indicates whether internal debug messages should be printed.  */
 static unsigned int solib_dsbt_debug = 0;
@@ -62,11 +63,6 @@ struct ext_elf32_dsbt_loadseg
 struct ext_elf32_dsbt_loadmap {
   /* Protocol version number, must be zero.  */
   ext_Elf32_Word version;
-  /* A pointer to the DSBT table; the DSBT size and the index of this
-     module.  */
-  ext_Elf32_Word dsbt_table_ptr;
-  ext_Elf32_Word dsbt_size;
-  ext_Elf32_Word dsbt_index;
   /* Number of segments in this map.  */
   ext_Elf32_Word nsegs;
   /* The actual memory map.  */
@@ -90,10 +86,6 @@ struct int_elf32_dsbt_loadmap
 {
   /* Protocol version number, must be zero.  */
   int version;
-  CORE_ADDR dsbt_table_ptr;
-  /* A pointer to the DSBT table; the DSBT size and the index of this
-     module.  */
-  int dsbt_size, dsbt_index;
   /* Number of segments in this map.  */
   int nsegs;
   /* The actual memory map.  */
@@ -619,6 +611,123 @@ lm_base (void)
   return info->lm_base_cache;
 }
 
+/* Scan for DYNTAG in .dynamic section of ABFD.  If DYNTAG is found 1
+   is returned and the corresponding PTR is set.  We only search in
+   the BFD, not in the target's memory.  */
+
+static int
+scan_dyntag_in_bfd (int dyntag, bfd *abfd, CORE_ADDR *ptr)
+{
+  int step, sect_size;
+  long dyn_tag;
+  CORE_ADDR dyn_ptr, dyn_addr;
+  gdb_byte *bufend, *bufstart, *buf;
+  Elf64_External_Dyn *x_dynp_64;
+  struct bfd_section *sect;
+  struct target_section *target_section;
+
+  if (abfd == NULL)
+    return 0;
+
+  if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
+    return 0;
+
+  /* Make sure the size is sane.  */
+  if (bfd_get_arch_size (abfd) == -1)
+    return 0;
+
+  /* Find the start address of the .dynamic section.  */
+  sect = bfd_get_section_by_name (abfd, ".dynamic");
+  if (sect == NULL)
+    return 0;
+
+  for (target_section = current_target_sections->sections;
+       target_section < current_target_sections->sections_end;
+       target_section++)
+    if (sect == target_section->the_bfd_section)
+      break;
+  if (target_section < current_target_sections->sections_end)
+    dyn_addr = target_section->addr;
+  else
+    {
+      /* ABFD may come from OBJFILE acting only as a symbol file
+	 without being loaded into the target
+	 (see add_symbol_file_command).  This case is such fallback
+	 to the file VMA address without the possibility of having
+	 the section relocated to its actual in-memory address.  */
+
+      dyn_addr = bfd_section_vma (abfd, sect);
+    }
+
+  /* Read in .dynamic from the BFD.  */
+  sect_size = bfd_section_size (abfd, sect);
+  buf = bufstart = alloca (sect_size);
+  if (!bfd_get_section_contents (abfd, sect,
+				 buf, 0, sect_size))
+    return 0;
+
+  /* Iterate over BUF and scan for DYNTAG.  If found, set PTR and
+     return.  */
+  step = sizeof (Elf64_External_Dyn);
+
+  for (bufend = buf + sect_size;
+       buf < bufend;
+       buf += step)
+  {
+    x_dynp_64 = (Elf64_External_Dyn *) buf;
+    dyn_tag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
+    dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
+
+    if (dyn_tag == DT_NULL)
+      return 0;
+    if (dyn_tag == dyntag)
+      {
+	if (ptr)
+	  {
+	    /* We don't want to read this information from memory.
+	       If a relocation is needed, it should be done
+	       elsewhere.  */
+	    *ptr = dyn_ptr;
+	  }
+	return 1;
+      }
+  }
+
+  return 0;
+}
+
+/* Given a shared library filename, load it up and find
+   out what is its dsbt index.  */
+
+static int
+fetch_solib_dsbt_index (const char *filename)
+{
+  unsigned long dsbt_index;
+  CORE_ADDR addr;
+  bfd *solib_bfd = NULL;
+  volatile struct gdb_exception ex;
+
+  if (filename == NULL)
+    return -1;
+
+  /* Open the shared library.  */
+  TRY_CATCH (ex, RETURN_MASK_ALL)
+    {
+      solib_bfd = solib_bfd_open ((char *) filename);
+    }
+  if (solib_bfd == NULL)
+    {
+      return -1;
+    }
+
+  /* Fetch the DSBT_INDEX from the shared library file on disk.  */
+  if (scan_dyntag_in_bfd (DT_TIC6X_DSBT_INDEX, solib_bfd, &addr) == 0)
+    return -1;
+
+  bfd_close (solib_bfd);
+
+  return addr;
+}
 
 /* Build a list of `struct so_list' objects describing the shared
    objects currently loaded in the inferior.  This list does not
@@ -661,10 +770,12 @@ dsbt_current_sos (void)
   while (lm_addr)
     {
       struct ext_link_map lm_buf;
-      ext_Elf32_Word indexword;
       CORE_ADDR map_addr;
       int dsbt_index;
       int ret;
+      CORE_ADDR addr;
+      int errcode;
+      char *name_buf = NULL;
 
       if (solib_dsbt_debug)
 	fprintf_unfiltered (gdb_stdlog,
@@ -684,24 +795,50 @@ dsbt_current_sos (void)
 					   sizeof lm_buf.l_addr.map,
 					   byte_order);
 
-      ret = target_read_memory (map_addr + 12, (gdb_byte *) &indexword,
-				sizeof indexword);
-      if (ret)
+      /* Fetch the name.  */
+      addr = extract_unsigned_integer (lm_buf.l_name,
+				       sizeof (lm_buf.l_name),
+				       byte_order);
+      target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
+			  &errcode);
+
+      if (errcode != 0)
 	{
-	  warning (_("dsbt_current_sos: Unable to read dsbt index."
-		     "  Shared object chain may be incomplete."));
-	  break;
+	  warning (_("dsbt_current_sos: Can't read pathname for link "
+		   "map entry: %s."), safe_strerror (errcode));
+
+	  /* Since we do not have a pathname, just assume that
+	     dsbt_index equals a dummy value of 1.  This way we can
+	     still add this library to the list, though it does not
+	     have a valid pathname.  We can't set it to 0 as only
+	     the main executable is allowed to have dsbt index 0.  */
+	  dsbt_index = -1;
+	}
+      else
+	{
+	  /* Fetch the DSBT index of this load module.  */
+	  dsbt_index = fetch_solib_dsbt_index (name_buf);
+
+	  if (dsbt_index < 0)
+	    {
+	      /* Something went wrong while fetching the dsbt
+		 index.  Warn about it.  */
+	      warning (_("dsbt_current_sos: Unable to read dsbt index."
+		       "  Shared object chain may be incomplete."));
+	    }
+	  else if (dsbt_index == 0)
+	    {
+	      /* This is the main executable, we do not need its
+		 name.  */
+	      xfree (name_buf);
+	    }
 	}
-      dsbt_index = extract_unsigned_integer (indexword, sizeof indexword,
-					     byte_order);
 
       /* If the DSBT index is zero, then we're looking at the entry
 	 for the main executable.  By convention, we don't include
 	 this in the list of shared objects.  */
       if (dsbt_index != 0)
 	{
-	  int errcode;
-	  char *name_buf;
 	  struct int_elf32_dsbt_loadmap *loadmap;
 	  struct so_list *sop;
 	  CORE_ADDR addr;
@@ -717,17 +854,8 @@ dsbt_current_sos (void)
 	  sop = xcalloc (1, sizeof (struct so_list));
 	  sop->lm_info = xcalloc (1, sizeof (struct lm_info));
 	  sop->lm_info->map = loadmap;
-	  /* Fetch the name.  */
-	  addr = extract_unsigned_integer (lm_buf.l_name,
-					   sizeof (lm_buf.l_name),
-					   byte_order);
-	  target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
-			      &errcode);
 
-	  if (errcode != 0)
-	    warning (_("Can't read pathname for link map entry: %s."),
-		     safe_strerror (errcode));
-	  else
+	  if (errcode == 0)
 	    {
 	      if (solib_dsbt_debug)
 		fprintf_unfiltered (gdb_stdlog, "current_sos: name = %s\n",

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Yao Qi]

On 08/10/2013 01:20 AM, Luis Machado wrote:
> +/* Scan for DYNTAG in .dynamic section of ABFD.  If DYNTAG is found 1
> +   is returned and the corresponding PTR is set.  We only search in
> +   the BFD, not in the target's memory.  */
> +
> +static int
> +scan_dyntag_in_bfd (int dyntag, bfd *abfd, CORE_ADDR *ptr)

Luis,
We've already have a function scan_dyntag in solib-dsbt.c which reads
in both BFD and the target memory.  There are some duplications in
scan_dyntag and scan_dyntag_in_bfd.  We can combine them into one
function, probably.


> +/* Given a shared library filename, load it up and find
> +   out what is its dsbt index.  */
> +
> +static int
> +fetch_solib_dsbt_index (const char *filename)
> +{
> +  unsigned long dsbt_index;
> +  CORE_ADDR addr;
> +  bfd *solib_bfd = NULL;
> +  volatile struct gdb_exception ex;
> +
> +  if (filename == NULL)
> +    return -1;
> +
> +  /* Open the shared library.  */
> +  TRY_CATCH (ex, RETURN_MASK_ALL)
> +    {
> +      solib_bfd = solib_bfd_open ((char *) filename);
> +    }
> +  if (solib_bfd == NULL)
> +    {
> +      return -1;
> +    }

Unnecessary braces.

> +
> +  /* Fetch the DSBT_INDEX from the shared library file on disk.  */
> +  if (scan_dyntag_in_bfd (DT_TIC6X_DSBT_INDEX, solib_bfd, &addr) == 0)

I don't find the definition of DT_TIC6X_DSBT_INDEX.  In uclibc, I only
find DT_C6000_DSBT_INDEX.

> @@ -684,24 +795,50 @@ dsbt_current_sos (void)
>   					   sizeof lm_buf.l_addr.map,
>   					   byte_order);
>
> -      ret = target_read_memory (map_addr + 12, (gdb_byte *) &indexword,
> -				sizeof indexword);
> -      if (ret)
> +      /* Fetch the name.  */
> +      addr = extract_unsigned_integer (lm_buf.l_name,
> +				       sizeof (lm_buf.l_name),
> +				       byte_order);
> +      target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
> +			  &errcode);
> +
> +      if (errcode != 0)
>   	{
> -	  warning (_("dsbt_current_sos: Unable to read dsbt index."
> -		     "  Shared object chain may be incomplete."));
> -	  break;
> +	  warning (_("dsbt_current_sos: Can't read pathname for link "
> +		   "map entry: %s."), safe_strerror (errcode));
> +
> +	  /* Since we do not have a pathname, just assume that
> +	     dsbt_index equals a dummy value of 1.  This way we can
                                                ^^ -1?

-- 
Yao (齐尧)

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Luis Machado]

Hi,

On 08/09/2013 09:32 PM, Yao Qi wrote:
> On 08/10/2013 01:20 AM, Luis Machado wrote:
>> +/* Scan for DYNTAG in .dynamic section of ABFD.  If DYNTAG is found 1
>> +   is returned and the corresponding PTR is set.  We only search in
>> +   the BFD, not in the target's memory.  */
>> +
>> +static int
>> +scan_dyntag_in_bfd (int dyntag, bfd *abfd, CORE_ADDR *ptr)
>
> Luis,
> We've already have a function scan_dyntag in solib-dsbt.c which reads
> in both BFD and the target memory.  There are some duplications in
> scan_dyntag and scan_dyntag_in_bfd.  We can combine them into one
> function, probably.

I seem to recall the code to read target memory, in the case of targets 
that do relocations based on loadmaps, get this information wrong, based 
on past experience with a different DSBT-based target.

 From what i recall, it tries to read the dsbt_index from the main 
executable's .dynamic, but the information is in the solib's .dynamic 
section.

I can't be sure it works for c6x since there is no target for me to test 
on. If someone has access to that, i can coordinate some testing. This 
is the biggest problem right now.

>
>
>> +/* Given a shared library filename, load it up and find
>> +   out what is its dsbt index.  */
>> +
>> +static int
>> +fetch_solib_dsbt_index (const char *filename)
>> +{
>> +  unsigned long dsbt_index;
>> +  CORE_ADDR addr;
>> +  bfd *solib_bfd = NULL;
>> +  volatile struct gdb_exception ex;
>> +
>> +  if (filename == NULL)
>> +    return -1;
>> +
>> +  /* Open the shared library.  */
>> +  TRY_CATCH (ex, RETURN_MASK_ALL)
>> +    {
>> +      solib_bfd = solib_bfd_open ((char *) filename);
>> +    }
>> +  if (solib_bfd == NULL)
>> +    {
>> +      return -1;
>> +    }
>
> Unnecessary braces.

Fixed.

>
>> +
>> +  /* Fetch the DSBT_INDEX from the shared library file on disk.  */
>> +  if (scan_dyntag_in_bfd (DT_TIC6X_DSBT_INDEX, solib_bfd, &addr) == 0)
>
> I don't find the definition of DT_TIC6X_DSBT_INDEX.  In uclibc, I only
> find DT_C6000_DSBT_INDEX.
>

That is exactly what it is. I named it DT_TIC6X_DSBT_INDEX to make it 
more meaningful, but i can use DT_C6000_DSBT_INDEX without problems i think.

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Pedro Alves]

On 08/12/2013 02:41 PM, Luis Machado wrote:

>>> +
>>> +  /* Fetch the DSBT_INDEX from the shared library file on disk.  */
>>> +  if (scan_dyntag_in_bfd (DT_TIC6X_DSBT_INDEX, solib_bfd, &addr) == 0)
>>
>> I don't find the definition of DT_TIC6X_DSBT_INDEX.  In uclibc, I only
>> find DT_C6000_DSBT_INDEX.
>>
> 
> That is exactly what it is. I named it DT_TIC6X_DSBT_INDEX to make it 
> more meaningful, but i can use DT_C6000_DSBT_INDEX without problems i think.

Indeed.  The kernel (arch/c6x/include/asm/elf.h) has:

 85 /* C6X specific DT_ tags */
 86 #define DT_C6000_DSBT_BASE      0x70000000
 87 #define DT_C6000_DSBT_SIZE      0x70000001
 88 #define DT_C6000_PREEMPTMAP     0x70000002
 89 #define DT_C6000_DSBT_INDEX     0x70000003

BFD also already has (include/elf/tic6x.h):

 /* The hard-coded DSBT index for this module, if any. */
 #define DT_C6000_DSBT_INDEX 0x70000003

IMO, yes, better stick with existing practice and not
invent new symbols.

-- 
Pedro Alves

Re: [PATCH, gdbserver] Further cleanup of FDPIC/DSBT divergences

[Mike Frysinger]

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

On Friday 09 August 2013 13:20:24 Luis Machado wrote:
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> 
>  }
> -#else
> -# define linux_read_loadmap NULL
> -#endif /* defined PT_GETDSBT || defined PTRACE_GETFDPIC */
> +#endif /* defined PTRACE_GETFDPIC */
> 
> @@ -6036,7 +6013,11 @@ static struct target_ops linux_target_ops = {
>    linux_common_core_of_thread,
> +#if defined PTRACE_GETFDPIC
>    linux_read_loadmap,
> +#else
> +  NULL,
> +#endif /* defined PTRACE_GETFDPIC */
>    linux_process_qsupported,
>    linux_supports_tracepoints,

personally, i liked the style before -- keep the logic where the func is 
defined rather than in the initialization of the target ops.

otherwise, the changes to the linux-low.c file LGTM
-mike

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