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

[Luis Machado <lgustavo@codesourcery.com>]

[-- 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;
 	}