Add a setting to always run all threads when stepping

[jingham]

Summary

There are some platforms (the xnu kernel being one) where trying to run just one thread can cause problems, and you need to always run all threads whenever you do more than a single step-i.

This patch adds a target.process.run-all-threads setting which overrides the default "run-mode" value for all the stepping commands.

I also needed to come up with a way to test this. The easiest way to do that was to use a scripted thread plan so that I could check the incoming value. But to do that I needed to plumb the stop others through the scripted-step & the ThreadPlanPython, which this patch also does.

[jingham]

clang-format

[jingham]

Remove a file that got inadvertently included from another commit.

[friss]

This LGTM. The only comment I had is about a comment you added on preexisting code:

lldb/source/Commands/CommandObjectThread.cpp

486–490

It feels like the last sentence shouldn't be there.

Also, the comment doesn't explain *why* "we need to pretend".

[jingham]

Clarify comment

[jingham]

lldb/source/Commands/CommandObjectThread.cpp

486–490

Is this better?

[friss]

lldb/source/Commands/CommandObjectThread.cpp

486–490

Yes, although it still doesn't explain why this implementation detail matters. The comment makes it sound it shouldn't matter. It would be great to document what would break if we didn't set it.

[jingham]

reduce comment to the part appropriate to this change.

[jingham]

lldb/source/Commands/CommandObjectThread.cpp

486–490

I don't know why the non-stop mode does things the way it does. That and the fact that it is most likely not very reliable makes me not what to try to put down how it does work here.

All that's relevant to this patch is that if non-stop mode is on, you are running all threads always, so whether you want to run them only when stepping is not relevant, and that's why in this case we aren't checking the setting.

[friss]

Sure. As I said I know this is unrelated to the path. this LGTM

lldb/bindings/interface/SBThreadPlan.i

~SBThreadPlan ();
     SetPlanComplete (bool success);
 
     bool
     IsPlanComplete();
 
     bool
     IsPlanStale();
 
+    %feature("docstring", "Return whether this plan will ask to stop other threads when it runs.") GetStopOthers;
+    bool
+    GetStopOthers();
+  
+    %feature("docstring", "Set whether this plan will ask to stop other threads when it runs.") GetStopOthers;
+    void
+    SetStopOthers(bool stop_others);
+
     SBThreadPlan
     QueueThreadPlanForStepOverRange (SBAddress &start_address,
                                      lldb::addr_t range_size);
 
     SBThreadPlan
     QueueThreadPlanForStepInRange (SBAddress &start_address,
                                    lldb::addr_t range_size);
 

lldb/include/lldb/API/SBThreadPlan.h

public:
   void SetPlanComplete(bool success);
 
   bool IsPlanComplete();
 
   bool IsPlanStale();
 
   bool IsValid();
 
+  bool GetStopOthers();
+
+  void SetStopOthers(bool stop_others);
+
   // This section allows an SBThreadPlan to push another of the common types of
   // plans...
   SBThreadPlan QueueThreadPlanForStepOverRange(SBAddress &start_address,
                                                lldb::addr_t range_size);
   SBThreadPlan QueueThreadPlanForStepOverRange(SBAddress &start_address,
                                                lldb::addr_t range_size,
                                                SBError &error);
 

lldb/include/lldb/Target/Process.h

public:
   bool GetDetachKeepsStopped() const;
   void SetDetachKeepsStopped(bool keep_stopped);
   bool GetWarningsOptimization() const;
   bool GetWarningsUnsupportedLanguage() const;
   bool GetStopOnExec() const;
   std::chrono::seconds GetUtilityExpressionTimeout() const;
   bool GetOSPluginReportsAllThreads() const;
   void SetOSPluginReportsAllThreads(bool does_report);
+  bool GetSteppingRunsAllThreads() const;
 
 protected:
   Process *m_process; // Can be nullptr for global ProcessProperties
   std::unique_ptr<ProcessExperimentalProperties> m_experimental_properties_up;
 };
 
 typedef std::shared_ptr<ProcessProperties> ProcessPropertiesSP;
 

lldb/include/lldb/Target/ThreadPlanPython.h

public:
   bool ValidatePlan(Stream *error) override;
 
   bool ShouldStop(Event *event_ptr) override;
 
   bool MischiefManaged() override;
 
   bool WillStop() override;
 
-  bool StopOthers() override;
+  bool StopOthers() override { return m_stop_others; }
+
+  void SetStopOthers(bool new_value) { m_stop_others = new_value; }
 
   void DidPush() override;
 
   bool IsPlanStale() override;
 
 protected:
   bool DoPlanExplainsStop(Event *event_ptr) override;
 
   lldb::StateType GetPlanRunState() override;
   
   ScriptInterpreter *GetScriptInterpreter();
 
 private:
   std::string m_class_name;
   StructuredDataImpl *m_args_data; // We own this, but the implementation
                                    // has to manage the UP (since that is
                                    // how it gets stored in the
                                    // SBStructuredData).
   std::string m_error_str;
   StructuredData::ObjectSP m_implementation_sp;
   bool m_did_push;
+  bool m_stop_others;
 
   ThreadPlanPython(const ThreadPlanPython &) = delete;
   const ThreadPlanPython &operator=(const ThreadPlanPython &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_THREADPLANPYTHON_H

lldb/source/API/SBThreadPlan.cpp

bool SBThreadPlan::IsValid() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBThreadPlan, IsValid);
 
   ThreadPlanSP thread_plan_sp(GetSP());
   if (thread_plan_sp)
     return thread_plan_sp->ValidatePlan(nullptr);
   return false;
 }
 
+bool SBThreadPlan::GetStopOthers() {
+  LLDB_RECORD_METHOD_NO_ARGS(bool, SBThreadPlan, GetStopOthers);
+
+  ThreadPlanSP thread_plan_sp(GetSP());
+  if (thread_plan_sp)
+    return thread_plan_sp->StopOthers();
+  return false;
+}
+
+void SBThreadPlan::SetStopOthers(bool stop_others) {
+  LLDB_RECORD_METHOD(void, SBThreadPlan, SetStopOthers, (bool), stop_others);
+
+  ThreadPlanSP thread_plan_sp(GetSP());
+  if (thread_plan_sp)
+    thread_plan_sp->SetStopOthers(stop_others);
+}
+
 // This section allows an SBThreadPlan to push another of the common types of
 // plans...
 //
 // FIXME, you should only be able to queue thread plans from inside the methods
 // of a Scripted Thread Plan.  Need a way to enforce that.
 
 SBThreadPlan
 SBThreadPlan::QueueThreadPlanForStepOverRange(SBAddress &sb_start_address,
LLDB_REGISTER_METHOD(uint64_t, SBThreadPlan, GetStopReasonDataAtIndex,
                        (uint32_t));
   LLDB_REGISTER_METHOD_CONST(lldb::SBThread, SBThreadPlan, GetThread, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBThreadPlan, GetDescription,
                              (lldb::SBStream &));
   LLDB_REGISTER_METHOD(void, SBThreadPlan, SetPlanComplete, (bool));
   LLDB_REGISTER_METHOD(bool, SBThreadPlan, IsPlanComplete, ());
   LLDB_REGISTER_METHOD(bool, SBThreadPlan, IsPlanStale, ());
   LLDB_REGISTER_METHOD(bool, SBThreadPlan, IsValid, ());
+  LLDB_REGISTER_METHOD(void, SBThreadPlan, SetStopOthers, (bool));
+  LLDB_REGISTER_METHOD(bool, SBThreadPlan, GetStopOthers, ());
   LLDB_REGISTER_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                        QueueThreadPlanForStepOverRange,
                        (lldb::SBAddress &, lldb::addr_t));
   LLDB_REGISTER_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                        QueueThreadPlanForStepOverRange,
                        (lldb::SBAddress &, lldb::addr_t, lldb::SBError &));
   LLDB_REGISTER_METHOD(lldb::SBThreadPlan, SBThreadPlan,
                        QueueThreadPlanForStepInRange,

lldb/source/Commands/CommandObjectThread.cpp

public:
   void OptionParsingStarting(ExecutionContext *execution_context) override {
     m_step_in_avoid_no_debug = eLazyBoolCalculate;
     m_step_out_avoid_no_debug = eLazyBoolCalculate;
     m_run_mode = eOnlyDuringStepping;
 
     // Check if we are in Non-Stop mode
     TargetSP target_sp =
         execution_context ? execution_context->GetTargetSP() : TargetSP();
-    if (target_sp && target_sp->GetNonStopModeEnabled())
+    if (target_sp && target_sp->GetNonStopModeEnabled()) {
+      // NonStopMode runs all threads by definition, so when it is on we don't
+      // need to check the process setting for runs all threads.
       m_run_mode = eOnlyThisThread;
+    } else {
+      ProcessSP process_sp =
+          execution_context ? execution_context->GetProcessSP() : ProcessSP();
+      if (process_sp && process_sp->GetSteppingRunsAllThreads())
+        m_run_mode = eAllThreads;
+    }
 
     m_avoid_regexp.clear();
     m_step_in_target.clear();
     m_step_count = 1;
     m_end_line = LLDB_INVALID_LINE_NUMBER;
     m_end_line_is_block_end = false;
   }
 
bool DoExecute(Args &command, CommandReturnObject &result) override {
     const lldb::RunMode stop_other_threads = m_options.m_run_mode;
 
     // This is a bit unfortunate, but not all the commands in this command
     // object support only while stepping, so I use the bool for them.
     bool bool_stop_other_threads;
     if (m_options.m_run_mode == eAllThreads)
       bool_stop_other_threads = false;
     else if (m_options.m_run_mode == eOnlyDuringStepping)
-      bool_stop_other_threads =
-          (m_step_type != eStepTypeOut && m_step_type != eStepTypeScripted);
+      bool_stop_other_threads = (m_step_type != eStepTypeOut);
     else
       bool_stop_other_threads = true;
 
     ThreadPlanSP new_plan_sp;
     Status new_plan_status;
 
     if (m_step_type == eStepTypeInto) {
       StackFrame *frame = thread->GetStackFrameAtIndex(0).get();

lldb/source/Target/Process.cpp

 
 std::chrono::seconds ProcessProperties::GetUtilityExpressionTimeout() const {
   const uint32_t idx = ePropertyUtilityExpressionTimeout;
   uint64_t value = m_collection_sp->GetPropertyAtIndexAsUInt64(
       nullptr, idx, g_process_properties[idx].default_uint_value);
   return std::chrono::seconds(value);
 }
 
+bool ProcessProperties::GetSteppingRunsAllThreads() const {
+  const uint32_t idx = ePropertySteppingRunsAllThreads;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+      nullptr, idx, g_process_properties[idx].default_uint_value != 0);
+}
+
 bool ProcessProperties::GetOSPluginReportsAllThreads() const {
   const bool fail_value = true;
   const Property *exp_property =
       m_collection_sp->GetPropertyAtIndex(nullptr, true, ePropertyExperimental);
   OptionValueProperties *exp_values =
       exp_property->GetValue()->GetAsProperties();
   if (!exp_values)
     return fail_value;

lldb/source/Target/TargetProperties.td

def WarningUnsupportedLanguage: Property<"unsupported-language-warnings", "Boolean">,
     Desc<"If true, warn when stopped in code that is written in a source language that LLDB does not support.">;
   def StopOnExec: Property<"stop-on-exec", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"If true, stop when a shared library is loaded or unloaded.">;
   def UtilityExpressionTimeout: Property<"utility-expression-timeout", "UInt64">,
     DefaultUnsignedValue<15>,
     Desc<"The time in seconds to wait for LLDB-internal utility expressions.">;
+  def SteppingRunsAllThreads: Property<"run-all-threads", "Boolean">,
+    DefaultFalse,
+    Desc<"If true, stepping operations will run all threads.  This is equivalent to setting the run-mode option to 'all-threads'.">;
 }
 
 let Definition = "platform" in {
   def UseModuleCache: Property<"use-module-cache", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"Use module cache.">;
   def ModuleCacheDirectory: Property<"module-cache-directory", "FileSpec">,

lldb/source/Target/Thread.cpp

lldb::ThreadPlanSP Thread::QueueThreadPlanForStepScripted(
   StructuredDataImpl *extra_args_impl = nullptr; 
   if (extra_args_sp) {
     extra_args_impl = new StructuredDataImpl();
     extra_args_impl->SetObjectSP(extra_args_sp);
   }
 
   ThreadPlanSP thread_plan_sp(new ThreadPlanPython(*this, class_name, 
                                                    extra_args_impl));
-
+  thread_plan_sp->SetStopOthers(stop_other_threads);
   status = QueueThreadPlan(thread_plan_sp, abort_other_plans);
   return thread_plan_sp;
 }
 
 uint32_t Thread::GetIndexID() const { return m_index_id; }
 
 TargetSP Thread::CalculateTarget() {
   TargetSP target_sp;

lldb/source/Target/ThreadPlanPython.cpp

 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // ThreadPlanPython
 
-ThreadPlanPython::ThreadPlanPython(Thread &thread, const char *class_name, 
+ThreadPlanPython::ThreadPlanPython(Thread &thread, const char *class_name,
                                    StructuredDataImpl *args_data)
     : ThreadPlan(ThreadPlan::eKindPython, "Python based Thread Plan", thread,
                  eVoteNoOpinion, eVoteNoOpinion),
-      m_class_name(class_name), m_args_data(args_data), m_did_push(false) {
+      m_class_name(class_name), m_args_data(args_data), m_did_push(false),
+      m_stop_others(false) {
   SetIsMasterPlan(true);
   SetOkayToDiscard(true);
   SetPrivate(false);
 }
 
 ThreadPlanPython::~ThreadPlanPython() {
   // FIXME, do I need to decrement the ref count on this implementation object
   // to make it go away?
if (script_interp) {
       run_state = script_interp->ScriptedThreadPlanGetRunState(
           m_implementation_sp, script_error);
     }
   }
   return run_state;
 }
 
 // The ones below are not currently exported to Python.
-
-bool ThreadPlanPython::StopOthers() {
-  // For now Python plans run all threads, but we should add some controls for
-  // this.
-  return false;
-}
-
 void ThreadPlanPython::GetDescription(Stream *s, lldb::DescriptionLevel level) {
   s->Printf("Python thread plan implemented by class %s.",
             m_class_name.c_str());
 }
 
 bool ThreadPlanPython::WillStop() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
   LLDB_LOGF(log, "%s called on Python Thread Plan: %s )", LLVM_PRETTY_FUNCTION,
             m_class_name.c_str());
   return true;
 }

lldb/test/API/functionalities/step_scripted/Steps.py

def should_stop(self, event):
 
         # If we've stepped out of this frame, stop.
         if not self.frame.IsValid():
             return True
 
         if not self.value.IsValid():
             return True
 
-        print("Got next value: %d"%(self.value.GetValueAsUnsigned()))
         if not self.value.GetValueDidChange():
             self.child_thread_plan = self.queue_child_thread_plan()
             return False
         else:
             return True
+
+# This plan does nothing, but sets stop_mode to the
+# value of GetStopOthers for this plan.
+class StepReportsStopOthers():
+    stop_mode_dict = {}
+    
+    def __init__(self, thread_plan, args_data, dict):
+        self.thread_plan = thread_plan
+        self.key = args_data.GetValueForKey("token").GetStringValue(1000)
+        
+    def should_stop(self, event):
+        self.thread_plan.SetPlanComplete(True)
+        print("Called in should_stop")
+        StepReportsStopOthers.stop_mode_dict[self.key] = self.thread_plan.GetStopOthers()
+        return True
+
+    def should_step(self):
+        return True
+
+    def explains_stop(self, event):
+        return True

lldb/test/API/functionalities/step_scripted/TestStepScripted.py

 """
 Tests stepping with scripted thread plans.
 """
-
+import threading
 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 
 class StepScriptedTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
def do_test_checking_variable(self, use_cli):
         # We should not have exited:
         self.assertEqual(process.GetState(), lldb.eStateStopped, "We are stopped")
         
         # We should still be in foo:
         self.assertEqual("foo", frame.GetFunctionName())
 
         # And foo should have changed:
         self.assertTrue(foo_val.GetValueDidChange(), "Foo changed")
+
+    def test_stop_others_from_command(self):
+        """Test that the stop-others flag is set correctly by the command line.
+           Also test that the run-all-threads property overrides this."""
+        self.do_test_stop_others()
+
+    def run_step(self, stop_others_value, run_mode, token):
+        import Steps
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+
+        cmd = "thread step-scripted -C Steps.StepReportsStopOthers -k token -v %s"%(token)
+        if run_mode != None:
+            cmd = cmd + " --run-mode %s"%(run_mode)
+        print(cmd)
+        interp.HandleCommand(cmd, result)
+        self.assertTrue(result.Succeeded(), "Step scripted failed: %s."%(result.GetError()))
+        print(Steps.StepReportsStopOthers.stop_mode_dict)
+        value = Steps.StepReportsStopOthers.stop_mode_dict[token]
+        self.assertEqual(value, stop_others_value, "Stop others has the correct value.")
+        
+    def do_test_stop_others(self):
+        self.build()
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                                                            "Set a breakpoint here",
+                                                                            self.main_source_file)
+        # First run with stop others false and see that we got that.
+        thread_id = ""
+        if sys.version_info.major == 2:
+            thread_id = str(threading._get_ident())
+        else:
+            thread_id = str(threading.get_ident())
+
+        # all-threads should set stop others to False.
+        self.run_step(False, "all-threads", thread_id)
+
+        # this-thread should set stop others to True
+        self.run_step(True, "this-thread", thread_id)
+
+        # The default value should be stop others:
+        self.run_step(True, None, thread_id)
+
+        # The target.process.run-all-threads should override this:
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+        
+        interp.HandleCommand("settings set target.process.run-all-threads true", result)
+        self.assertTrue(result.Succeeded, "setting run-all-threads works.")
+
+        self.run_step(False, None, thread_id)
+
+        
+        
+
+