If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[jingham]

Summary

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".

[aprantl]

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

83

Why not just write C++11-style = LLDB_INVALID_ADDRESS in the declaration?

102

clang-format :-)

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h

107

Perhaps add a /// comment explaining what is being stored here?

[jingham]

Added a comment and removed an errant space.

[jingham]

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

83

I think it's confusing to have some declaration initialization and some constructor initialization, and I didn't want to change this class over to all initialization as it would add noise to the patch.

[aprantl]

Let's try it.

[aprantl]

If this patch works, then this bot should turn green: http://green.lab.llvm.org/green/view/LLDB/job/lldb-sanitized/

[jasonmolenda]

LGTM.

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

104

Do you want to copy the image_infos_address value into m_maybe_image_infos_address so we can detect the next exec?

[jingham]

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

104

You don't need to. If we've exec'ed we have to call DoInitialImageFetch again - since the world has changed - which resets the value. I don't want to do it twice, but I can put in a comment explaining why if that would help.

[jasonmolenda]

I don't feel strongly about it, your choice. But if I misunderstood it this time, I'll likely misunderstand it again in the future. :)

[jingham]

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

104

Eh, that's dumb, it's just a single assign, so why not. I added that in the committed version.

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h

protected:
 
   lldb::addr_t
   GetDyldLockVariableAddressFromModule(lldb_private::Module *module);
 
   uint32_t m_image_infos_stop_id; // The Stop ID the last time we
                                   // loaded/unloaded images
   lldb::user_id_t m_break_id;
   mutable std::recursive_mutex m_mutex;
+  lldb::addr_t m_maybe_image_infos_address; // If dyld is still maintaining the
+                                            // all_image_infos address, store it
+                                            // here so we can use it to detect
+                                            // exec's when talking to
+                                            // debugservers that don't support
+                                            // the "reason:exec" annotation.
 
 private:
   DISALLOW_COPY_AND_ASSIGN(DynamicLoaderMacOS);
 };
 
 #endif // liblldb_DynamicLoaderMacOS_h_

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

DynamicLoader *DynamicLoaderMacOS::CreateInstance(Process *process,
   return NULL;
 }
 
 //----------------------------------------------------------------------
 // Constructor
 //----------------------------------------------------------------------
 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
 //----------------------------------------------------------------------
 DynamicLoaderMacOS::~DynamicLoaderMacOS() {
   if (LLDB_BREAK_ID_IS_VALID(m_break_id))
     m_process->GetTarget().RemoveBreakpointByID(m_break_id);
 }
 
 bool DynamicLoaderMacOS::ProcessDidExec() {
   std::lock_guard<std::recursive_mutex> baseclass_guard(GetMutex());
   bool did_exec = false;
   if (m_process) {
     // If we are stopped after an exec, we will have only one thread...
     if (m_process->GetThreadList().GetSize() == 1) {
+      // 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) {
+          // We don't really have to reset this here, since we are going to
+          // call DoInitialImageFetch right away to handle the exec.  But in
+          // case anybody looks at it in the meantime, it can't hurt.
+          m_maybe_image_infos_address = 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;
             }
           }
         }
       }
     }
+  }
 
   if (did_exec) {
     m_libpthread_module_wp.reset();
     m_pthread_getspecific_addr.Clear();
   }
   return did_exec;
 }
 
if (JSONImageInformationIntoImageInfo(all_image_info_json_sp,
                     (uint64_t)image_infos.size());
 
       UpdateSpecialBinariesFromNewImageInfos(image_infos);
       AddModulesUsingImageInfos(image_infos);
     }
   }
 
   m_dyld_image_infos_stop_id = m_process->GetStopID();
+  m_maybe_image_infos_address = m_process->GetImageInfoAddress();
 }
 
 bool DynamicLoaderMacOS::NeedToDoInitialImageFetch() { return true; }
 
 //----------------------------------------------------------------------
 // Static callback function that gets called when our DYLD notification
 // breakpoint gets hit. We update all of our image infos and then let our super
 // class DynamicLoader class decide if we should stop or not (based on global