[LLDB] Fix crash when enabling "target.process.thread.trace-thread" setting

[vangelists]

Summary

Fixes the crash occurring when target.process.thread.trace-thread is enabled.

The cause of the crash was the call of a virtual method during object construction, as shown below:

virtual Process.UpdateThreadList()
└── ProcessGDBRemote.UpdateThreadList()
    └── new ThreadGDBRemote()
        └── new Thread()
            └── new ThreadPlanBase()
                ├── new ThreadPlanAssemblyTracer()
                └── virtual ThreadPlanAssemblyTracer::EnableTracing()
                    └── virtual ThreadPlanTracer::TracingStarted()
                        └── virtual Thread::GetRegisterContext() ← Virtual method call of Thread under construction!
                            └── __cxa_pure_virtual()

This patch also contains an attempt to make the tracing API a bit better and more clear.

Generated against llvm-8.0.0 git tag (commit SHA: d2298e74235598f15594fe2c99bbac870a507c59).

lldb/include/lldb/Target/Thread.h

   virtual bool
   RestoreThreadStateFromCheckpoint(ThreadStateCheckpoint &saved_state);
 
-  void EnableTracer(bool value, bool single_step);
-
   void SetTracer(lldb::ThreadPlanTracerSP &tracer_sp);
+  void EnableTracing();
+  void DisableTracing();
+
+  void EnableSingleStepping();
+  void DisableSingleStepping();
 
   //------------------------------------------------------------------
   // Get the thread index ID. The index ID that is guaranteed to not be re-used

lldb/include/lldb/Target/ThreadPlanTracer.h

 
   virtual void TracingEnded() {}
 
-  bool EnableTracing(bool value) {
-    bool old_value = m_enabled;
-    m_enabled = value;
-    if (old_value == false && value == true)
-      TracingStarted();
-    else if (old_value == true && value == false)
-      TracingEnded();
-
-    return old_value;
-  }
-
-  bool TracingEnabled() { return m_enabled; }
-
-  bool EnableSingleStep(bool value) {
-    bool old_value = m_single_step;
-    m_single_step = value;
-    return old_value;
-  }
-
-  bool SingleStepEnabled() { return m_single_step; }
+  void EnableTracing();
+  void DisableTracing();
+  bool TracingEnabled() const;
+
+  void EnableSingleStepping();
+  void DisableSingleStepping();
+  bool SingleSteppingEnabled() const;
 
 protected:
   Thread &m_thread;

lldb/source/Target/Thread.cpp

     log->Printf("Plan stack initial state:\n%s", s.GetData());
   }
 
-  // The top most plan always gets to do the trace log...
+  // Query "target.process.thread.trace-thread" setting to find out whether
+  // the user has enabled or disabled tracing in the meantime.
+  if (GetTraceEnabledState()) {
+    EnableTracing();
+  } else {
+    DisableTracing();
+  }
+
+  // The top most plan always gets to do the trace log, if tracing is enabled.
   current_plan->DoTraceLog();
 
   // First query the stop info's ShouldStopSynchronous.  This handles

   return status;
 }
 
-void Thread::EnableTracer(bool value, bool single_stepping) {
-  int stack_size = m_plan_stack.size();
-  for (int i = 0; i < stack_size; i++) {
-    if (m_plan_stack[i]->GetThreadPlanTracer()) {
-      m_plan_stack[i]->GetThreadPlanTracer()->EnableTracing(value);
-      m_plan_stack[i]->GetThreadPlanTracer()->EnableSingleStep(single_stepping);
+void Thread::EnableSingleStepping() {
+  for (auto &thread_plan : m_plan_stack) {
+    auto thread_plan_tracer = thread_plan->GetThreadPlanTracer();
+    if (thread_plan_tracer) {
+      thread_plan_tracer->EnableSingleStepping();
+    }
+  }
+}
+
+void Thread::DisableSingleStepping() {
+  for (auto &thread_plan : m_plan_stack) {
+    auto thread_plan_tracer = thread_plan->GetThreadPlanTracer();
+    if (thread_plan_tracer) {
+      thread_plan_tracer->DisableSingleStepping();
+    }
+  }
+}
+
+void Thread::EnableTracing() {
+  for (auto &thread_plan : m_plan_stack) {
+    auto thread_plan_tracer = thread_plan->GetThreadPlanTracer();
+    if (thread_plan_tracer) {
+      thread_plan_tracer->EnableTracing();
+    }
+  }
+}
+
+void Thread::DisableTracing() {
+  for (auto &thread_plan : m_plan_stack) {
+    auto thread_plan_tracer = thread_plan->GetThreadPlanTracer();
+    if (thread_plan_tracer) {
+      thread_plan_tracer->DisableTracing();
     }
   }
 }
 
 void Thread::SetTracer(lldb::ThreadPlanTracerSP &tracer_sp) {
-  int stack_size = m_plan_stack.size();
-  for (int i = 0; i < stack_size; i++)
-    m_plan_stack[i]->SetThreadPlanTracer(tracer_sp);
+  for (auto &thread_plan : m_plan_stack) {
+    thread_plan->SetThreadPlanTracer(tracer_sp);
+  }
 }
 
 bool Thread::DiscardUserThreadPlansUpToIndex(uint32_t thread_index) {

lldb/source/Target/ThreadPlan.cpp

 
 lldb::StateType ThreadPlan::RunState() {
   if (m_tracer_sp && m_tracer_sp->TracingEnabled() &&
-      m_tracer_sp->SingleStepEnabled())
+      m_tracer_sp->SingleSteppingEnabled())
     return eStateStepping;
   else
     return GetPlanRunState();

lldb/source/Target/ThreadPlanBase.cpp

     : ThreadPlan(ThreadPlan::eKindBase, "base plan", thread, eVoteYes,
                  eVoteNoOpinion) {
 // Set the tracer to a default tracer.
-// FIXME: need to add a thread settings variable to pix various tracers...
+// FIXME: need to add a thread settings variable to pick various tracers...
 #define THREAD_PLAN_USE_ASSEMBLY_TRACER 1
 
 #ifdef THREAD_PLAN_USE_ASSEMBLY_TRACER

 #else
   ThreadPlanTracerSP new_tracer_sp(new ThreadPlanTracer(m_thread));
 #endif
-  new_tracer_sp->EnableTracing(m_thread.GetTraceEnabledState());
+  if (m_thread.GetTraceEnabledState()) {
+    new_tracer_sp->EnableTracing();
+  }
   SetThreadPlanTracer(new_tracer_sp);
   SetIsMasterPlan(true);
 }

lldb/source/Target/ThreadPlanTracer.cpp

 ThreadPlanTracer::ThreadPlanTracer(Thread &thread)
     : m_thread(thread), m_single_step(true), m_enabled(false), m_stream_sp() {}
 
+void ThreadPlanTracer::EnableTracing() {
+  if (!m_enabled) {
+    m_enabled = true;
+    TracingStarted();
+  }
+}
+
+void ThreadPlanTracer::DisableTracing() {
+  if (m_enabled) {
+    m_enabled = false;
+    TracingEnded();
+  }
+}
+
+bool ThreadPlanTracer::TracingEnabled() const {
+  return m_enabled;
+}
+
+void ThreadPlanTracer::EnableSingleStepping() {
+  m_single_step = true;
+}
+
+void ThreadPlanTracer::DisableSingleStepping() {
+  m_single_step = false;
+}
+
+bool ThreadPlanTracer::SingleSteppingEnabled() const {
+  return m_single_step;
+}
+
 Stream *ThreadPlanTracer::GetLogStream() {
   if (m_stream_sp)
     return m_stream_sp.get();

 
 ThreadPlanAssemblyTracer::~ThreadPlanAssemblyTracer() = default;
 
-void ThreadPlanAssemblyTracer::TracingStarted() {
-  RegisterContext *reg_ctx = m_thread.GetRegisterContext().get();
-
-  if (m_register_values.empty())
-    m_register_values.resize(reg_ctx->GetRegisterCount());
-}
+void ThreadPlanAssemblyTracer::TracingStarted() {}
 
 void ThreadPlanAssemblyTracer::TracingEnded() { m_register_values.clear(); }
 

     return;
 
   RegisterContext *reg_ctx = m_thread.GetRegisterContext().get();
+  if (m_register_values.empty())
+    m_register_values.resize(reg_ctx->GetRegisterCount());
 
   lldb::addr_t pc = reg_ctx->GetPC();
   ProcessSP process_sp(m_thread.GetProcess());