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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
