[lldb] Inline invariant params to AppleThreadPlanStepThrough (NFC)

[kastiglione]

Summary

These two AppleThreadPlanStepThrough thread plans have parameterized behavior
that is unutilized. To make their interface and implementation simpler, this
change inlines those outside parameters.

[kastiglione]

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp

1163

This is one parameter being inlined.

1188–1189

These are two other parameters being inlined.

[jingham]

I can't think of a really good reason why you would need to override the general "step in avoids nodebug" behavior. I'm pretty sure I was thinking of a trampoline that got you half way to somewhere interesting, at which point you would want to negotiate again for the trampoline to take you the rest of the way. In that case you would not want to obey the general setting. But this setting controls what happens when you don't find a good place to go, so that control isn't necessary.

It still seems to me like a trampoline which knew that to implement itself, all it had to do was a couple of stepi's - for instance if a dyld stub knew that the stub had been filled in and would jump straight to the target, it could run without letting the other threads go. That's actually something the dyld stubs could figure out, they just don't at present. So I'd rather not remove this control.

[kastiglione]

In D96276#2549007, @jingham wrote:

It still seems to me like a trampoline which knew that to implement itself, all it had to do was a couple of stepi's - for instance if a dyld stub knew that the stub had been filled in and would jump straight to the target, it could run without letting the other threads go. That's actually something the dyld stubs could figure out, they just don't at present. So I'd rather not remove this control.

Happy to restore the stop_others parameter, but for my understanding I have two questions:

1. Is the behavior you envision something that applies to these specific thread plan subclasses (ie not dyld stubs)
2. Will callers decide to stop others from the outside, or is the decision to stop others something that these subclasses would make internally?

[jingham]

In D96276#2549406, @kastiglione wrote:

In D96276#2549007, @jingham wrote:

It still seems to me like a trampoline which knew that to implement itself, all it had to do was a couple of stepi's - for instance if a dyld stub knew that the stub had been filled in and would jump straight to the target, it could run without letting the other threads go. That's actually something the dyld stubs could figure out, they just don't at present. So I'd rather not remove this control.

Happy to restore the stop_others parameter, but for my understanding I have two questions:

1. Is the behavior you envision something that applies to these specific thread plan subclasses (ie not dyld stubs)
2. Will callers decide to stop others from the outside, or is the decision to stop others something that these subclasses would make internally?

Good questions. I was reading this incorrectly. This is the enqueuing plan telling the StepThrough plan whether to stop others. That actually seems like a bad thing to do, since the enqueuer can't really know whether it is actually safe to stop others while running the enqueued plan. These should go.

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp

if (impl_addr != LLDB_INVALID_ADDRESS) {
         break;
       }
       if (log && log->GetVerbose())
         flag_value.GetScalar() = 1;
       else
         flag_value.GetScalar() = 0; // FIXME - Set to 0 when debugging is done.
       dispatch_values.PushValue(flag_value);
 
-      // The step through code might have to fill in the cache, so it
-      // is not safe to run only one thread.  So we override the
-      // stop_others value passed in to us here:
-      const bool trampoline_stop_others = false;
       ret_plan_sp = std::make_shared<AppleThreadPlanStepThroughObjCTrampoline>(
-          thread, *this, dispatch_values, isa_addr, sel_addr,
-          trampoline_stop_others);
+          thread, *this, dispatch_values, isa_addr, sel_addr);
       if (log) {
         StreamString s;
         ret_plan_sp->GetDescription(&s, eDescriptionLevelFull);
         LLDB_LOGF(log, "Using ObjC step plan: %s.\n", s.GetData());
       }
     }
   }
   
   // Finally, check if we have hit an "optimized dispatch" function.  This will
   // either directly call the base implementation or dispatch an objc_msgSend
   // if the method has been overridden.  So we just do a "step in/step out",
   // setting a breakpoint on objc_msgSend, and if we hit the msgSend, we 
   // will automatically step in again.  That's the job of the 
   // AppleThreadPlanStepThroughDirectDispatch.
   if (!this_dispatch && !ret_plan_sp) {
     MsgsendMap::iterator pos;
     pos = m_opt_dispatch_map.find(curr_pc);
     if (pos != m_opt_dispatch_map.end()) {
-
       const char *opt_name = g_opt_dispatch_names[(*pos).second];
-
-      bool trampoline_stop_others = false;
-      LazyBool step_in_should_stop = eLazyBoolCalculate;
-      ret_plan_sp = std::make_shared<AppleThreadPlanStepThroughDirectDispatch> (
-          thread, *this, opt_name, trampoline_stop_others, step_in_should_stop);
+      ret_plan_sp = std::make_shared<AppleThreadPlanStepThroughDirectDispatch>(
+          thread, *this, opt_name);
     }
   }
 
   return ret_plan_sp;
 }
 
 FunctionCaller *
 AppleObjCTrampolineHandler::GetLookupImplementationFunctionCaller() {
   return m_impl_code->GetFunctionCaller();
 }

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h

 #include "lldb/lldb-types.h"
 
 namespace lldb_private {
 
 class AppleThreadPlanStepThroughObjCTrampoline : public ThreadPlan {
 public:
   AppleThreadPlanStepThroughObjCTrampoline(
       Thread &thread, AppleObjCTrampolineHandler &trampoline_handler,
-      ValueList &values, lldb::addr_t isa_addr, lldb::addr_t sel_addr,
-      bool stop_others);
+      ValueList &values, lldb::addr_t isa_addr, lldb::addr_t sel_addr);
 
   ~AppleThreadPlanStepThroughObjCTrampoline() override;
 
   static bool PreResumeInitializeFunctionCaller(void *myself);
 
   void GetDescription(Stream *s, lldb::DescriptionLevel level) override;
 
   bool ValidatePlan(Stream *error) override;
 
   lldb::StateType GetPlanRunState() override;
 
   bool ShouldStop(Event *event_ptr) override;
 
-  bool StopOthers() override { return m_stop_others; }
+  // The step through code might have to fill in the cache, so it is not safe
+  // to run only one thread.
+  bool StopOthers() override { return false; }
 
   // The base class MischiefManaged does some cleanup - so you have to call it
   // in your MischiefManaged derived class.
   bool MischiefManaged() override;
 
   void DidPush() override;
 
   bool WillStop() override;
private:
   lldb::addr_t m_sel_addr;
   lldb::ThreadPlanSP m_func_sp; /// This is the function call plan.  We fill it
                                 /// at start, then set it to NULL when this plan
                                 /// is done.  That way we know to go on to:
   lldb::ThreadPlanSP m_run_to_sp;  /// The plan that runs to the target.
   FunctionCaller *m_impl_function; /// This is a pointer to a impl function that
                                    /// is owned by the client that pushes this
                                    /// plan.
-  bool m_stop_others;  /// Whether we should stop other threads.
 };
 
 class AppleThreadPlanStepThroughDirectDispatch: public ThreadPlanStepOut {
 public:
-  AppleThreadPlanStepThroughDirectDispatch(
-      Thread &thread, AppleObjCTrampolineHandler &handler,
-      llvm::StringRef dispatch_func_name, bool stop_others,
-      LazyBool step_in_avoids_code_without_debug_info);
+  AppleThreadPlanStepThroughDirectDispatch(Thread &thread,
+                                           AppleObjCTrampolineHandler &handler,
+                                           llvm::StringRef dispatch_func_name);
 
   ~AppleThreadPlanStepThroughDirectDispatch() override;
 
   void GetDescription(Stream *s, lldb::DescriptionLevel level) override;
 
   bool ShouldStop(Event *event_ptr) override;
 
-  bool StopOthers() override { return m_stop_others; }
+  bool StopOthers() override { return false; }
 
   bool MischiefManaged() override;
 
   bool DoWillResume(lldb::StateType resume_state, bool current_plan) override;
 
   void SetFlagsToDefault() override {
           GetFlags().Set(ThreadPlanStepInRange::GetDefaultFlagsValue());
   }
 
 protected:
   bool DoPlanExplainsStop(Event *event_ptr) override;
 
   AppleObjCTrampolineHandler &m_trampoline_handler;
   std::string m_dispatch_func_name;  /// Which dispatch function we're stepping
                                      /// through.
   lldb::ThreadPlanSP m_objc_step_through_sp; /// When we hit an objc_msgSend,
                                              /// we'll use this plan to get to
                                              /// its target.
   std::vector<lldb::BreakpointSP> m_msgSend_bkpts; /// Breakpoints on the objc
                                                    /// dispatch functions.
   bool m_at_msg_send;  /// Are we currently handling an msg_send
-  bool m_stop_others;  /// Whether we should stop other threads.
 
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_LANGUAGERUNTIME_OBJC_APPLEOBJCRUNTIME_APPLETHREADPLANSTEPTHROUGHOBJCTRAMPOLINE_H

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 
 // ThreadPlanStepThroughObjCTrampoline constructor
 AppleThreadPlanStepThroughObjCTrampoline::
     AppleThreadPlanStepThroughObjCTrampoline(
         Thread &thread, AppleObjCTrampolineHandler &trampoline_handler,
-        ValueList &input_values, lldb::addr_t isa_addr, lldb::addr_t sel_addr,
-        bool stop_others)
+        ValueList &input_values, lldb::addr_t isa_addr, lldb::addr_t sel_addr)
     : ThreadPlan(ThreadPlan::eKindGeneric,
                  "MacOSX Step through ObjC Trampoline", thread, eVoteNoOpinion,
                  eVoteNoOpinion),
       m_trampoline_handler(trampoline_handler),
       m_args_addr(LLDB_INVALID_ADDRESS), m_input_values(input_values),
-      m_isa_addr(isa_addr), m_sel_addr(sel_addr), m_impl_function(nullptr),
-      m_stop_others(stop_others) {}
+      m_isa_addr(isa_addr), m_sel_addr(sel_addr), m_impl_function(nullptr) {}
 
 // Destructor
 AppleThreadPlanStepThroughObjCTrampoline::
     ~AppleThreadPlanStepThroughObjCTrampoline() {}
 
 void AppleThreadPlanStepThroughObjCTrampoline::DidPush() {
   // Setting up the memory space for the called function text might require
   // allocations, i.e. a nested function call.  This needs to be done as a
if (m_args_addr == LLDB_INVALID_ADDRESS) {
       return false;
     }
     m_impl_function =
         m_trampoline_handler.GetLookupImplementationFunctionCaller();
     ExecutionContext exc_ctx;
     EvaluateExpressionOptions options;
     options.SetUnwindOnError(true);
     options.SetIgnoreBreakpoints(true);
-    options.SetStopOthers(m_stop_others);
+    options.SetStopOthers(false);
     GetThread().CalculateExecutionContext(exc_ctx);
     m_func_sp = m_impl_function->GetThreadPlanToCallFunction(
         exc_ctx, m_args_addr, options, diagnostics);
     m_func_sp->SetOkayToDiscard(true);
     PushPlan(m_func_sp);
   }
   return true;
 }
if (m_trampoline_handler.AddrIsMsgForward(target_addr)) {
 
       SymbolContext sc = GetThread().GetStackFrameAtIndex(0)->GetSymbolContext(
           eSymbolContextEverything);
       Status status;
       const bool abort_other_plans = false;
       const bool first_insn = true;
       const uint32_t frame_idx = 0;
       m_run_to_sp = GetThread().QueueThreadPlanForStepOutNoShouldStop(
-          abort_other_plans, &sc, first_insn, m_stop_others, eVoteNoOpinion,
+          abort_other_plans, &sc, first_insn, false, eVoteNoOpinion,
           eVoteNoOpinion, frame_idx, status);
       if (m_run_to_sp && status.Success())
         m_run_to_sp->SetPrivate(true);
       return false;
     }
 
     LLDB_LOGF(log, "Running to ObjC method implementation: 0x%" PRIx64,
               target_addr);
 
     ObjCLanguageRuntime *objc_runtime =
         ObjCLanguageRuntime::Get(*GetThread().GetProcess());
     assert(objc_runtime != nullptr);
     objc_runtime->AddToMethodCache(m_isa_addr, m_sel_addr, target_addr);
     LLDB_LOGF(log,
               "Adding {isa-addr=0x%" PRIx64 ", sel-addr=0x%" PRIx64
               "} = addr=0x%" PRIx64 " to cache.",
               m_isa_addr, m_sel_addr, target_addr);
 
     // Extract the target address from the value:
 
     m_run_to_sp = std::make_shared<ThreadPlanRunToAddress>(
-        GetThread(), target_so_addr, m_stop_others);
+        GetThread(), target_so_addr, false);
     PushPlan(m_run_to_sp);
     return false;
   } else if (GetThread().IsThreadPlanDone(m_run_to_sp.get())) {
     // Third stage, work the run to target plan.
     SetPlanComplete();
     return true;
   }
   return false;

 // by init.)  So we can't give up at the first objc_msgSend.
 // That means among other things that we have to handle the "ShouldStopHere" - 
 // since we can't just return control to the plan that's controlling us on the
 // first step.
 
 AppleThreadPlanStepThroughDirectDispatch ::
     AppleThreadPlanStepThroughDirectDispatch(
         Thread &thread, AppleObjCTrampolineHandler &handler,
-        llvm::StringRef dispatch_func_name, bool stop_others,
-        LazyBool step_in_avoids_code_without_debug_info)
-    : ThreadPlanStepOut(thread, nullptr, true /* first instruction */,
-                        stop_others, eVoteNoOpinion, eVoteNoOpinion,
+        llvm::StringRef dispatch_func_name)
+    : ThreadPlanStepOut(thread, nullptr, true /* first instruction */, false,
+                        eVoteNoOpinion, eVoteNoOpinion,
                         0 /* Step out of zeroth frame */,
                         eLazyBoolNo /* Our parent plan will decide this
                                when we are done */
                         ,
                         true /* Run to branch for inline step out */,
                         false /* Don't gather the return value */),
       m_trampoline_handler(handler),
       m_dispatch_func_name(std::string(dispatch_func_name)),
-      m_at_msg_send(false), m_stop_others(stop_others) {
+      m_at_msg_send(false) {
   // Set breakpoints on the dispatch functions:
   auto bkpt_callback = [&] (lldb::addr_t addr, 
                             const AppleObjCTrampolineHandler
                                 ::DispatchFunction &dispatch) {
     m_msgSend_bkpts.push_back(GetTarget().CreateBreakpoint(addr,
                                                            true /* internal */,
                                                            false /* hard */));
     m_msgSend_bkpts.back()->SetThreadID(GetThread().GetID());
   };
   handler.ForEachDispatchFunction(bkpt_callback);
 
   // We'll set the step-out plan in the DidPush so it gets queued in the right
   // order.
 
-  bool avoid_nodebug = true;
-
-  switch (step_in_avoids_code_without_debug_info) {
-  case eLazyBoolYes:
-    avoid_nodebug = true;
-    break;
-  case eLazyBoolNo:
-    avoid_nodebug = false;
-    break;
-  case eLazyBoolCalculate:
-    avoid_nodebug = GetThread().GetStepInAvoidsNoDebug();
-    break;
-  }
-  if (avoid_nodebug)
+  if (GetThread().GetStepInAvoidsNoDebug())
     GetFlags().Set(ThreadPlanShouldStopHere::eStepInAvoidNoDebug);
   else
     GetFlags().Clear(ThreadPlanShouldStopHere::eStepInAvoidNoDebug);
   // We only care about step in.  Our parent plan will figure out what to
   // do when we've stepped out again.
   GetFlags().Clear(ThreadPlanShouldStopHere::eStepOutAvoidNoDebug);
 }
 
bool AppleThreadPlanStepThroughDirectDispatch::ShouldStop(Event *event_ptr) {
   // plan:
   
   if (m_at_msg_send) {
     LanguageRuntime *objc_runtime 
       = GetThread().GetProcess()->GetLanguageRuntime(eLanguageTypeObjC);
     // There's no way we could have gotten here without an ObjC language 
     // runtime.
     assert(objc_runtime);
-    m_objc_step_through_sp 
-      = objc_runtime->GetStepThroughTrampolinePlan(GetThread(), m_stop_others);
+    m_objc_step_through_sp =
+        objc_runtime->GetStepThroughTrampolinePlan(GetThread(), false);
     // If we failed to find the target for this dispatch, just keep going and
     // let the step out complete.
     if (!m_objc_step_through_sp) {
       LLDB_LOG(log, "Couldn't find target for message dispatch, continuing.");
       return false;
     }
     // Otherwise push the step through plan and continue.
     GetThread().QueueThreadPlan(m_objc_step_through_sp, false);