gdb/solib-svr4: fix wrong namespace id for dynamic linker

When running a program that uses multiple linker namespaces, I get
something like:

    $ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.base/dlmopen-ns-ids/dlmopen-ns-ids -ex "tb 50" -ex r -ex "info shared" -batch
    ...
    From                To                  NS Syms Read   Shared Object Library
    0x00007ffff7fc6000  0x00007ffff7fff000  0  Yes         /lib64/ld-linux-x86-64.so.2
    0x00007ffff7e93000  0x00007ffff7f8b000  0  Yes         /usr/lib/libm.so.6
    0x00007ffff7ca3000  0x00007ffff7e93000  0  Yes         /usr/lib/libc.so.6
    0x00007ffff7fb7000  0x00007ffff7fbc000  1  Yes         /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/dlmopen-ns-ids/dlmopen-lib.so
    0x00007ffff7b77000  0x00007ffff7c6f000  1  Yes         /usr/lib/libm.so.6
    0x00007ffff7987000  0x00007ffff7b77000  1  Yes         /usr/lib/libc.so.6
    0x00007ffff7fc6000  0x00007ffff7fff000  1  Yes         /usr/lib/ld-linux-x86-64.so.2
    0x00007ffff7fb2000  0x00007ffff7fb7000  2  Yes         /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/dlmopen-ns-ids/dlmopen-lib.so
    0x00007ffff788f000  0x00007ffff7987000  2  Yes         /usr/lib/libm.so.6
    0x00007ffff769f000  0x00007ffff788f000  2  Yes         /usr/lib/libc.so.6
    0x00007ffff7fc6000  0x00007ffff7fff000  1! Yes         /usr/lib/ld-linux-x86-64.so.2
    0x00007ffff7fad000  0x00007ffff7fb2000  3  Yes         /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/dlmopen-ns-ids/dlmopen-lib.so
    0x00007ffff75a7000  0x00007ffff769f000  3  Yes         /usr/lib/libm.so.6
    0x00007ffff73b7000  0x00007ffff75a7000  3  Yes         /usr/lib/libc.so.6
    0x00007ffff7fc6000  0x00007ffff7fff000  1! Yes         /usr/lib/ld-linux-x86-64.so.2

Some namespace IDs for the dynamic linker entries (ld-linux) are wrong
(I placed a ! next to those that are wrong).

The dynamic linker is special: it is loaded only once (notice how all
ld-linux entries have the same addresses), but it is visible in all
namespaces.  It is therefore listed separately in all namespaces.

The problem happens like this:

 - for each solib, print_solib_list_table calls solib_ops::find_solib_ns
   to get the namespace ID to print
 - svr4_solib_ops::find_solib_ns calls find_debug_base_for_solib
 - find_debug_base_for_solib iterates on the list of solibs in all
   namespaces, looking for a match for the given solib.  For this, it
   uses svr4_same, which compares two SOs by name and low address.
   Because there are entries for the dynamic linker in all namespaces,
   with the same low address, find_debug_base_for_solib is unable to
   distinguish them, and sometimes returns the wrong namespace.

To fix this, save in lm_info_svr4 the debug base address that this
lm/solib comes from, as a way to distinguish two solibs that would be
otherwise identical.

The code changes are:

 - Add a constructor to lm_info_svr4 accepting the debug base.  Update
   all callers, which sometimes requires passing down the debug base.
 - Modify find_debug_base_for_solib to return the debug base directly
   from lm_info_svr4.
 - Modify svr4_same to consider the debug base value of the two
   libraries before saying they are the same.  While at it, move the
   address checks before the name check, since they are likely less
   expensive to do.
 - Modify svr4_solib_ops::default_debug_base to update the debug base of
   existing solibs when the default debug base becomes known.

I found the last point to be necessary, because when running an
inferior, we list the shared libraries very early (before the first
instruction):

    #0  svr4_solib_ops::current_sos (this=0x7c1ff1e09710)
    #1  0x00005555643c774e in update_solib_list (from_tty=0)
    #2  0x00005555643ca377 in solib_add (pattern=0x0, from_tty=0, readsyms=1)
    #3  0x0000555564335585 in svr4_solib_ops::enable_break (this=0x7c1ff1e09710, info=0x7d2ff1de8c40, from_tty=0)
    #4  0x000055556433c85c in svr4_solib_ops::create_inferior_hook (this=0x7c1ff1e09710, from_tty=0)
    #5  0x00005555643d22cb in solib_create_inferior_hook (from_tty=0)
    #6  0x000055556337071b in post_create_inferior (from_tty=0, set_pspace_solib_ops=true)
    #7  0x00005555633726a2 in run_command_1 (args=0x0, from_tty=0, run_how=RUN_NORMAL)
    #8  0x0000555563372b35 in run_command (args=0x0, from_tty=0)

At this point, the dynamic linker hasn't yet filled the DT_DEBUG slot,
which normally points at the base of r_debug.  Since we're unable to
list shared libraries at this point, we go through
svr4_solib_ops::default_sos, which creates an solib entry for the
dynamic linker.  At this point, we have no choice but to create it with
a debug base of 0 (or some other value that indicates "unknown").  If we
left it as-is, then it would later not be recognized to be part of any
existing namespace and that would cause problems down the line.

With this change, the namespaces of the dynamic linker become correct.

I was not sure if the code in library_list_start_library was conflating
debug base and lmid.  The documentation says this about the "lmid" field
in the response of a qxfer:libraries-svr4:read packet:

    lmid, which is an identifier for a linker namespace, such as the
    memory address of the r_debug object that contains this namespace’s
    load map or the namespace identifier returned by dlinfo (3).

When I read "lmid", I typically think about "the namespace identifier
returned by dlinfo (3)".  In library_list_start_library, we use the
value of the "lmid" attribute as the debug base address.  This is the
case even before this patch, since we do:

      solist = &list->solib_lists[lmid];

The key for the solib_lists map is documented as being the debug base
address.  In practice, GDBserver uses the debug base address for the
"lmid" field, so we're good for now.

If the remote side instead used "the namespace identifier returned by
dlinfo (3)" (which in practice with glibc are sequential integers
starting at 0), I think we would be mostly fine.  If we use the qxfer
packet to read the libraries, we normally won't use the namespace base
address to do any memory reads, as all the information comes from the
XML.  There might be some problems however because we treat the
namespace 0 specially, for instance in
svr4_solib_ops::update_incremental.  In that case, we might need a
different way of indicating that the remote side does not give namespace
information than using namespace 0.  This is just a thought for the
future.

I improved the existing test gdb.base/dlmopen-ns-ids.exp to verify that
"info sharedlibrary" does not show duplicate libraries, duplicate
meaning same address range, namespace and name.

Change-Id: I84467c6abf4e0109b1c53a86ef688b934e8eff99
Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

4 files changed, 143 insertions, 34 deletions

diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
index 4c59e8a..c39d9d7 100644
--- a/gdb/solib-rocm.c
+++ b/gdb/solib-rocm.c
@@ -796,7 +796,8 @@ rocm_update_solib_list ()
 
       gdb::unique_xmalloc_ptr<char> uri_bytes_holder (uri_bytes);
 
-      lm_info_svr4_up li = std::make_unique<lm_info_svr4> ();
+      /* Pass a dummy debug base.  */
+      lm_info_svr4_up li = std::make_unique<lm_info_svr4> (-1);
       li->l_addr = l_addr;
 
       /* Generate a unique name so that code objects with the same URI but
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index eb25087..66409e7 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -155,13 +155,19 @@ svr4_same (const char *gdb_name, const char *inferior_name,
 	   const lm_info_svr4 &gdb_lm_info,
 	   const lm_info_svr4 &inferior_lm_info)
 {
-  if (!svr4_same_name (gdb_name, inferior_name))
+  /* There may be different instances of the same library, in different
+     namespaces.  Each instance is typically loaded at a different address
+     so its relocation offset would be different.  */
+  if (gdb_lm_info.l_addr_inferior != inferior_lm_info.l_addr_inferior)
     return false;
 
-  /* There may be different instances of the same library, in different
-     namespaces.  Each instance, however, must have been loaded at a
-     different address so its relocation offset would be different.  */
-  return gdb_lm_info.l_addr_inferior == inferior_lm_info.l_addr_inferior;
+  /* There may be multiple entries for the same dynamic linker instance (at
+     the same address) visible in different namespaces.  Those are considered
+     different instances.  */
+  if (gdb_lm_info.debug_base != inferior_lm_info.debug_base)
+    return false;
+
+  return svr4_same_name (gdb_name, inferior_name);
 }
 
 bool
@@ -175,7 +181,7 @@ svr4_solib_ops::same (const solib &gdb, const solib &inferior) const
 }
 
 lm_info_svr4_up
-svr4_solib_ops::read_lm_info (CORE_ADDR lm_addr) const
+svr4_solib_ops::read_lm_info (CORE_ADDR lm_addr, CORE_ADDR debug_base) const
 {
   link_map_offsets *lmo = this->fetch_link_map_offsets ();
   lm_info_svr4_up lm_info;
@@ -190,7 +196,7 @@ svr4_solib_ops::read_lm_info (CORE_ADDR lm_addr) const
       type *ptr_type
 	= builtin_type (current_inferior ()->arch ())->builtin_data_ptr;
 
-      lm_info = std::make_unique<lm_info_svr4> ();
+      lm_info = std::make_unique<lm_info_svr4> (debug_base);
       lm_info->lm_addr = lm_addr;
 
       lm_info->l_addr_inferior = extract_typed_address (&lm[lmo->l_addr_offset],
@@ -798,10 +804,28 @@ svr4_solib_ops::default_debug_base (svr4_info *info, bool *changed) const
   CORE_ADDR default_debug_base = locate_default_debug_base ();
 
   if (changed != nullptr)
-    *changed = default_debug_base == info->default_debug_base;
+    *changed = default_debug_base != info->default_debug_base;
 
-  info->default_debug_base = default_debug_base;
-  return default_debug_base;
+  if (default_debug_base != info->default_debug_base)
+    {
+      /* Update the debug base value for existing solibs.  The only known case
+	 where this is required is when a struct solib was created for the
+	 dynamic linker itself by svr4_solib_ops::default_sos, before the
+	 default debug base was known.  */
+      for (const auto &solib : m_pspace->solibs ())
+	{
+	  if (&solib.ops () != this)
+	    continue;
+
+	  if (auto &li = get_lm_info_svr4 (solib);
+	      li.debug_base == info->default_debug_base)
+	    li.debug_base = default_debug_base;
+	}
+
+      info->default_debug_base = default_debug_base;
+    }
+
+  return info->default_debug_base;
 }
 
 /* Find the first element in the inferior's dynamic link map, and
@@ -938,7 +962,8 @@ svr4_solib_ops::keep_data_in_core (CORE_ADDR vaddr, unsigned long size) const
   if (!ldsomap)
     return false;
 
-  std::unique_ptr<lm_info_svr4> li = this->read_lm_info (ldsomap);
+  std::unique_ptr<lm_info_svr4> li
+    = this->read_lm_info (ldsomap, info->default_debug_base);
   name_lm = li != NULL ? li->l_name : 0;
 
   return (name_lm >= vaddr && name_lm < vaddr + size);
@@ -1078,28 +1103,33 @@ library_list_start_library (struct gdb_xml_parser *parser,
   ULONGEST *l_ldp
     = (ULONGEST *) xml_find_attribute (attributes, "l_ld")->value.get ();
 
-  lm_info_svr4_up li = std::make_unique<lm_info_svr4> ();
-  li->lm_addr = *lmp;
-  li->l_addr_inferior = *l_addrp;
-  li->l_ld = *l_ldp;
-
   std::vector<svr4_so> *solist;
 
   /* Older versions did not supply lmid.  Put the element into the flat
      list of the special namespace zero in that case.  */
   gdb_xml_value *at_lmid = xml_find_attribute (attributes, "lmid");
   svr4_info *info = get_svr4_info (current_program_space);
+  ULONGEST lmid;
+
   if (at_lmid == nullptr)
     {
       solist = list->cur_list;
       svr4_maybe_add_namespace (info, 0);
+      lmid = 0;
     }
   else
     {
-      ULONGEST lmid = *(ULONGEST *) at_lmid->value.get ();
+      lmid = *(ULONGEST *) at_lmid->value.get ();
       solist = &list->solib_lists[lmid];
       svr4_maybe_add_namespace (info, lmid);
     }
+
+  lm_info_svr4_up li = std::make_unique<lm_info_svr4> (lmid);
+
+  li->lm_addr = *lmp;
+  li->l_addr_inferior = *l_addrp;
+  li->l_ld = *l_ldp;
+
   solist->emplace_back (name, std::move (li));
 }
 
@@ -1238,7 +1268,7 @@ svr4_solib_ops::default_sos (svr4_info *info) const
   if (!info->debug_loader_offset_p)
     return {};
 
-  auto li = std::make_unique<lm_info_svr4> ();
+  auto li = std::make_unique<lm_info_svr4> (0);
 
   /* Nothing will ever check the other fields if we set l_addr_p.  */
   li->l_addr = li->l_addr_inferior = info->debug_loader_offset;
@@ -1263,14 +1293,15 @@ svr4_solib_ops::default_sos (svr4_info *info) const
 
 int
 svr4_solib_ops::read_so_list (svr4_info *info, CORE_ADDR lm, CORE_ADDR prev_lm,
-			      std::vector<svr4_so> &sos, int ignore_first) const
+			      CORE_ADDR debug_base, std::vector<svr4_so> &sos,
+			      int ignore_first) const
 {
   CORE_ADDR first_l_name = 0;
   CORE_ADDR next_lm;
 
   for (; lm != 0; prev_lm = lm, lm = next_lm)
     {
-      lm_info_svr4_up li = this->read_lm_info (lm);
+      lm_info_svr4_up li = this->read_lm_info (lm, debug_base);
       if (li == NULL)
 	return 0;
 
@@ -1402,8 +1433,8 @@ svr4_solib_ops::current_sos_direct (svr4_info *info) const
       if (lm != 0)
 	{
 	  svr4_maybe_add_namespace (info, debug_base);
-	  this->read_so_list (info, lm, 0, info->solib_lists[debug_base],
-			      ignore_first);
+	  this->read_so_list (info, lm, 0, debug_base,
+			      info->solib_lists[debug_base], ignore_first);
 	}
     }
 
@@ -1423,7 +1454,7 @@ svr4_solib_ops::current_sos_direct (svr4_info *info) const
       if (info->solib_lists.find (debug_base) == info->solib_lists.end ())
 	{
 	  svr4_maybe_add_namespace (info, debug_base);
-	  this->read_so_list (info, debug_base, 0,
+	  this->read_so_list (info, debug_base, 0, debug_base,
 			      info->solib_lists[debug_base], 0);
 	}
     }
@@ -2089,7 +2120,7 @@ svr4_solib_ops::update_incremental (svr4_info *info, CORE_ADDR debug_base,
 	 above check and deferral to solist_update_full ensures
 	 that this call to svr4_read_so_list will never see the
 	 first element.  */
-      if (!this->read_so_list (info, lm, prev_lm, solist, 0))
+      if (!this->read_so_list (info, lm, prev_lm, debug_base, solist, 0))
 	return 0;
     }
 
@@ -3604,13 +3635,7 @@ find_debug_base_for_solib (const solib *solib)
 
   auto &lm_info = get_lm_info_svr4 (*solib);
 
-  for (const auto &[debug_base, sos] : info->solib_lists)
-    for (const svr4_so &so : sos)
-      if (svr4_same (solib->original_name.c_str (), so.name.c_str (), lm_info,
-		     *so.lm_info))
-	return debug_base;
-
-  return 0;
+  return lm_info.debug_base;
 }
 
 /* Search order for ELF DSOs linked with -Bsymbolic.  Those DSOs have a
diff --git a/gdb/solib-svr4.h b/gdb/solib-svr4.h
index 4584d18..7b38ff4 100644
--- a/gdb/solib-svr4.h
+++ b/gdb/solib-svr4.h
@@ -34,6 +34,10 @@ struct svr4_so;
 
 struct lm_info_svr4 final : public lm_info
 {
+  explicit lm_info_svr4 (CORE_ADDR debug_base)
+    : debug_base (debug_base)
+  {}
+
   /* Amount by which addresses in the binary should be relocated to
      match the inferior.  The direct inferior value is L_ADDR_INFERIOR.
      When prelinking is involved and the prelink base address changes,
@@ -49,6 +53,18 @@ struct lm_info_svr4 final : public lm_info
 
   /* Values read in from inferior's fields of the same name.  */
   CORE_ADDR l_ld = 0, l_next = 0, l_prev = 0, l_name = 0;
+
+  /* The address of the dynamic linker structure (r_debug) this solib comes
+     from.  This identifies which namespace this library is in.
+
+     This field can be 0 in the following situations:
+
+       - we receive the libraries through XML from an old gdbserver that
+	 doesn't include the "lmid" field
+       - the default debug base is not yet known
+
+     In other cases, this field should have a sensible value.  */
+  CORE_ADDR debug_base;
 };
 
 using lm_info_svr4_up = std::unique_ptr<lm_info_svr4>;
@@ -121,8 +137,9 @@ private:
   CORE_ADDR find_r_ldsomap (svr4_info *info) const;
   owning_intrusive_list<solib> default_sos (svr4_info *info) const;
   int read_so_list (svr4_info *info, CORE_ADDR lm, CORE_ADDR prev_lm,
-		    std::vector<svr4_so> &sos, int ignore_first) const;
-  lm_info_svr4_up read_lm_info (CORE_ADDR lm_addr) const;
+		    CORE_ADDR debug_base, std::vector<svr4_so> &sos,
+		    int ignore_first) const;
+  lm_info_svr4_up read_lm_info (CORE_ADDR lm_addr, CORE_ADDR debug_base) const;
   int has_lm_dynamic_from_link_map () const;
   CORE_ADDR lm_addr_check (const solib &so, bfd *abfd) const;
   CORE_ADDR read_r_next (CORE_ADDR debug_base) const;
diff --git a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
index eb12dad..51815c3 100644
--- a/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
+++ b/gdb/testsuite/gdb.base/dlmopen-ns-ids.exp
@@ -38,6 +38,70 @@ if { [build_executable "failed to build" $testfile $srcfile \
     return
 }
 
+# Return a list of shared libraries extract from the "info sharedlibrary"
+# command.  Each item in the list is itself a list with the following items:
+#
+#  - "from" address
+#  - "to" address
+#  - namespace ID
+#  - name (file path)
+
+proc get_info_shared {{arg ""}} {
+    set from_re "($::hex)\\s+"
+    set to_re "($::hex)\\s+"
+    set ns_re "(?:($::decimal)\\s+)?"
+    set syms_read_re "(Yes( \\(\\*\\))?|No)\\s+"
+    set name_re "(\[^\r\n\]+)"
+    set libs {}
+
+    gdb_test_multiple "info sharedlibrary $arg" "" {
+	-re {From\s+To\s+(NS\s+)?Syms Read\s+Shared Object Library\r\n} {
+		exp_continue
+	}
+
+	-re "^${from_re}${to_re}${ns_re}${syms_read_re}${name_re}\r\n" {
+		set from $expect_out(1,string)
+		set to $expect_out(2,string)
+		set ns $expect_out(3,string)
+		set name $expect_out(4,string)
+
+		lappend libs [list $from $to $ns $name]
+		exp_continue
+	}
+
+	-re {^\(\*\): Shared library is missing debugging information\.\r\n} {
+	    exp_continue
+	}
+
+	-re "^$::gdb_prompt " {
+	    pass $gdb_test_name
+	}
+    }
+
+    return $libs
+}
+
+# Verify that "info sharedlibrary" does not contain duplicate entries.
+
+proc check_no_duplicates {} {
+    with_test_prefix "check no duplicates" {
+	set libs [get_info_shared]
+	array set seen {}
+	set seen_duplicate 0
+
+	foreach lib $libs {
+	    if {[info exists seen($lib)]} {
+		verbose -log "already seen: $lib"
+		set seen_duplicate 1
+	    }
+
+	    set seen($lib) 1
+	}
+
+	gdb_assert {!$seen_duplicate} "no duplicates"
+    }
+}
+
 # Run the command "info sharedlibrary" and get the first namespace
 # for the so
 proc get_first_so_ns {} {
@@ -81,6 +145,8 @@ proc test_info_shared {} {
 	"From\\s+To\\s+Linker NS\\s+Syms\\s+Read\\s+Shared Object Library.*" \
 	"after loading everything"
 
+    check_no_duplicates
+
     gdb_assert {[get_first_so_ns] == 1} "before closing any library"
 
     gdb_test "next" ".*second dlclose.*" "close first library"