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

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 9 inline comments as done.
jingham added a comment.

Addressed Pavel's review comments.



================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:113
+
+class ThreadPlanStackMap {
+public:
----------------
labath wrote:
> 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.
Mostly because if not here then all this logic goes in Process and that is already pretty huge.  This isn't a class that we will turn around and wire the methods through process, so I think it helps keep a little more organization.

The update task is trivial at this point because there aren't actually any plan stacks that outlive their threads, but it will grow as we decide when we want to do about this.  

I also plan (though I haven't done it yet) to move thread plan listing/deleting/etc to go through here.  Right now the "thread plan list" & Co. commands go through the thread list to individual threads, but that means you can't get at thread plan stacks for unreported threads, which doesn't seem right.  That belongs to the map of threads.


================
Comment at: lldb/source/Target/Process.cpp:1261
+    return llvm::make_error<llvm::StringError>(
+        llvm::formatv("Invalid option with value '{0}'.", tid),
+        llvm::inconvertibleErrorCode());
----------------
labath wrote:
> 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?
I originally had this as a pointer that returns NULL when not found, but I thought we were going towards Expected's for everything that might not produce a value.  I'm happier with it as just a pointer, so I put it back to that.


================
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();
----------------
labath wrote:
> 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?
Partly.  

This is actually a feature that was present but more or less buried in the old way of doing things.  The problem it addresses is:

1) You stop at the end of a step over
2) You run an expression, which also stops because it hit the breakpoint at _start
3) The user does "thread list".

What do you show as the stop reason for the thread?  The answer is "step over" not "expression completed".  This is not as simple as it seems, since the expression evaluation could push lots of plans on the stack.  For instance, the expression evaluation could hit a breakpoint, the user could step around a bit, run some more function, etc.  But when the expression evaluation eventually unwinds, we need to restore the original "step over" as the completed plan that gets reported as the "completed plan for this stop."

This part of the job (you also needed to update a few other things) used to be managed in the Thread separate from the rest of the SavedState.  That wasn't very clean, and since Threads might go away is also not safe.  I moved it into the saved state first because you have to if the Thread might go away, but also because it makes more sense where it is.


================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+  }
+}
----------------
labath wrote:
> 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.
I added a default, but what do you mean by "if we are to be fully pedantic, it isn't"?  It's an enum with 3 cases, all of which are listed.  How is that not fully covered?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75880