Improve step over performance

[jarin]

Summary

This patch improves step over performance for the case when we are stepping over a call with a next-branch-breakpoint (see https://reviews.llvm.org/D58678), and we encounter a stop during the call. Currently, this causes the thread plan to step-out each frame until it reaches the step-over range. This is a regression introduced by https://reviews.llvm.org/D58678 (which did improve other things!). Prior to that change, the step-over plan would always step-out just once.

With this patch, if we find ourselves stopped in a deeper stack frame and we already have a next branch breakpoint, we simply return from the step-over plan's ShouldStop handler without pushing the step out plan.

In my experiments this improved the time of stepping over a call that loads 12 dlls from 14s to 5s. This was in remote debugging scenario with 10ms RTT, the call in question was Vulkan initialization (vkCreateInstance), which loads various driver dlls. Loading those dlls must stop on the rendezvous breakpoint, causing the perf problem described above.

[jarin]

lldb/source/Target/ThreadPlanStepOverRange.cpp

176

We could do some more sanity checking here.

For example, we could ensure that the return address is still before the next branch breakpoint, or, ideally, record the beginning of the next-branch range and check the return address is in that range. However, I am not quite sure what this would protect us against (mucking with the stack, perhaps?) and what to do if the check fail - falling back to the old behavior does not really seem to solve anything because that introduces even more noise by pushing all the step-out plans.

[labath]

This seems reasonable, but let's wait for Greg and Jim's oppinions.

lldb/source/Target/ThreadPlanStepOverRange.cpp

176

Theoretically the extra stops forced by the step-outs could give us an opportunity to bail out if we detect that the inferior stack is changing unexpectedly, but I'm not sure how often that would actually result in something useful.

In general, all thread plans kind of have to assume that the process is using its stack in reasonable manner, and they can fail spectacularly if the process does some unexpected control flow or stack manipulations (e.g. a longjmp).

[jingham]

This looks fine to me.

The thread plans are self-healing, in the sense that they always check after every stop whether they are still relevant, (IsStale) and unship themselves if they aren't. For instance, if a step-over plan finds that the stack has unwound past its target frame, it will pop itself.

The way to actively handle exceptions and longjmp and the like currently is to put breakpoints on them by hand, and then you will get to see the state before the unwinding. I haven't done that automatically in the thread plans because I don't have a way of knowing whether the exception throw will unwind past the last stack frame of interest to the thread plans, and it would be annoying to have stepping stop all the time to tell you about exceptions you don't care about. It's better to let the users do this by hand.

One missing piece of lldb functionality is the ability to analyze these exceptional stack events and predict their results at the throw point. This should be possible for C++ at least using the Itanium ABI for exception throwing, since you would just be replaying the scan pass of the exception handling. Once that is in place, then you could always stop on exceptions, but ignore all the ones that would get caught before the first frame of interest, and stop for ones that would throw past it.

gdb used to handle longjmp by always breaking on setjmp and reading out the target of the jump. We could also do that, but TTTT, I have had people complain about exceptions losing control of stepping, but I haven't had anybody bring up setjmp/longjmp for quite a while.

[jarin]

Thanks for all the clarifications, this is very useful. I have always wanted to learn about thread plans, and this was a nice opportunity to do that. The extra background from you guys is a nice bonus.

Regarding the patch itself, is there anything preventing an LGTM?

[jarin]

Rebase

[clayborg]

Jim is the one that really needs to mark this as accepted as the thread plans are one of his many areas of expertise.

[jarin]

Thanks Greg, I will wait for Jim's comment.

I also see that build-bot is not happy about my patch. Clang-tidy somewhat mysteriously fails on missing lldb/Target/ThreadPlanStepOverRange.h, which I did not touch at all (neither the fail nor the #include). Any idea what that is about?

[labath]

In D76216#1930689, @jarin wrote:

I also see that build-bot is not happy about my patch. Clang-tidy somewhat mysteriously fails on missing lldb/Target/ThreadPlanStepOverRange.h, which I did not touch at all (neither the fail nor the #include). Any idea what that is about?

These pre-merge tests are a fairly new thing, and they don't fully work for lldb currently. Just ignore that for now...

[jingham]

Sorry, forgot to select the action...

[jarin]

Pavel or Jim, could you possibly land this for me?

lldb/source/Target/ThreadPlanStepOverRange.cpp

for (uint32_t i = 1;; ++i) {
         // We can't unwind the next frame we should just get out of here &
         // stop...
         break;
       }
 
       const SymbolContext &older_context =
           older_frame_sp->GetSymbolContext(eSymbolContextEverything);
       if (IsEquivalentContext(older_context)) {
+        // If we have the  next-branch-breakpoint in the range, we can just
+        // rely on that breakpoint to trigger once we return to the range.
+        if (m_next_branch_bp_sp)
+          return false;
         new_plan_sp = m_thread.QueueThreadPlanForStepOutNoShouldStop(
             false, nullptr, true, stop_others, eVoteNo, eVoteNoOpinion, 0,
             m_status, true);
         break;
       } else {
         new_plan_sp = m_thread.QueueThreadPlanForStepThrough(
             m_stack_id, false, stop_others, m_status);
         // If we found a way through, then we should stop recursing.