[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D158583#4609320 <https://reviews.llvm.org/D158583#4609320>, @DavidSpickett wrote:

> I'm not familiar with this mechanism but just out of curiosity: `ld.so loads the main executable and any dependent shared libraries and wants to update the "_r_debug" structure, but it now finds "_r_debug" in the a.out program and updates the state in this other copy`
>
> Is this some undefined behaviour or is there a good use case for this? From the program's point of view.

This is just how name lookups happen across shared library boundaries as far as I know. External symbols get resolved by searching the shared libraries including the main executable.

I have verified this is indeed what is happening by debugging the ld.so binary and stepping through it at this code in rtld.c:

  /* Notify the debugger all new objects are now ready to go.  We must re-get
     the address since by now the variable might be in another object.  */
  r = _dl_debug_initialize (0, LM_ID_BASE);
  r->r_state = RT_CONSISTENT;
  _dl_debug_state ();

The code for _dl_debug_initialize() looks like:

  struct r_debug *
  _dl_debug_initialize (ElfW(Addr) ldbase, Lmid_t ns)
  {
    struct r_debug *r;
  
    if (ns == LM_ID_BASE)
      r = &_r_debug; /// <- this will switch to 
    else
      r = &GL(dl_ns)[ns]._ns_debug;
  
    if (r->r_map == NULL || ldbase != 0)
      {
        /* Tell the debugger where to find the map of loaded objects.  */
        r->r_version = 1	/* R_DEBUG_VERSION XXX */;
        r->r_ldbase = ldbase ?: _r_debug.r_ldbase;
        r->r_map = (void *) GL(dl_ns)[ns]._ns_loaded;
        r->r_brk = (ElfW(Addr)) &_dl_debug_state;
      }
  
    return r;
  }

And the line "r = &_r_debug;" picks up the first item in the library search paths for "_r_debug" which gets the one from the a.out program...



================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:260-266
+    // the dynamic loader. The problem happens as soon as the executable or
+    // shared library that exports another "_r_debug" is loaded by the dynamic
+    // loader due to the way symbols are found using the shared library search
+    // paths will mean that the new copy takes precedence over the one in the
+    // dynamic loader and the dynamic loader will be updating the other copy
+    // somewhere else that we won't find and that copy will have the
+    // eConsistent state.
----------------
DavidSpickett wrote:
> DavidSpickett wrote:
> > Can we split this sentence just for readability? Bullet points of each step might be clearer.
> What you have in the commit message basically.
yeah, I was tired last night when finishing this up, I will grab the stuff from the commit message


================
Comment at: lldb/test/API/functionalities/dyld-multiple-rdebug/TestDyldWithMultupleRDebug.py:22
+    @no_debug_info_test
+    @skipIf(oslist=["linux"], archs=["arm"])
+    def test(self):
----------------
DavidSpickett wrote:
> Any specific reason for this? Not that it really matters, it'll get plenty of testing elsewhere.
Yeah, copy paste issue. I would be able to remove this.


================
Comment at: lldb/test/API/functionalities/dyld-multiple-rdebug/TestDyldWithMultupleRDebug.py:28
+        exe = self.getBuildArtifact("a.out")
+        print(exe)
+        target = self.dbg.CreateTarget(exe)
----------------
DavidSpickett wrote:
> Leftover debug print.
good catch, yes!


================
Comment at: lldb/test/API/functionalities/dyld-multiple-rdebug/TestDyldWithMultupleRDebug.py:54
+        self.assertIn("main",
+                      thread.GetFrameAtIndex(0).GetDisplayFunctionName())
+        process.Continue()
----------------
aprantl wrote:
> You should be able to shorten the test setup considerably by using lldbutil.run_to_name_breakpoint() and only manually setting the second breakpoint afterwards.
will do


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158583/new/

https://reviews.llvm.org/D158583