[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:153
+    cursor = info_addr =
+        ResolveRendezvousAddress(m_process, m_executable_interpreter);
   else
----------------
At this point, you can just make `ResolveRendezvousAddress` a (private) member of the DYLDRendezvous class.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:318
+    if (!SOEntryIsMainExecutable(entry)) {
+      if (entry.file_spec.GetFilename().IsEmpty())
+        UpdateFileSpecIfNecessary(entry);
----------------
Since the function already has `IfNecessary` in the name, you don't need to guard the call once more.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:453-455
+    // When executable is run with argv[0] as ld.so, ld.so corresponds to
+    // m_exe_file_spec. In this case, the returned link map
+    // has empty entry for executable, which is not the main executable.
----------------
How about this:
```
// If were debugging ld.so, then all SOEntries should be treated as libraries, including the "main" one (denoted by an empty string). 
if (m_executable_interpreter)
  return false;
return !entry.file_spec;


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:552
+  // When argv[0] is ld.so, we need to be update executable path.
+  if (entry.file_spec.GetFilename().IsEmpty()) {
+    MemoryRegionInfo region;
----------------
I don't think this should make a difference, but `if(!entry.file_spec)` is shorter and consistent with the logic in `SOEntryIsMainExecutable`.


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h:186
 
+  /// It is set to true when executable is run with argv[0] as ld.so.
+  bool m_executable_interpreter;
----------------
I get what you're trying to say, but I'm having issues with these comments, as they're wrong on multiple levels. For one, argv[0] essentially has no relationship to the binary being executed (ld.so even has an argument to supply an arbitrary string as argv[0] to the executable). But more importantly, when the executable is "run" in this way (as an argument to ld.so), as far as the operating system (and lldb) is concerned ld.so _is_ the main executable. You can see this in the /proc/$PID/exe symlink, or in all the target->GetExecutableModule calls in this patch. What this patch actually does is to _disable_ the logic of treating the "main executable" (the one with an empty name in the rendezvous structure) as special, so that it can be treated as one of the libraries.

With that in mind, I think it'd be better to say that this flag indicates whether the main program is the dynamic linker/loader/program interpreter. 


================
Comment at: lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py:18-24
+        candidates = [
+                "/lib64/ld-linux-x86-64.so.2",
+                "/usr/lib/ld-linux-x86-64.so.2"
+        ]
+        exe = next((c for c in candidates if os.path.exists(c)), None)
+        if not os.path.exists(exe):
+            return
----------------
It should be possible to retrieve the actual interpreter path by inspecting the compiled executable. Something like `SBModule(exe)->FindSection(".interp")->GetSectionData()` ought to do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108061