Re: [patch] Implement qXfer:libraries for Linux/gdbserver

Jan Kratochvil
2011-08-08T21:24:36Z

On Mon, 08 Aug 2011 20:55:40 +0200, Paul Pluzhnikov wrote:
> On Mon, Aug 8, 2011 at 11:36 AM, Jan Kratochvil > <jan.kratochvil@redhat.com> wrote:
[...]
> The trouble is that solib-target.c's notion of what info is in solib->lm_info
> is radically different from solib-svr4.c's notion, each having a private
> and distinct 'struct lm_info'. Un-tangling that seemed troublesome.

I agree the solib-svr4.c split looks a bit complex to me.

> > It would make libexpat mandatory even for the linux-nat.c usage so libexpat
> > could be bundler into the sourceware tree.
>
> I think it's highly desirable to keep the current solib-srv4 code working
> (e.g. in case you have a gdbserver that answers with empty list; either
> because it is old, or because it's for a platform that hasn't been updated).
>
> Given above, you can always fall back to it when libexpat is not detected.

I tried to copy how windows-nat.c works, which even in local mode produces and
consumes the XML document, exchanged internally as TARGET_OBJECT_LIBRARIES.
Therefore the core svr4_current_sos function would also produce
TARGET_OBJECT_LIBRARIES XML document, even in local run.

Sure other solutions exist but this unifies the remote and local codepath the
most.

I do not think it is any problem to require + bundle libexpat, there was even
some FAQ of people with broken GDB for ARM remote targets before some proper
error messages were added indicating libexpat is missing for GDB.

> The producers would have to stay separate anyway, as the details of getting
> the list differ greatly.

While it is a detail it saves 46 LoC (Lines of Code) and it IMO also makes the
code more readable. Getting the list and producing the XML are two different
parts of code.

> Yes, the two isolated consumers are a bit bothersome.

Unfortunately the solib-svr4.c part is not so trivial as this one but IMO it
should be also done, I could get to it only later.

Thanks,
Jan

gdb/gdbserver/
2011-08-08 Jan Kratochvil <jan.kratochvil@redhat.com>

* inferiors.c (clear_all_dlls): New function.
(clear_inferiors): Move there the code from here, call it here.
* linux-low.c (linux_qxfer_libraries): Rename to ...
(linux_refresh_libraries): ... here. Drop all the XML handling. Call
clear_all_dlls and loaded_dll instead. Clear DLLS_CHANGED.
(linux_target_ops): Rename linux_qxfer_libraries to
linux_refresh_libraries.
* server.c (handle_qxfer_libraries): Likewise. Continue execution even
if REFRESH_LIBRARIES exists.
* server.h (clear_all_dlls): New prototype.
* target.h (struct target_ops): Rename linux_qxfer_libraries to
linux_refresh_libraries. Drop its parameters and return type.

--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -312,14 +312,22 @@ unloaded_dll (const char *name, CORE_ADDR base_addr)
#define clear_list(LIST) \
do { (LIST)->head = (LIST)->tail = NULL; } while (0)

+/* Clear ALL_DLLS. */
+
void
-clear_inferiors (void)
+clear_all_dlls (void)
{
- for_each_inferior (&all_threads, free_one_thread);
for_each_inferior (&all_dlls, free_one_dll);
+ clear_list (&all_dlls);
+}

+void
+clear_inferiors (void)
+{
+ for_each_inferior (&all_threads, free_one_thread);
clear_list (&all_threads);
- clear_list (&all_dlls);
+
+ clear_all_dlls ();

current_inferior = NULL;
}
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4939,16 +4939,13 @@ struct link_map_offsets

/* Construct qXfer:libraries:read reply. */

-static int
-linux_qxfer_libraries (const char *annex, unsigned char *readbuf,
- unsigned const char *writebuf,
- CORE_ADDR offset, int len)
+static void
+linux_refresh_libraries (void)
{
- char *document;
- unsigned document_len;
struct process_info_private *const priv = current_process ()->private;
char filename[PATH_MAX];
- int pid, is_elf64;
+ int pid, is_elf64, ptr_size, r_version;
+ CORE_ADDR lm_addr, lm_prev, l_name, l_addr, l_next, l_prev;

static const struct link_map_offsets lmo_32bit_offsets =
{
@@ -4971,34 +4968,17 @@ linux_qxfer_libraries (const char *annex, unsigned char *readbuf,
};
const struct link_map_offsets *lmo;

- if (writebuf != NULL)
- return -2;
- if (readbuf == NULL)
- return -1;
-
pid = lwpid_of (get_thread_lwp (current_inferior));
xsnprintf (filename, sizeof filename, "/proc/%d/exe", pid);
is_elf64 = elf_64_file_p (filename);
lmo = is_elf64 ? &lmo_64bit_offsets : &lmo_32bit_offsets;
+ ptr_size = is_elf64 ? 8 : 4;

if (priv->r_debug == 0)
priv->r_debug = get_r_debug (pid, is_elf64);

if (priv->r_debug == (CORE_ADDR) -1 || priv->r_debug == 0)
- {
- document = xstrdup ("<library-list/>\n");
- }
- else
- {
- int allocated = 1024;
- char *p;
- const int ptr_size = is_elf64 ? 8 : 4;
- CORE_ADDR lm_addr, lm_prev, l_name, l_addr, l_next, l_prev;
- int r_version;
-
- document = xmalloc (allocated);
- strcpy (document, "<library-list>");
- p = document + strlen (document);
+ return;

r_version = 0;
if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
@@ -5007,7 +4987,7 @@ linux_qxfer_libraries (const char *annex, unsigned char *readbuf,
|| r_version != 1)
{
warning ("unexpected r_debug version %d", r_version);
- goto done;
+ return;
}

if (read_one_ptr (priv->r_debug + lmo->r_map_offset,
@@ -5015,9 +4995,11 @@ linux_qxfer_libraries (const char *annex, unsigned char *readbuf,
{
warning ("unable to read r_map from 0x%lx",
(long) priv->r_debug + lmo->r_map_offset);
- goto done;
+ return;
}

+ clear_all_dlls ();
+
lm_prev = 0;
while (read_one_ptr (lm_addr + lmo->l_name_offset,
&l_name, ptr_size) == 0
@@ -5042,33 +5024,7 @@ linux_qxfer_libraries (const char *annex, unsigned char *readbuf,
libname[0] = '\0';
linux_read_memory (l_name, libname, sizeof (libname));
if (libname[0] != '\0')
- {
- size_t len = strlen ((char *) libname);
- char *name;
-
- while (allocated < p - document + len + 100)
- {
- /* Expand to guarantee sufficient storage. */
- uintptr_t document_len = p - document;
-
- document = xrealloc (document, 2 * allocated);
- allocated *= 2;
- p = document + document_len;
- }
-
- strcpy (p, "<library name=\"");
- p = p + strlen (p);
- name = xml_escape_text ((char *) libname);
- strcpy (p, name);
- free (name);
- p = p + strlen (p);
- strcpy (p, "\"><segment address=\"");
- p = p + strlen (p);
- sprintf (p, "0x%lx", (long) l_addr);
- p = p + strlen (p);
- strcpy (p, "\"/></library>");
- p = p + strlen (p);
- }
+ loaded_dll ((const char *) libname, l_addr);

if (l_next == 0)
break;
@@ -5076,18 +5032,9 @@ linux_qxfer_libraries (const char *annex, unsigned char *readbuf,
lm_prev = lm_addr;
lm_addr = l_next;
}
- done:
- strcpy (p, "</library-list>");
- }

- document_len = strlen (document + offset);
- if (len > document_len)
- len = document_len;
-
- memcpy (readbuf, document + offset, len);
- xfree (document);
-
- return len;
+ /* The library notification response is not expected for GNU/Linux. */
+ dlls_changed = 0;
}

@@ -5150,7 +5097,7 @@ static struct target_ops linux_target_ops = {
linux_stabilize_threads,
linux_install_fast_tracepoint_jump_pad,
linux_emit_ops,
- linux_qxfer_libraries
+ linux_refresh_libraries
};

static void
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -918,8 +918,8 @@ handle_qxfer_libraries (const char *annex,
if (annex[0] != '\0' || !target_running ())
return -1;

- if (the_target->qxfer_libraries != NULL)
- return the_target->qxfer_libraries (annex, readbuf, writebuf, offset, len);
+ if (the_target->refresh_libraries != NULL)
+ the_target->refresh_libraries ();

/* Over-estimate the necessary memory. Assume that every character
in the library name must be escaped. */
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -260,6 +260,7 @@ ptid_t thread_id_to_gdb_id (ptid_t);
ptid_t thread_to_gdb_id (struct thread_info *);
ptid_t gdb_id_to_thread_id (ptid_t);
struct thread_info *gdb_id_to_thread (unsigned int);
+void clear_all_dlls (void);
void clear_inferiors (void);
struct inferior_list_entry *find_inferior
(struct inferior_list *,
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -374,10 +374,8 @@ struct target_ops
Returns NULL if bytecode compilation is not supported. */
struct emit_ops *(*emit_ops) (void);

- /* Read solib info. */
- int (*qxfer_libraries) (const char *annex, unsigned char *readbuf,
- unsigned const char *writebuf,
- CORE_ADDR offset, int len);
+ /* Refresh ALL_DLLS. */
+ void (*refresh_libraries) (void);
};

extern struct target_ops *the_target;