Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH v2] Support glibc multiple namespace extension
@ 2021-10-02  0:11 H.J. Lu via Gdb-patches
  2021-10-02 20:43 ` [PATCH v3] " H.J. Lu via Gdb-patches
  0 siblings, 1 reply; 8+ messages in thread
From: H.J. Lu via Gdb-patches @ 2021-10-02  0:11 UTC (permalink / raw)
  To: GDB

Changes in v2:

1. Don't check shared libraries in other namespaces when updating shared
libraries in a new namespace.


H.J.
---
In glibc, the r_debug structure contains (amongst others) the following
fields:

  int r_version:
    Version number for this protocol.  It should be greater than 0.

If r_version is 2, struct r_debug is extended to struct r_debug_extended
with one additional field:

  struct r_debug_extended *r_next;
    Link to the next r_debug_extended structure.  Each r_debug_extended
    structure represents a different namespace.  The first r_debug_extended
    structure is for the default namespace.

1. Change solib_svr4_r_map argument to take the debug base.
2. Add solib_svr4_r_next to find the link map in the next namespace from
the r_next field.
3. Update svr4_current_sos_direct to get the link map in the next namespace
from the r_next field.
4. Don't check shared libraries in other namespaces when updating shared
libraries in a new namespace.

This fixes PR 11839.
---
 gdb/linux-tdep.c       |   2 +
 gdb/mips-fbsd-tdep.c   |   2 +
 gdb/mips-netbsd-tdep.c |   2 +
 gdb/solib-svr4.c       |  78 ++++++++++---
 gdb/solib-svr4.h       |   3 +
 gdbserver/linux-low.cc | 254 +++++++++++++++++++++++++----------------
 6 files changed, 227 insertions(+), 114 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index ae2f7c14f6d..a3b20070adc 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2744,6 +2744,7 @@ linux_ilp32_fetch_link_map_offsets ()
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = 20;
 
       /* Everything we need is in the first 20 bytes.  */
       lmo.link_map_size = 20;
@@ -2772,6 +2773,7 @@ linux_lp64_fetch_link_map_offsets ()
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = 40;
 
       /* Everything we need is in the first 40 bytes.  */
       lmo.link_map_size = 40;
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 0b7c97c445f..00e38a8f1c4 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -495,6 +495,7 @@ mips_fbsd_ilp32_fetch_link_map_offsets (void)
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       lmo.link_map_size = 24;
       lmo.l_addr_offset = 0;
@@ -522,6 +523,7 @@ mips_fbsd_lp64_fetch_link_map_offsets (void)
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       lmo.link_map_size = 48;
       lmo.l_addr_offset = 0;
diff --git a/gdb/mips-netbsd-tdep.c b/gdb/mips-netbsd-tdep.c
index a32ae5e3a29..12d702ef359 100644
--- a/gdb/mips-netbsd-tdep.c
+++ b/gdb/mips-netbsd-tdep.c
@@ -308,6 +308,7 @@ mipsnbsd_ilp32_fetch_link_map_offsets (void)
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 24 bytes.  */
       lmo.link_map_size = 24;
@@ -336,6 +337,7 @@ mipsnbsd_lp64_fetch_link_map_offsets (void)
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 40 bytes.  */
       lmo.link_map_size = 48;
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 3de1bb9c7f7..9d851ba8930 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -767,7 +767,7 @@ locate_base (struct svr4_info *info)
    RT_CONSISTENT.  */
 
 static CORE_ADDR
-solib_svr4_r_map (struct svr4_info *info)
+solib_svr4_r_map (CORE_ADDR debug_base)
 {
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
@@ -775,7 +775,7 @@ solib_svr4_r_map (struct svr4_info *info)
 
   try
     {
-      addr = read_memory_typed_address (info->debug_base + lmo->r_map_offset,
+      addr = read_memory_typed_address (debug_base + lmo->r_map_offset,
 					ptr_type);
     }
   catch (const gdb_exception_error &ex)
@@ -829,6 +829,37 @@ solib_svr4_r_ldsomap (struct svr4_info *info)
 				    ptr_type);
 }
 
+/* Find the next namespace from the r_next field.  */
+
+static CORE_ADDR
+solib_svr4_r_next (CORE_ADDR debug_base)
+{
+  struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
+  struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
+  enum bfd_endian byte_order = type_byte_order (ptr_type);
+  ULONGEST version = 0;
+
+  try
+    {
+      /* Check version, and return zero if `struct r_debug' doesn't have
+	 the r_next member.  */
+      version
+	= read_memory_unsigned_integer (debug_base + lmo->r_version_offset,
+					lmo->r_version_size, byte_order);
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      exception_print (gdb_stderr, ex);
+    }
+
+  /* The r_next field is added with r_version == 2.  */
+  if (version < 2 || lmo->r_next_offset == -1)
+    return 0;
+
+  return read_memory_typed_address (debug_base + lmo->r_next_offset,
+				    ptr_type);
+}
+
 /* On Solaris systems with some versions of the dynamic linker,
    ld.so's l_name pointer points to the SONAME in the string table
    rather than into writable memory.  So that GDB can find shared
@@ -886,7 +917,7 @@ open_symbol_file_object (int from_tty)
     return 0;	/* failed somehow...  */
 
   /* First link map member should be the executable.  */
-  lm = solib_svr4_r_map (info);
+  lm = solib_svr4_r_map (info->debug_base);
   if (lm == 0)
     return 0;	/* failed somehow...  */
 
@@ -1323,7 +1354,7 @@ svr4_current_sos_direct (struct svr4_info *info)
 
   /* Walk the inferior's link map list, and build our list of
      `struct so_list' nodes.  */
-  lm = solib_svr4_r_map (info);
+  lm = solib_svr4_r_map (info->debug_base);
   if (lm)
     svr4_read_so_list (info, lm, 0, &link_ptr, ignore_first);
 
@@ -1335,6 +1366,18 @@ svr4_current_sos_direct (struct svr4_info *info)
   if (lm)
     svr4_read_so_list (info, lm, 0, &link_ptr, 0);
 
+  /* Get the next namespace from the r_next field.  */
+  lm = solib_svr4_r_next (info->debug_base);
+  while (lm)
+    {
+      /* Get the link map in this namespace.  */
+      CORE_ADDR link_map = solib_svr4_r_map (lm);
+      if (link_map)
+	svr4_read_so_list (info, link_map, 0, &link_ptr, 0);
+      /* Go to the next namespace.  */
+      lm = solib_svr4_r_next (lm);
+    }
+
   cleanup.release ();
 
   if (head == NULL)
@@ -1706,7 +1749,8 @@ solist_update_full (struct svr4_info *info)
    failure.  */
 
 static int
-solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
+solist_update_incremental (struct svr4_info *info, CORE_ADDR debug_base,
+			   CORE_ADDR lm)
 {
   struct so_list *tail;
   CORE_ADDR prev_lm;
@@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
   for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
     /* Nothing.  */;
 
-  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
-  prev_lm = li->lm_addr;
+  /* Don't check shared libraries in other namespaces when updating
+     shared libraries in a new namespace.  */
+  if (debug_base == info->debug_base)
+    {
+      lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
+      prev_lm = li->lm_addr;
+    }
+  else
+    prev_lm = 0;
 
   /* Read the new objects.  */
   if (info->using_xfer)
@@ -1869,13 +1920,6 @@ svr4_handle_solib_event (void)
 	  return;
       }
 
-    /* GDB does not currently support libraries loaded via dlmopen
-       into namespaces other than the initial one.  We must ignore
-       any namespace other than the initial namespace here until
-       support for this is added to GDB.  */
-    if (debug_base != info->debug_base)
-      action = DO_NOTHING;
-
     if (action == UPDATE_OR_RELOAD)
       {
 	try
@@ -1901,7 +1945,7 @@ svr4_handle_solib_event (void)
 
   if (action == UPDATE_OR_RELOAD)
     {
-      if (!solist_update_incremental (info, lm))
+      if (!solist_update_incremental (info, debug_base, lm))
 	action = FULL_RELOAD;
     }
 
@@ -2136,7 +2180,7 @@ enable_break (struct svr4_info *info, int from_tty)
 
   solib_add (NULL, from_tty, auto_solib_add);
   sym_addr = 0;
-  if (info->debug_base && solib_svr4_r_map (info) != 0)
+  if (info->debug_base && solib_svr4_r_map (info->debug_base) != 0)
     sym_addr = solib_svr4_r_brk (info);
 
   if (sym_addr != 0)
@@ -3087,6 +3131,7 @@ svr4_ilp32_fetch_link_map_offsets (void)
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = 20;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 20 bytes.  */
       lmo.link_map_size = 20;
@@ -3118,6 +3163,7 @@ svr4_lp64_fetch_link_map_offsets (void)
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = 40;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 40 bytes.  */
       lmo.link_map_size = 40;
diff --git a/gdb/solib-svr4.h b/gdb/solib-svr4.h
index 8d94d9cb26e..64854e2edd9 100644
--- a/gdb/solib-svr4.h
+++ b/gdb/solib-svr4.h
@@ -66,6 +66,9 @@ struct link_map_offsets
     /* Offset of r_debug.r_ldsomap.  */
     int r_ldsomap_offset;
 
+    /* Offset of r_debug_extended.r_next.  */
+    int r_next_offset;
+
     /* Size of struct link_map (or equivalent), or at least enough of it
        to be able to obtain the fields below.  */
     int link_map_size;
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d27a2169029..5c3503ba69f 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -6733,6 +6733,9 @@ struct link_map_offsets
     /* Offset and size of r_debug.r_map.  */
     int r_map_offset;
 
+    /* Offset of r_debug_extended.r_next.  */
+    int r_next_offset;
+
     /* Offset to l_addr field in struct link_map.  */
     int l_addr_offset;
 
@@ -6749,117 +6752,59 @@ struct link_map_offsets
     int l_prev_offset;
   };
 
-/* Construct qXfer:libraries-svr4:read reply.  */
+static const struct link_map_offsets lmo_32bit_offsets =
+  {
+    0,     /* r_version offset. */
+    4,     /* r_debug.r_map offset.  */
+    20,    /* r_debug_extended.r_next.  */
+    0,     /* l_addr offset in link_map.  */
+    4,     /* l_name offset in link_map.  */
+    8,     /* l_ld offset in link_map.  */
+    12,    /* l_next offset in link_map.  */
+    16     /* l_prev offset in link_map.  */
+  };
 
-int
-linux_process_target::qxfer_libraries_svr4 (const char *annex,
-					    unsigned char *readbuf,
-					    unsigned const char *writebuf,
-					    CORE_ADDR offset, int len)
-{
-  struct process_info_private *const priv = current_process ()->priv;
-  char filename[PATH_MAX];
-  int pid, is_elf64;
+static const struct link_map_offsets lmo_64bit_offsets =
+  {
+    0,     /* r_version offset. */
+    8,     /* r_debug.r_map offset.  */
+    40,    /* r_debug_extended.r_next.  */
+    0,     /* l_addr offset in link_map.  */
+    8,     /* l_name offset in link_map.  */
+    16,    /* l_ld offset in link_map.  */
+    24,    /* l_next offset in link_map.  */
+    32     /* l_prev offset in link_map.  */
+  };
 
-  static const struct link_map_offsets lmo_32bit_offsets =
-    {
-      0,     /* r_version offset. */
-      4,     /* r_debug.r_map offset.  */
-      0,     /* l_addr offset in link_map.  */
-      4,     /* l_name offset in link_map.  */
-      8,     /* l_ld offset in link_map.  */
-      12,    /* l_next offset in link_map.  */
-      16     /* l_prev offset in link_map.  */
-    };
+/* Get the loaded shared libraries from one namespace.  */
 
-  static const struct link_map_offsets lmo_64bit_offsets =
-    {
-      0,     /* r_version offset. */
-      8,     /* r_debug.r_map offset.  */
-      0,     /* l_addr offset in link_map.  */
-      8,     /* l_name offset in link_map.  */
-      16,    /* l_ld offset in link_map.  */
-      24,    /* l_next offset in link_map.  */
-      32     /* l_prev offset in link_map.  */
-    };
-  const struct link_map_offsets *lmo;
-  unsigned int machine;
-  int ptr_size;
+static void
+read_link_map (std::string &document, CORE_ADDR r_debug, int ptr_size,
+	       const struct link_map_offsets *lmo, bool ignore_first,
+	       int &header_done)
+{
   CORE_ADDR lm_addr = 0, lm_prev = 0;
   CORE_ADDR l_name, l_addr, l_ld, l_next, l_prev;
-  int header_done = 0;
-
-  if (writebuf != NULL)
-    return -2;
-  if (readbuf == NULL)
-    return -1;
-
-  pid = lwpid_of (current_thread);
-  xsnprintf (filename, sizeof filename, "/proc/%d/exe", pid);
-  is_elf64 = elf_64_file_p (filename, &machine);
-  lmo = is_elf64 ? &lmo_64bit_offsets : &lmo_32bit_offsets;
-  ptr_size = is_elf64 ? 8 : 4;
-
-  while (annex[0] != '\0')
-    {
-      const char *sep;
-      CORE_ADDR *addrp;
-      int name_len;
-
-      sep = strchr (annex, '=');
-      if (sep == NULL)
-	break;
-
-      name_len = sep - annex;
-      if (name_len == 5 && startswith (annex, "start"))
-	addrp = &lm_addr;
-      else if (name_len == 4 && startswith (annex, "prev"))
-	addrp = &lm_prev;
-      else
-	{
-	  annex = strchr (sep, ';');
-	  if (annex == NULL)
-	    break;
-	  annex++;
-	  continue;
-	}
-
-      annex = decode_address_to_semicolon (addrp, sep + 1);
-    }
 
   if (lm_addr == 0)
     {
       int r_version = 0;
 
-      if (priv->r_debug == 0)
-	priv->r_debug = get_r_debug (pid, is_elf64);
-
-      /* We failed to find DT_DEBUG.  Such situation will not change
-	 for this inferior - do not retry it.  Report it to GDB as
-	 E01, see for the reasons at the GDB solib-svr4.c side.  */
-      if (priv->r_debug == (CORE_ADDR) -1)
-	return -1;
-
-      if (priv->r_debug != 0)
+      if (linux_read_memory (r_debug + lmo->r_version_offset,
+			     (unsigned char *) &r_version,
+			     sizeof (r_version)) != 0
+	  || r_version < 1)
 	{
-	  if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
-				 (unsigned char *) &r_version,
-				 sizeof (r_version)) != 0
-	      || r_version < 1)
-	    {
-	      warning ("unexpected r_debug version %d", r_version);
-	    }
-	  else if (read_one_ptr (priv->r_debug + lmo->r_map_offset,
-				 &lm_addr, ptr_size) != 0)
-	    {
-	      warning ("unable to read r_map from 0x%lx",
-		       (long) priv->r_debug + lmo->r_map_offset);
-	    }
+	  warning ("unexpected r_debug version %d", r_version);
+	}
+      else if (read_one_ptr (r_debug + lmo->r_map_offset,
+			     &lm_addr, ptr_size) != 0)
+	{
+	  warning ("unable to read r_map from 0x%lx",
+		   (long) r_debug + lmo->r_map_offset);
 	}
     }
 
-  std::string document = "<library-list-svr4 version=\"1.0\"";
-
   while (lm_addr
 	 && read_one_ptr (lm_addr + lmo->l_name_offset,
 			  &l_name, ptr_size) == 0
@@ -6887,7 +6832,7 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
 	 (see solib-svr4.c parameter ignore_first).  But in such case the main
 	 executable does not have PT_DYNAMIC present and this function already
 	 exited above due to failed get_r_debug.  */
-      if (lm_prev == 0)
+      if (ignore_first && lm_prev == 0)
 	string_appendf (document, " main-lm=\"0x%lx\"", (unsigned long) lm_addr);
       else
 	{
@@ -6917,6 +6862,119 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
       lm_prev = lm_addr;
       lm_addr = l_next;
     }
+}
+
+/* Find the next namespace from the r_next field.  */
+
+static CORE_ADDR
+get_r_next (CORE_ADDR r_debug, int ptr_size,
+	    const struct link_map_offsets *lmo)
+{
+  int r_version = 0;
+  CORE_ADDR lm_addr = 0;
+  if (linux_read_memory (r_debug + lmo->r_version_offset,
+			 (unsigned char *) &r_version,
+			 sizeof (r_version)) == 0
+      && r_version >= 2)
+    {
+      if (read_one_ptr (r_debug + lmo->r_next_offset,
+			&lm_addr, ptr_size) != 0)
+	warning ("unable to read r_next from 0x%lx",
+		 (long) r_debug + lmo->r_next_offset);
+    }
+  return lm_addr;
+}
+
+/* Construct qXfer:libraries-svr4:read reply.  */
+
+int
+linux_process_target::qxfer_libraries_svr4 (const char *annex,
+					    unsigned char *readbuf,
+					    unsigned const char *writebuf,
+					    CORE_ADDR offset, int len)
+{
+  struct process_info_private *const priv = current_process ()->priv;
+  char filename[PATH_MAX];
+  int pid, is_elf64;
+  unsigned int machine;
+  CORE_ADDR lm_addr = 0, lm_prev = 0;
+  int header_done = 0;
+
+  if (writebuf != NULL)
+    return -2;
+  if (readbuf == NULL)
+    return -1;
+
+  pid = lwpid_of (current_thread);
+  xsnprintf (filename, sizeof filename, "/proc/%d/exe", pid);
+  is_elf64 = elf_64_file_p (filename, &machine);
+  const struct link_map_offsets *lmo;
+  int ptr_size;
+  if (is_elf64)
+    {
+      lmo = &lmo_64bit_offsets;
+      ptr_size = 8;
+    }
+  else
+    {
+      lmo = &lmo_32bit_offsets;
+      ptr_size = 4;
+    }
+  CORE_ADDR r_debug = priv->r_debug;
+  if (r_debug == 0)
+    r_debug = get_r_debug (pid, is_elf64);
+
+  /* We failed to find DT_DEBUG.  Such situation will not change
+     for this inferior - do not retry it.  Report it to GDB as
+     E01, see for the reasons at the GDB solib-svr4.c side.  */
+  if (r_debug == (CORE_ADDR) -1)
+    return -1;
+
+  while (annex[0] != '\0')
+    {
+      const char *sep;
+      CORE_ADDR *addrp;
+      int name_len;
+
+      sep = strchr (annex, '=');
+      if (sep == NULL)
+	break;
+
+      name_len = sep - annex;
+      if (name_len == 5 && startswith (annex, "start"))
+	addrp = &lm_addr;
+      else if (name_len == 4 && startswith (annex, "prev"))
+	addrp = &lm_prev;
+      else
+	{
+	  annex = strchr (sep, ';');
+	  if (annex == NULL)
+	    break;
+	  annex++;
+	  continue;
+	}
+
+      annex = decode_address_to_semicolon (addrp, sep + 1);
+    }
+
+  std::string document = "<library-list-svr4 version=\"1.0\"";
+
+  if (r_debug != 0)
+    {
+      /* Ignore the first entry in the default namespace.  */
+      read_link_map (document, r_debug, ptr_size, lmo, true,
+		     header_done);
+      /* Get the next namespace from the r_next field.  */
+      CORE_ADDR lm_next = get_r_next (r_debug, ptr_size, lmo);
+      while (lm_next)
+	{
+	  /* Keep the first entry in the non-default namespace.  */
+	  read_link_map (document, lm_next, ptr_size, lmo, false,
+			 header_done);
+	  /* Go to the next namespace.  */
+	  lm_next = get_r_next (lm_next, ptr_size, lmo);
+	}
+    }
 
   if (!header_done)
     {
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3] Support glibc multiple namespace extension
  2021-10-02  0:11 [PATCH v2] Support glibc multiple namespace extension H.J. Lu via Gdb-patches
@ 2021-10-02 20:43 ` H.J. Lu via Gdb-patches
  2022-02-10 23:08   ` Kevin Buettner via Gdb-patches
  0 siblings, 1 reply; 8+ messages in thread
From: H.J. Lu via Gdb-patches @ 2021-10-02 20:43 UTC (permalink / raw)
  To: GDB, Markus Metzger

Changes in v3:

1. Fix gdbserver support.

Changes in v2:

1. Don't check shared libraries in other namespaces when updating shared
libraries in a new namespace.


H.J.
---
In glibc, the r_debug structure contains (amongst others) the following
fields:

  int r_version:
    Version number for this protocol.  It should be greater than 0.

If r_version is 2, struct r_debug is extended to struct r_debug_extended
with one additional field:

  struct r_debug_extended *r_next;
    Link to the next r_debug_extended structure.  Each r_debug_extended
    structure represents a different namespace.  The first r_debug_extended
    structure is for the default namespace.

1. Change solib_svr4_r_map argument to take the debug base.
2. Add solib_svr4_r_next to find the link map in the next namespace from
the r_next field.
3. Update svr4_current_sos_direct to get the link map in the next namespace
from the r_next field.
4. Don't check shared libraries in other namespaces when updating shared
libraries in a new namespace.

This fixes PR 11839.
---
 gdb/linux-tdep.c       |   2 +
 gdb/mips-fbsd-tdep.c   |   2 +
 gdb/mips-netbsd-tdep.c |   2 +
 gdb/solib-svr4.c       |  78 +++++++++++---
 gdb/solib-svr4.h       |   3 +
 gdbserver/linux-low.cc | 224 ++++++++++++++++++++++++++---------------
 6 files changed, 214 insertions(+), 97 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index ae2f7c14f6d..a3b20070adc 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2744,6 +2744,7 @@ linux_ilp32_fetch_link_map_offsets ()
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = 20;
 
       /* Everything we need is in the first 20 bytes.  */
       lmo.link_map_size = 20;
@@ -2772,6 +2773,7 @@ linux_lp64_fetch_link_map_offsets ()
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = 40;
 
       /* Everything we need is in the first 40 bytes.  */
       lmo.link_map_size = 40;
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 0b7c97c445f..00e38a8f1c4 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -495,6 +495,7 @@ mips_fbsd_ilp32_fetch_link_map_offsets (void)
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       lmo.link_map_size = 24;
       lmo.l_addr_offset = 0;
@@ -522,6 +523,7 @@ mips_fbsd_lp64_fetch_link_map_offsets (void)
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       lmo.link_map_size = 48;
       lmo.l_addr_offset = 0;
diff --git a/gdb/mips-netbsd-tdep.c b/gdb/mips-netbsd-tdep.c
index a32ae5e3a29..12d702ef359 100644
--- a/gdb/mips-netbsd-tdep.c
+++ b/gdb/mips-netbsd-tdep.c
@@ -308,6 +308,7 @@ mipsnbsd_ilp32_fetch_link_map_offsets (void)
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 24 bytes.  */
       lmo.link_map_size = 24;
@@ -336,6 +337,7 @@ mipsnbsd_lp64_fetch_link_map_offsets (void)
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = -1;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 40 bytes.  */
       lmo.link_map_size = 48;
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 3de1bb9c7f7..9d851ba8930 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -767,7 +767,7 @@ locate_base (struct svr4_info *info)
    RT_CONSISTENT.  */
 
 static CORE_ADDR
-solib_svr4_r_map (struct svr4_info *info)
+solib_svr4_r_map (CORE_ADDR debug_base)
 {
   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
@@ -775,7 +775,7 @@ solib_svr4_r_map (struct svr4_info *info)
 
   try
     {
-      addr = read_memory_typed_address (info->debug_base + lmo->r_map_offset,
+      addr = read_memory_typed_address (debug_base + lmo->r_map_offset,
 					ptr_type);
     }
   catch (const gdb_exception_error &ex)
@@ -829,6 +829,37 @@ solib_svr4_r_ldsomap (struct svr4_info *info)
 				    ptr_type);
 }
 
+/* Find the next namespace from the r_next field.  */
+
+static CORE_ADDR
+solib_svr4_r_next (CORE_ADDR debug_base)
+{
+  struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
+  struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
+  enum bfd_endian byte_order = type_byte_order (ptr_type);
+  ULONGEST version = 0;
+
+  try
+    {
+      /* Check version, and return zero if `struct r_debug' doesn't have
+	 the r_next member.  */
+      version
+	= read_memory_unsigned_integer (debug_base + lmo->r_version_offset,
+					lmo->r_version_size, byte_order);
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      exception_print (gdb_stderr, ex);
+    }
+
+  /* The r_next field is added with r_version == 2.  */
+  if (version < 2 || lmo->r_next_offset == -1)
+    return 0;
+
+  return read_memory_typed_address (debug_base + lmo->r_next_offset,
+				    ptr_type);
+}
+
 /* On Solaris systems with some versions of the dynamic linker,
    ld.so's l_name pointer points to the SONAME in the string table
    rather than into writable memory.  So that GDB can find shared
@@ -886,7 +917,7 @@ open_symbol_file_object (int from_tty)
     return 0;	/* failed somehow...  */
 
   /* First link map member should be the executable.  */
-  lm = solib_svr4_r_map (info);
+  lm = solib_svr4_r_map (info->debug_base);
   if (lm == 0)
     return 0;	/* failed somehow...  */
 
@@ -1323,7 +1354,7 @@ svr4_current_sos_direct (struct svr4_info *info)
 
   /* Walk the inferior's link map list, and build our list of
      `struct so_list' nodes.  */
-  lm = solib_svr4_r_map (info);
+  lm = solib_svr4_r_map (info->debug_base);
   if (lm)
     svr4_read_so_list (info, lm, 0, &link_ptr, ignore_first);
 
@@ -1335,6 +1366,18 @@ svr4_current_sos_direct (struct svr4_info *info)
   if (lm)
     svr4_read_so_list (info, lm, 0, &link_ptr, 0);
 
+  /* Get the next namespace from the r_next field.  */
+  lm = solib_svr4_r_next (info->debug_base);
+  while (lm)
+    {
+      /* Get the link map in this namespace.  */
+      CORE_ADDR link_map = solib_svr4_r_map (lm);
+      if (link_map)
+	svr4_read_so_list (info, link_map, 0, &link_ptr, 0);
+      /* Go to the next namespace.  */
+      lm = solib_svr4_r_next (lm);
+    }
+
   cleanup.release ();
 
   if (head == NULL)
@@ -1706,7 +1749,8 @@ solist_update_full (struct svr4_info *info)
    failure.  */
 
 static int
-solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
+solist_update_incremental (struct svr4_info *info, CORE_ADDR debug_base,
+			   CORE_ADDR lm)
 {
   struct so_list *tail;
   CORE_ADDR prev_lm;
@@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
   for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
     /* Nothing.  */;
 
-  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
-  prev_lm = li->lm_addr;
+  /* Don't check shared libraries in other namespaces when updating
+     shared libraries in a new namespace.  */
+  if (debug_base == info->debug_base)
+    {
+      lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
+      prev_lm = li->lm_addr;
+    }
+  else
+    prev_lm = 0;
 
   /* Read the new objects.  */
   if (info->using_xfer)
@@ -1869,13 +1920,6 @@ svr4_handle_solib_event (void)
 	  return;
       }
 
-    /* GDB does not currently support libraries loaded via dlmopen
-       into namespaces other than the initial one.  We must ignore
-       any namespace other than the initial namespace here until
-       support for this is added to GDB.  */
-    if (debug_base != info->debug_base)
-      action = DO_NOTHING;
-
     if (action == UPDATE_OR_RELOAD)
       {
 	try
@@ -1901,7 +1945,7 @@ svr4_handle_solib_event (void)
 
   if (action == UPDATE_OR_RELOAD)
     {
-      if (!solist_update_incremental (info, lm))
+      if (!solist_update_incremental (info, debug_base, lm))
 	action = FULL_RELOAD;
     }
 
@@ -2136,7 +2180,7 @@ enable_break (struct svr4_info *info, int from_tty)
 
   solib_add (NULL, from_tty, auto_solib_add);
   sym_addr = 0;
-  if (info->debug_base && solib_svr4_r_map (info) != 0)
+  if (info->debug_base && solib_svr4_r_map (info->debug_base) != 0)
     sym_addr = solib_svr4_r_brk (info);
 
   if (sym_addr != 0)
@@ -3087,6 +3131,7 @@ svr4_ilp32_fetch_link_map_offsets (void)
       lmo.r_map_offset = 4;
       lmo.r_brk_offset = 8;
       lmo.r_ldsomap_offset = 20;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 20 bytes.  */
       lmo.link_map_size = 20;
@@ -3118,6 +3163,7 @@ svr4_lp64_fetch_link_map_offsets (void)
       lmo.r_map_offset = 8;
       lmo.r_brk_offset = 16;
       lmo.r_ldsomap_offset = 40;
+      lmo.r_next_offset = -1;
 
       /* Everything we need is in the first 40 bytes.  */
       lmo.link_map_size = 40;
diff --git a/gdb/solib-svr4.h b/gdb/solib-svr4.h
index 8d94d9cb26e..64854e2edd9 100644
--- a/gdb/solib-svr4.h
+++ b/gdb/solib-svr4.h
@@ -66,6 +66,9 @@ struct link_map_offsets
     /* Offset of r_debug.r_ldsomap.  */
     int r_ldsomap_offset;
 
+    /* Offset of r_debug_extended.r_next.  */
+    int r_next_offset;
+
     /* Size of struct link_map (or equivalent), or at least enough of it
        to be able to obtain the fields below.  */
     int link_map_size;
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d27a2169029..f1e3920669a 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -6733,6 +6733,9 @@ struct link_map_offsets
     /* Offset and size of r_debug.r_map.  */
     int r_map_offset;
 
+    /* Offset of r_debug_extended.r_next.  */
+    int r_next_offset;
+
     /* Offset to l_addr field in struct link_map.  */
     int l_addr_offset;
 
@@ -6749,6 +6752,120 @@ struct link_map_offsets
     int l_prev_offset;
   };
 
+static const struct link_map_offsets lmo_32bit_offsets =
+  {
+    0,     /* r_version offset. */
+    4,     /* r_debug.r_map offset.  */
+    20,    /* r_debug_extended.r_next.  */
+    0,     /* l_addr offset in link_map.  */
+    4,     /* l_name offset in link_map.  */
+    8,     /* l_ld offset in link_map.  */
+    12,    /* l_next offset in link_map.  */
+    16     /* l_prev offset in link_map.  */
+  };
+
+static const struct link_map_offsets lmo_64bit_offsets =
+  {
+    0,     /* r_version offset. */
+    8,     /* r_debug.r_map offset.  */
+    40,    /* r_debug_extended.r_next.  */
+    0,     /* l_addr offset in link_map.  */
+    8,     /* l_name offset in link_map.  */
+    16,    /* l_ld offset in link_map.  */
+    24,    /* l_next offset in link_map.  */
+    32     /* l_prev offset in link_map.  */
+  };
+
+/* Get the loaded shared libraries from one namespace.  */
+
+static void
+read_link_map (std::string &document, CORE_ADDR lm_addr, int ptr_size,
+	       const struct link_map_offsets *lmo, bool ignore_first,
+	       int &header_done)
+{
+  CORE_ADDR lm_prev = 0;
+  CORE_ADDR l_name, l_addr, l_ld, l_next, l_prev;
+
+  while (lm_addr
+	 && read_one_ptr (lm_addr + lmo->l_name_offset,
+			  &l_name, ptr_size) == 0
+	 && read_one_ptr (lm_addr + lmo->l_addr_offset,
+			  &l_addr, ptr_size) == 0
+	 && read_one_ptr (lm_addr + lmo->l_ld_offset,
+			  &l_ld, ptr_size) == 0
+	 && read_one_ptr (lm_addr + lmo->l_prev_offset,
+			  &l_prev, ptr_size) == 0
+	 && read_one_ptr (lm_addr + lmo->l_next_offset,
+			  &l_next, ptr_size) == 0)
+    {
+      unsigned char libname[PATH_MAX];
+
+      if (lm_prev != l_prev)
+	{
+	  warning ("Corrupted shared library list: 0x%lx != 0x%lx",
+		   (long) lm_prev, (long) l_prev);
+	  break;
+	}
+
+      /* Ignore the first entry even if it has valid name as the first entry
+	 corresponds to the main executable.  The first entry should not be
+	 skipped if the dynamic loader was loaded late by a static executable
+	 (see solib-svr4.c parameter ignore_first).  But in such case the main
+	 executable does not have PT_DYNAMIC present and this function already
+	 exited above due to failed get_r_debug.  */
+      if (ignore_first && lm_prev == 0)
+	string_appendf (document, " main-lm=\"0x%lx\"", (unsigned long) lm_addr);
+      else
+	{
+	  /* Not checking for error because reading may stop before
+	     we've got PATH_MAX worth of characters.  */
+	  libname[0] = '\0';
+	  linux_read_memory (l_name, libname, sizeof (libname) - 1);
+	  libname[sizeof (libname) - 1] = '\0';
+	  if (libname[0] != '\0')
+	    {
+	      if (!header_done)
+		{
+		  /* Terminate `<library-list-svr4'.  */
+		  document += '>';
+		  header_done = 1;
+		}
+
+	      string_appendf (document, "<library name=\"");
+	      xml_escape_text_append (&document, (char *) libname);
+	      string_appendf (document, "\" lm=\"0x%lx\" "
+			      "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
+			      (unsigned long) lm_addr, (unsigned long) l_addr,
+			      (unsigned long) l_ld);
+	    }
+	}
+
+      lm_prev = lm_addr;
+      lm_addr = l_next;
+    }
+}
+
+/* Find the next namespace from the r_next field.  */
+
+static CORE_ADDR
+get_r_next (CORE_ADDR r_debug, int ptr_size,
+	    const struct link_map_offsets *lmo)
+{
+  int r_version = 0;
+  CORE_ADDR lm_addr = 0;
+  if (linux_read_memory (r_debug + lmo->r_version_offset,
+			 (unsigned char *) &r_version,
+			 sizeof (r_version)) == 0
+      && r_version >= 2)
+    {
+      if (read_one_ptr (r_debug + lmo->r_next_offset,
+			&lm_addr, ptr_size) != 0)
+	warning ("unable to read r_next from 0x%lx",
+		 (long) r_debug + lmo->r_next_offset);
+    }
+  return lm_addr;
+}
+
 /* Construct qXfer:libraries-svr4:read reply.  */
 
 int
@@ -6760,33 +6877,8 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
   struct process_info_private *const priv = current_process ()->priv;
   char filename[PATH_MAX];
   int pid, is_elf64;
-
-  static const struct link_map_offsets lmo_32bit_offsets =
-    {
-      0,     /* r_version offset. */
-      4,     /* r_debug.r_map offset.  */
-      0,     /* l_addr offset in link_map.  */
-      4,     /* l_name offset in link_map.  */
-      8,     /* l_ld offset in link_map.  */
-      12,    /* l_next offset in link_map.  */
-      16     /* l_prev offset in link_map.  */
-    };
-
-  static const struct link_map_offsets lmo_64bit_offsets =
-    {
-      0,     /* r_version offset. */
-      8,     /* r_debug.r_map offset.  */
-      0,     /* l_addr offset in link_map.  */
-      8,     /* l_name offset in link_map.  */
-      16,    /* l_ld offset in link_map.  */
-      24,    /* l_next offset in link_map.  */
-      32     /* l_prev offset in link_map.  */
-    };
-  const struct link_map_offsets *lmo;
   unsigned int machine;
-  int ptr_size;
   CORE_ADDR lm_addr = 0, lm_prev = 0;
-  CORE_ADDR l_name, l_addr, l_ld, l_next, l_prev;
   int header_done = 0;
 
   if (writebuf != NULL)
@@ -6797,8 +6889,18 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
   pid = lwpid_of (current_thread);
   xsnprintf (filename, sizeof filename, "/proc/%d/exe", pid);
   is_elf64 = elf_64_file_p (filename, &machine);
-  lmo = is_elf64 ? &lmo_64bit_offsets : &lmo_32bit_offsets;
-  ptr_size = is_elf64 ? 8 : 4;
+  const struct link_map_offsets *lmo;
+  int ptr_size;
+  if (is_elf64)
+    {
+      lmo = &lmo_64bit_offsets;
+      ptr_size = 8;
+    }
+  else
+    {
+      lmo = &lmo_32bit_offsets;
+      ptr_size = 4;
+    }
 
   while (annex[0] != '\0')
     {
@@ -6827,7 +6929,9 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
       annex = decode_address_to_semicolon (addrp, sep + 1);
     }
 
-  if (lm_addr == 0)
+  /* Ignore the first entry in the default namespace.  */
+  bool ignore_first = lm_addr == 0;
+  if (ignore_first)
     {
       int r_version = 0;
 
@@ -6860,62 +6964,20 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
 
   std::string document = "<library-list-svr4 version=\"1.0\"";
 
-  while (lm_addr
-	 && read_one_ptr (lm_addr + lmo->l_name_offset,
-			  &l_name, ptr_size) == 0
-	 && read_one_ptr (lm_addr + lmo->l_addr_offset,
-			  &l_addr, ptr_size) == 0
-	 && read_one_ptr (lm_addr + lmo->l_ld_offset,
-			  &l_ld, ptr_size) == 0
-	 && read_one_ptr (lm_addr + lmo->l_prev_offset,
-			  &l_prev, ptr_size) == 0
-	 && read_one_ptr (lm_addr + lmo->l_next_offset,
-			  &l_next, ptr_size) == 0)
+  if (lm_addr != 0)
     {
-      unsigned char libname[PATH_MAX];
-
-      if (lm_prev != l_prev)
-	{
-	  warning ("Corrupted shared library list: 0x%lx != 0x%lx",
-		   (long) lm_prev, (long) l_prev);
-	  break;
-	}
-
-      /* Ignore the first entry even if it has valid name as the first entry
-	 corresponds to the main executable.  The first entry should not be
-	 skipped if the dynamic loader was loaded late by a static executable
-	 (see solib-svr4.c parameter ignore_first).  But in such case the main
-	 executable does not have PT_DYNAMIC present and this function already
-	 exited above due to failed get_r_debug.  */
-      if (lm_prev == 0)
-	string_appendf (document, " main-lm=\"0x%lx\"", (unsigned long) lm_addr);
-      else
+      read_link_map (document, lm_addr, ptr_size, lmo, ignore_first,
+		     header_done);
+      /* Get the next namespace from the r_next field.  */
+      CORE_ADDR lm_next = get_r_next (lm_addr, ptr_size, lmo);
+      while (lm_next)
 	{
-	  /* Not checking for error because reading may stop before
-	     we've got PATH_MAX worth of characters.  */
-	  libname[0] = '\0';
-	  linux_read_memory (l_name, libname, sizeof (libname) - 1);
-	  libname[sizeof (libname) - 1] = '\0';
-	  if (libname[0] != '\0')
-	    {
-	      if (!header_done)
-		{
-		  /* Terminate `<library-list-svr4'.  */
-		  document += '>';
-		  header_done = 1;
-		}
-
-	      string_appendf (document, "<library name=\"");
-	      xml_escape_text_append (&document, (char *) libname);
-	      string_appendf (document, "\" lm=\"0x%lx\" "
-			      "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
-			      (unsigned long) lm_addr, (unsigned long) l_addr,
-			      (unsigned long) l_ld);
-	    }
+	  /* Keep the first entry in the non-default namespace.  */
+	  read_link_map (document, lm_next, ptr_size, lmo, false,
+			 header_done);
+	  /* Go to the next namespace.  */
+	  lm_next = get_r_next (lm_next, ptr_size, lmo);
 	}
-
-      lm_prev = lm_addr;
-      lm_addr = l_next;
     }
 
   if (!header_done)
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Support glibc multiple namespace extension
  2021-10-02 20:43 ` [PATCH v3] " H.J. Lu via Gdb-patches
@ 2022-02-10 23:08   ` Kevin Buettner via Gdb-patches
  2022-02-16  0:43     ` H.J. Lu via Gdb-patches
  2022-02-16 11:10     ` Metzger, Markus T via Gdb-patches
  0 siblings, 2 replies; 8+ messages in thread
From: Kevin Buettner via Gdb-patches @ 2022-02-10 23:08 UTC (permalink / raw)
  To: H.J. Lu via Gdb-patches

Hi H.J.,

This work looks pretty good to me.  I found a couple of coding style nits
and have a question/request; see below.

On Sat, 2 Oct 2021 13:43:40 -0700
"H.J. Lu via Gdb-patches" <gdb-patches@sourceware.org> wrote:

[...]
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 3de1bb9c7f7..9d851ba8930 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
[...]
> @@ -1335,6 +1366,18 @@ svr4_current_sos_direct (struct svr4_info *info)
>    if (lm)
>      svr4_read_so_list (info, lm, 0, &link_ptr, 0);
>  
> +  /* Get the next namespace from the r_next field.  */
> +  lm = solib_svr4_r_next (info->debug_base);
> +  while (lm)

Due to the GDB coding standard, this test needs to be turned into an
explicit comparison against 0.  See:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_nullptr_And_Zero

(FWIW, I don't really like this part of our standard; I personally
find it more readable to code it the way that you did.  Also, I do
realize that there are existing implicit tests against 0 in this file
and elsewhere and that they do not conform to GDB's current coding
standard.  I'm in no hurry to fix them - but, obviously, I won't
object if someone does.)

> +    {
> +      /* Get the link map in this namespace.  */
> +      CORE_ADDR link_map = solib_svr4_r_map (lm);
> +      if (link_map)

Likewise, for the check above.

> +	svr4_read_so_list (info, link_map, 0, &link_ptr, 0);
> +      /* Go to the next namespace.  */
> +      lm = solib_svr4_r_next (lm);
> +    }
> +
>    cleanup.release ();
>  
>    if (head == NULL)
> @@ -1706,7 +1749,8 @@ solist_update_full (struct svr4_info *info)
>     failure.  */
>  
>  static int
> -solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
> +solist_update_incremental (struct svr4_info *info, CORE_ADDR debug_base,
> +			   CORE_ADDR lm)
>  {
>    struct so_list *tail;
>    CORE_ADDR prev_lm;
> @@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
>    for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
>      /* Nothing.  */;
>  
> -  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
> -  prev_lm = li->lm_addr;
> +  /* Don't check shared libraries in other namespaces when updating
> +     shared libraries in a new namespace.  */

Shared libraries in other namespaces aren't being neglected though,
right?

Assuming that's true, could you add a sentence or two to your comment
addressing this concern?

Kevin


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Support glibc multiple namespace extension
  2022-02-10 23:08   ` Kevin Buettner via Gdb-patches
@ 2022-02-16  0:43     ` H.J. Lu via Gdb-patches
  2022-02-16 11:10     ` Metzger, Markus T via Gdb-patches
  1 sibling, 0 replies; 8+ messages in thread
From: H.J. Lu via Gdb-patches @ 2022-02-16  0:43 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: H.J. Lu via Gdb-patches

On Thu, Feb 10, 2022 at 3:08 PM Kevin Buettner <kevinb@redhat.com> wrote:
>
> Hi H.J.,
>
> This work looks pretty good to me.  I found a couple of coding style nits
> and have a question/request; see below.
>
> On Sat, 2 Oct 2021 13:43:40 -0700
> "H.J. Lu via Gdb-patches" <gdb-patches@sourceware.org> wrote:
>
> [...]
> > diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> > index 3de1bb9c7f7..9d851ba8930 100644
> > --- a/gdb/solib-svr4.c
> > +++ b/gdb/solib-svr4.c
> [...]
> > @@ -1335,6 +1366,18 @@ svr4_current_sos_direct (struct svr4_info *info)
> >    if (lm)
> >      svr4_read_so_list (info, lm, 0, &link_ptr, 0);
> >
> > +  /* Get the next namespace from the r_next field.  */
> > +  lm = solib_svr4_r_next (info->debug_base);
> > +  while (lm)
>
> Due to the GDB coding standard, this test needs to be turned into an
> explicit comparison against 0.  See:
>
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_nullptr_And_Zero
>
> (FWIW, I don't really like this part of our standard; I personally
> find it more readable to code it the way that you did.  Also, I do
> realize that there are existing implicit tests against 0 in this file
> and elsewhere and that they do not conform to GDB's current coding
> standard.  I'm in no hurry to fix them - but, obviously, I won't
> object if someone does.)
>
> > +    {
> > +      /* Get the link map in this namespace.  */
> > +      CORE_ADDR link_map = solib_svr4_r_map (lm);
> > +      if (link_map)
>
> Likewise, for the check above.
>
> > +     svr4_read_so_list (info, link_map, 0, &link_ptr, 0);
> > +      /* Go to the next namespace.  */
> > +      lm = solib_svr4_r_next (lm);
> > +    }
> > +
> >    cleanup.release ();
> >
> >    if (head == NULL)
> > @@ -1706,7 +1749,8 @@ solist_update_full (struct svr4_info *info)
> >     failure.  */
> >
> >  static int
> > -solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
> > +solist_update_incremental (struct svr4_info *info, CORE_ADDR debug_base,
> > +                        CORE_ADDR lm)
> >  {
> >    struct so_list *tail;
> >    CORE_ADDR prev_lm;
> > @@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
> >    for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
> >      /* Nothing.  */;
> >
> > -  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
> > -  prev_lm = li->lm_addr;
> > +  /* Don't check shared libraries in other namespaces when updating
> > +     shared libraries in a new namespace.  */
>
> Shared libraries in other namespaces aren't being neglected though,
> right?
>
> Assuming that's true, could you add a sentence or two to your comment
> addressing this concern?
>
> Kevin
>

Hi Kevin,

Markus has a much better patch to support multiple namespaces.

-- 
H.J.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v3] Support glibc multiple namespace extension
  2022-02-10 23:08   ` Kevin Buettner via Gdb-patches
  2022-02-16  0:43     ` H.J. Lu via Gdb-patches
@ 2022-02-16 11:10     ` Metzger, Markus T via Gdb-patches
  2022-02-21  6:53       ` Metzger, Markus T via Gdb-patches
  1 sibling, 1 reply; 8+ messages in thread
From: Metzger, Markus T via Gdb-patches @ 2022-02-16 11:10 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: H.J. Lu via Gdb-patches

Hello Kevin,

There's a newer version of this patch
https://sourceware.org/pipermail/gdb-patches/2021-November/183498.html
that already addresses the style concerns you raise.

>> @@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info,
>CORE_ADDR lm)
>>    for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
>>      /* Nothing.  */;
>>
>> -  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
>> -  prev_lm = li->lm_addr;
>> +  /* Don't check shared libraries in other namespaces when updating
>> +     shared libraries in a new namespace.  */
>
>Shared libraries in other namespaces aren't being neglected though,
>right?

They're not, but this code ...

  /* Walk to the end of the list.  */
  for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
    /* Nothing.  */;

  /* Don't check shared libraries in other namespaces when updating
     shared libraries in a new namespace.  */
  if (debug_base == info->debug_base)
    {
      lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
      prev_lm = li->lm_addr;
    }
  else
    prev_lm = 0;

... looks a bit odd to me.  We're adding to the same solib_list, yet we use
different prev_lm.  Let me look into this some more.

Regards,
Markus.


>-----Original Message-----
>From: Kevin Buettner <kevinb@redhat.com>
>Sent: Friday, February 11, 2022 12:08 AM
>To: H.J. Lu via Gdb-patches <gdb-patches@sourceware.org>
>Cc: H.J. Lu <hjl.tools@gmail.com>; Metzger, Markus T
><markus.t.metzger@intel.com>
>Subject: Re: [PATCH v3] Support glibc multiple namespace extension
>
>Hi H.J.,
>
>This work looks pretty good to me.  I found a couple of coding style nits
>and have a question/request; see below.
>
>On Sat, 2 Oct 2021 13:43:40 -0700
>"H.J. Lu via Gdb-patches" <gdb-patches@sourceware.org> wrote:
>
>[...]
>> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
>> index 3de1bb9c7f7..9d851ba8930 100644
>> --- a/gdb/solib-svr4.c
>> +++ b/gdb/solib-svr4.c
>[...]
>> @@ -1335,6 +1366,18 @@ svr4_current_sos_direct (struct svr4_info *info)
>>    if (lm)
>>      svr4_read_so_list (info, lm, 0, &link_ptr, 0);
>>
>> +  /* Get the next namespace from the r_next field.  */
>> +  lm = solib_svr4_r_next (info->debug_base);
>> +  while (lm)
>
>Due to the GDB coding standard, this test needs to be turned into an
>explicit comparison against 0.  See:
>
>https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-
>Standards#Comparison_With_nullptr_And_Zero
>
>(FWIW, I don't really like this part of our standard; I personally
>find it more readable to code it the way that you did.  Also, I do
>realize that there are existing implicit tests against 0 in this file
>and elsewhere and that they do not conform to GDB's current coding
>standard.  I'm in no hurry to fix them - but, obviously, I won't
>object if someone does.)
>
>> +    {
>> +      /* Get the link map in this namespace.  */
>> +      CORE_ADDR link_map = solib_svr4_r_map (lm);
>> +      if (link_map)
>
>Likewise, for the check above.
>
>> +	svr4_read_so_list (info, link_map, 0, &link_ptr, 0);
>> +      /* Go to the next namespace.  */
>> +      lm = solib_svr4_r_next (lm);
>> +    }
>> +
>>    cleanup.release ();
>>
>>    if (head == NULL)
>> @@ -1706,7 +1749,8 @@ solist_update_full (struct svr4_info *info)
>>     failure.  */
>>
>>  static int
>> -solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
>> +solist_update_incremental (struct svr4_info *info, CORE_ADDR debug_base,
>> +			   CORE_ADDR lm)
>>  {
>>    struct so_list *tail;
>>    CORE_ADDR prev_lm;
>> @@ -1727,8 +1771,15 @@ solist_update_incremental (struct svr4_info *info,
>CORE_ADDR lm)
>>    for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
>>      /* Nothing.  */;
>>
>> -  lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
>> -  prev_lm = li->lm_addr;
>> +  /* Don't check shared libraries in other namespaces when updating
>> +     shared libraries in a new namespace.  */
>
>Shared libraries in other namespaces aren't being neglected though,
>right?
>
>Assuming that's true, could you add a sentence or two to your comment
>addressing this concern?
>
>Kevin

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v3] Support glibc multiple namespace extension
  2022-02-16 11:10     ` Metzger, Markus T via Gdb-patches
@ 2022-02-21  6:53       ` Metzger, Markus T via Gdb-patches
  2022-02-25 16:53         ` Metzger, Markus T via Gdb-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Metzger, Markus T via Gdb-patches @ 2022-02-21  6:53 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: gdb-patches

Hello Kevin,

>They're not, but this code ...
>
>  /* Walk to the end of the list.  */
>  for (tail = info->solib_list; tail->next != NULL; tail = tail->next)
>    /* Nothing.  */;
>
>  /* Don't check shared libraries in other namespaces when updating
>     shared libraries in a new namespace.  */
>  if (debug_base == info->debug_base)
>    {
>      lm_info_svr4 *li = (lm_info_svr4 *) tail->lm_info;
>      prev_lm = li->lm_addr;
>    }
>  else
>    prev_lm = 0;
>
>... looks a bit odd to me.  We're adding to the same solib_list, yet we use
>different prev_lm.  Let me look into this some more.

This is indeed wrong and leads to

      if (li->l_prev != prev_lm)
	{
	  warning (_("Corrupted shared library list: %s != %s"),
		   paddress (target_gdbarch (), prev_lm),
		   paddress (target_gdbarch (), li->l_prev));
	  return 0;
	}

in svr4_read_so_list().  Assigning prev_lm to zero avoided the warning for
the first library in another namespace.  I added more test cases to detect
this reliably.

I'll try organizing SVr4 DSOs in per-namespace lists.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v3] Support glibc multiple namespace extension
  2022-02-21  6:53       ` Metzger, Markus T via Gdb-patches
@ 2022-02-25 16:53         ` Metzger, Markus T via Gdb-patches
  2022-02-25 20:25           ` Kevin Buettner via Gdb-patches
  0 siblings, 1 reply; 8+ messages in thread
From: Metzger, Markus T via Gdb-patches @ 2022-02-25 16:53 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: gdb-patches

Hello Kevin,

>This is indeed wrong and leads to
>
>      if (li->l_prev != prev_lm)
>	{
>	  warning (_("Corrupted shared library list: %s != %s"),
>		   paddress (target_gdbarch (), prev_lm),
>		   paddress (target_gdbarch (), li->l_prev));
>	  return 0;
>	}
>
>in svr4_read_so_list().  Assigning prev_lm to zero avoided the warning for
>the first library in another namespace.  I added more test cases to detect
>this reliably.
>
>I'll try organizing SVr4 DSOs in per-namespace lists.

That works.  During testing I noticed

	Invalid cast.
	warning: Probes-based dynamic linker interface failed.
	Reverting to original interface.

on dlcose().  This can also be reproduced with gdb.base/unload.exp
and upstream GDB.  I have not looked into this, yet.

This is not detected by the test system and I'm not sure whether we
actually want to consider falling back to the old interface a test fail.
The invalid cast error we probably want to detect.

I added support for detecting this invalid cast and the corrupted
library list warning in gdb_continue_to_breakpoint.

I still need to extend the library-list-svr4 XML to cover namespaces
before I send an updated patch.  I'll be out the next week so this
will take a while.

I currently check the XML version and put everything into a special
namespace on the GDB side.  We cannot use incremental updates
this way but we're backwards compatible.  The final version will
support both the new format with namespaces and incremental
updates and the old format without.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] Support glibc multiple namespace extension
  2022-02-25 16:53         ` Metzger, Markus T via Gdb-patches
@ 2022-02-25 20:25           ` Kevin Buettner via Gdb-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Kevin Buettner via Gdb-patches @ 2022-02-25 20:25 UTC (permalink / raw)
  To: Metzger, Markus T; +Cc: gdb-patches

On Fri, 25 Feb 2022 16:53:38 +0000
"Metzger, Markus T" <markus.t.metzger@intel.com> wrote:

> Hello Kevin,
> 
> >This is indeed wrong and leads to
> >
> >      if (li->l_prev != prev_lm)
> >	{
> >	  warning (_("Corrupted shared library list: %s != %s"),
> >		   paddress (target_gdbarch (), prev_lm),
> >		   paddress (target_gdbarch (), li->l_prev));
> >	  return 0;
> >	}
> >
> >in svr4_read_so_list().  Assigning prev_lm to zero avoided the warning for
> >the first library in another namespace.  I added more test cases to detect
> >this reliably.
> >
> >I'll try organizing SVr4 DSOs in per-namespace lists.  
> 
> That works.  During testing I noticed
> 
> 	Invalid cast.
> 	warning: Probes-based dynamic linker interface failed.
> 	Reverting to original interface.
> 
> on dlcose().  This can also be reproduced with gdb.base/unload.exp
> and upstream GDB.  I have not looked into this, yet.
> 
> This is not detected by the test system and I'm not sure whether we
> actually want to consider falling back to the old interface a test fail.
> The invalid cast error we probably want to detect.
> 
> I added support for detecting this invalid cast and the corrupted
> library list warning in gdb_continue_to_breakpoint.
> 
> I still need to extend the library-list-svr4 XML to cover namespaces
> before I send an updated patch.  I'll be out the next week so this
> will take a while.
> 
> I currently check the XML version and put everything into a special
> namespace on the GDB side.  We cannot use incremental updates
> this way but we're backwards compatible.  The final version will
> support both the new format with namespaces and incremental
> updates and the old format without.

This sounds like good progress!

I look forward to seeing your next patch...

Kevin


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-02-25 20:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-02  0:11 [PATCH v2] Support glibc multiple namespace extension H.J. Lu via Gdb-patches
2021-10-02 20:43 ` [PATCH v3] " H.J. Lu via Gdb-patches
2022-02-10 23:08   ` Kevin Buettner via Gdb-patches
2022-02-16  0:43     ` H.J. Lu via Gdb-patches
2022-02-16 11:10     ` Metzger, Markus T via Gdb-patches
2022-02-21  6:53       ` Metzger, Markus T via Gdb-patches
2022-02-25 16:53         ` Metzger, Markus T via Gdb-patches
2022-02-25 20:25           ` Kevin Buettner via Gdb-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox