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

Luis Machado
2013-08-09T17:20:47Z

[-- 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",