[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: jasonmolenda, aprantl.
Herald added subscribers: lldb-commits, abidh.

The test "TestExec.py" has been on and off flakey on the GreenDragon MacOS bots for a while now.  I'm trying to fix that.  It adds a lot of noise to the bots.

For background:

The job of detecting an exec used to be handled by the DynamicLoaderMacOSX plugin.  But when dyld switched over to a remote API approach rather than data structures lldb inspects, that job became hard.  About the only clue remaining for the new dyld API loader (DynamicLoaderMacOS) is whether the program is back at dyld_start when it gets a SIGTRAP.  But if dyld has moved in the course of the exec then the new dyld_start address is not the one we knew it as before the exec, so this detection will fail.  To make this work on the lldb side, we'd have to refresh the shared library state every time we got a SIGTRAP.  But that's expensive.

Fortunately, newer debugservers figure this out on their end and annotate the stop packet with "reason:exec".  So there's no need to do it on the lldb side.  But because we didn't want to deal with code signing on the bots, they sometimes run older debugservers.  And indeed, when Adrian and I were investigating the failures by looking at the packet log we only saw failures when the stop reason didn't include reason:exec.

Also fortunately, in order to support other tools that haven't updated to the new dyld API's, for now dyld is still maintaining the data structures lldb used to look at.

So this patch adds back the check for the dyld info data structure moving that the DynamicLoaderMacOSX used.  For debugservers that send reason:exec, this code won't ever get run since ProcessGDBRemote will immediately turn the SIGTRAP into an exec.  And it will work correctly with older debugservers until dyld actually removes this structure.  At that point, the info address will be LLDB_INVALID_ADDRESS, and this code won't help anymore.  But presumably by then we won't have any debugservers we care about that don't send "reason:exec".


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55399

Files:
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h


Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
===================================================================
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
@@ -104,6 +104,7 @@
                                   // loaded/unloaded images
   lldb::user_id_t m_break_id;
   mutable std::recursive_mutex m_mutex;
+  lldb::addr_t m_maybe_image_infos_address;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(DynamicLoaderMacOS);
Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
===================================================================
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
@@ -79,7 +79,8 @@
 //----------------------------------------------------------------------
 DynamicLoaderMacOS::DynamicLoaderMacOS(Process *process)
     : DynamicLoaderDarwin(process), m_image_infos_stop_id(UINT32_MAX),
-      m_break_id(LLDB_INVALID_BREAK_ID), m_mutex() {}
+      m_break_id(LLDB_INVALID_BREAK_ID), m_mutex()
+      , m_maybe_image_infos_address(LLDB_INVALID_ADDRESS) {}
 
 //----------------------------------------------------------------------
 // Destructor
@@ -95,16 +96,26 @@
   if (m_process) {
     // If we are stopped after an exec, we will have only one thread...
     if (m_process->GetThreadList().GetSize() == 1) {
-      // See if we are stopped at '_dyld_start'
-      ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
-      if (thread_sp) {
-        lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
-        if (frame_sp) {
-          const Symbol *symbol =
-              frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol;
-          if (symbol) {
-            if (symbol->GetName() == ConstString("_dyld_start"))
-              did_exec = true;
+      // Maybe we still have an image infos address around?  If so see
+      // if that has changed, and if so we have exec'ed:
+      if (m_maybe_image_infos_address != LLDB_INVALID_ADDRESS) {
+        lldb::addr_t  image_infos_address = m_process->GetImageInfoAddress();
+        if (image_infos_address != m_maybe_image_infos_address)
+          did_exec = true;
+      }
+
+      if (!did_exec) {
+        // See if we are stopped at '_dyld_start'
+        ThreadSP thread_sp(m_process->GetThreadList().GetThreadAtIndex(0));
+        if (thread_sp) {
+          lldb::StackFrameSP frame_sp(thread_sp->GetStackFrameAtIndex(0));
+          if (frame_sp) {
+            const Symbol *symbol =
+                frame_sp->GetSymbolContext(eSymbolContextSymbol).symbol;
+            if (symbol) {
+              if (symbol->GetName() == ConstString("_dyld_start"))
+                did_exec = true;
+            }
           }
         }
       }
@@ -180,6 +191,7 @@
   }
 
   m_dyld_image_infos_stop_id = m_process->GetStopID();
+  m_maybe_image_infos_address = m_process->GetImageInfoAddress();
 }
 
 bool DynamicLoaderMacOS::NeedToDoInitialImageFetch() { return true; }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55399.177077.patch
Type: text/x-patch
Size: 3101 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20181207/dd008eac/attachment.bin>