[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:571-579
+
+  // ld.so saves empty file name for the executable file in the link map.
+  // When argv[0] is ld.so, we need to be update executable path.
+  if (file_path.empty()) {
+    MemoryRegionInfo region;
+    Status region_status =
+        m_process->GetMemoryRegionInfo(entry.dyn_addr, region);
----------------
Setting this here defeats all the `if (!SOEntryIsMainExecutable(entry))` checks in this code, for the "normal" case of running the executable directly. I'm guessing that will result in multiple modules being added for the main executable.

I think it would be better to remove this logic from here (it seems a bit too fancy for a function called `ReadSOEntryFromMemory`), and then teach the higher levels that they ought to treat "" specially (in some circumstances).


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:345-350
+      if (NameMatches(m_process->GetTarget()
+                          .GetExecutableModulePointer()
+                          ->GetFileSpec()
+                          .GetFilename()
+                          .GetCString(),
+                      NameMatch::StartsWith, "ld-")) {
----------------
This check is unnecessary. A statically linked executable can still contain the dynamic linker structures (even though it does not have an elf interpreter), if it contains runtime calls to dlopen. In this case the main executable will contain _dl_debug_state et al. through the inclusion of libdl.a, and omitting this check should make that case work too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108061