[Lldb-commits] [PATCH] D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

As we've seen, I don't know much about thread plans so I don't have much to say on the higher-level aspects of this patch.

But I see a bunch of opportunities to make this more consistent with llvm code style.



================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:9
+
+#ifndef liblldb_ThreadPlanStack_h_
+#define liblldb_ThreadPlanStack_h_
----------------
the include guards were recently normalized to LLDB_TARGET_THREADPLANSTACK_H


================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:35
+public:
+  ThreadPlanStack(Thread &thread){};
+  ~ThreadPlanStack() {}
----------------
superfluous semicolon


================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:113
+
+class ThreadPlanStackMap {
+public:
----------------
It's not clear to me what is the value of this class. Most of it is just about wrapping the underlying container in a slightly different way. If the ThreadDestroyed method can be called in the ThreadPlanStack destructor (which appears to be possible and would be a nice clean up regardless) then RemoveTID would boil down to `container.erase(tid)`. At that point the only non-trivial piece of functionality is the `Update` function (which this patch doesn't even implement), which could just as well be implemented outside of the class.


================
Comment at: lldb/source/Target/Process.cpp:1261
+    return llvm::make_error<llvm::StringError>(
+        llvm::formatv("Invalid option with value '{0}'.", tid),
+        llvm::inconvertibleErrorCode());
----------------
This error message doesn't really make sense for a function like this. What option is it talking about?

As the only reason this can fail is because there are no plans for the given tid, maybe you don't actually need the Expected, and can signal that with a simple nullptr?


================
Comment at: lldb/source/Target/Thread.cpp:505
   saved_state.current_inlined_depth = GetCurrentInlinedDepth();
-  saved_state.m_completed_plan_stack = m_completed_plan_stack;
+  saved_state.m_completed_plan_checkpoint =
+      GetPlans().CheckpointCompletedPlans();
----------------
What's the reason for the introduction of the "checkpoint" indirection? Does it have something to do with needing to update the checkpointed plans when the thread disappears?


================
Comment at: lldb/source/Target/Thread.cpp:1054-1058
+  llvm::Expected<ThreadPlanStack *> plans =
+      GetProcess()->FindThreadPlans(GetID());
+  if (!plans)
+    llvm_unreachable("Thread left with an empty plan stack");
+  return **plans;
----------------
maybe `return cantFail(GetProcess()->FindThreadPlans(GetID()));`


================
Comment at: lldb/source/Target/Thread.cpp:1061
+
 void Thread::PushPlan(ThreadPlanSP &thread_plan_sp) {
+  if (!thread_plan_sp)
----------------
You don't modify the callers shared pointer => `const ThreadPlanSP &` or a `ThreadPlanSP` with a PushPlan(std::move(thread_plan_sp))`.


================
Comment at: lldb/source/Target/Thread.cpp:1062-1063
 void Thread::PushPlan(ThreadPlanSP &thread_plan_sp) {
-  if (thread_plan_sp) {
-    // If the thread plan doesn't already have a tracer, give it its parent's
-    // tracer:
-    if (!thread_plan_sp->GetThreadPlanTracer()) {
-      assert(!m_plan_stack.empty());
-      thread_plan_sp->SetThreadPlanTracer(
-          m_plan_stack.back()->GetThreadPlanTracer());
-    }
-    m_plan_stack.push_back(thread_plan_sp);
+  if (!thread_plan_sp)
+    llvm_unreachable("Don't push an empty thread plan.");
 
----------------
assert


================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:72-73
+  auto result = m_completed_plan_store.find(checkpoint);
+  if (result == m_completed_plan_store.end())
+    llvm_unreachable("Asked for a checkpoint that didn't exist");
+  m_completed_plans.swap((*result).second);
----------------
The usual way to write that is `assert(result == m_completed_plan_store.end() && "Asked for a checkpoint that didn't exist")`. llvm_unreachable is used where an existing control flow path needs to be declared invalid. We don't introduce new control flow just for the sake of using it.


================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:84-90
+  for (auto plan : m_plans)
+    plan->ThreadDestroyed();
+
+  for (auto plan : m_discarded_plans)
+    plan->ThreadDestroyed();
+
+  for (auto plan : m_completed_plans)
----------------
Please put the actual type (ThreadPlanSP ?) instead of `auto`.


================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:126-127
+  // The first plan has to be a base plan:
+  if (m_plans.size() == 0 && !new_plan_sp->IsBasePlan())
+    llvm_unreachable("Zeroth plan must be a base plan");
+
----------------
assert instead of branch-to-unreachable


================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+  }
+}
----------------
This is the usual place where one would add a llvm_unreachable. Some compilers do not consider the above switch to be fully covered (and if we are to be fully pedantic, it isn't) and will complain about falling off of the end of a non-void function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75880/new/

https://reviews.llvm.org/D75880