Advance the return-address breakpoint location to the end of the next source line, or the next branching instruction, when stepping over a func call

[jasonmolenda]

Summary

When lldb is instruction stepping (or fast-stepping / instruction stepping) over a source line, and it instruction steps into a function call, and the function is not "interesting" to the user, lldb sets a breakpoint on the return address and continues the process until that breakpoint is hit on the current thread.

lldb has a fast-stepping approach to avoid stopping at every instruction in a source line range - it only stops on instructions that can branch / return / call functions.

So we have one extra stop when nexting over a function call. We step into the uninteresting-function, put a breakpoint on the return address, continue, hit the breakpoint on the return address and then we fast-step to the next branch instruction. That was four extra gdb-remote protocol packets for every function call in a source line.

This patch advances the stop breakpoint to the first branching instruction after we return, or to the end of the source line.

It passes the testsuite. I'll be doing more by-hand testing in the following week or two when I have time, but it's a straightforward change, it shouldn't cause problems unless I've missed something. I added the new Process::AdvanceAddressToNextBranchInstruction() method but I'm only calling it from ThreadPlanStepOut::ThreadPlanStepOut. I thought I'd be able to unify this new function with the code in ThreadPlanStepRange::SetNextBranchBreakpoint but I don't think the few lines I could remove from ThreadPlanStepRange::SetNextBranchBreakpoint would be worth the change. Jim might disagree with that.

The one point where this would be incorrect is a command like "finish" which displays the return value after the function exits. On an architecture where the return values are passed in volatile registers (x86, arm), that register may be overwritten after the function return so we must stop on the return address.

[jingham]

I don't so much mind that you couldn't reuse AdvanceAddressToNextBranchInstruction, we wouldn't be using the "get the disassembly" part of it, which is the biggest bit, since that's already done in GetInstructionsForAddress which also handles the cache of disassembled instructions. If the disassembly part of this stepping ever shows up as a big time sink, then we can move the cache of instruction fragments to the process, and get it from there.

The only bit I'm concerned with is why you needed to call Clear on the disassembler's instruction list. That seems odd to me.

include/lldb/Target/Process.h

3182–3183

"The" should be "the".

include/lldb/Target/Thread.h

912–913

Can you call this something that says what it does, like "continue_to_next_branch"? private_step_out seems too generic.

Also, should this be defaulted to false?

source/Target/Process.cpp

6578–6579

I don't understand this comment, and this seems like an odd thing to have to do. The disassembler is going to get destroyed when we leave this function. Why should we have to clear its instruction list manually?

[jasonmolenda]

Thanks for the comments. As for calling Clear() on the instruction list manually, I was aping the dtor on ThreadPlanStepRange which reads,

// FIXME: The DisassemblerLLVMC has a reference cycle and won't go away if it has any active instructions.
// I'll fix that but for now, just clear the list and it will go away nicely.
for (size_t i = 0; i < num_instruction_ranges; i++)
{
    if (m_instruction_ranges[i])
        m_instruction_ranges[i]->GetInstructionList().Clear();
}

I should probably check that this is the case before I copy the hack.

I'll see if I can consolidate the disassembly into one big of shared code.

[jasonmolenda]

Updated to address Jim's first round of comments.

1. fix typeo in comment

2. update change argument name from "private_step_out" to "continue_to_next_branch"

3. Change continue_to_next_branch argument to have a default value of false (I'm always a little reluctant to use these default arguments because it can make it more difficult to understand which method is being called with overloaded methods, or if when it gets out of hand, which arguments are going where -- but I'm sure it won't be a problem in this case)

4. The call to InstructionList::Clear is necessary, letting the Disassembler dtor execute is not sufficient (I did a quick test to make sure the comment was still right). This comment:

// The DisassemblerLLVMC has a reference cycle and won't go away if it has any active instructions.

appears in six places around the codebase already (one of them being in the ThreadPlanStepRange dtor). I updated to use the same comment.

5. The code duplication in ThreadPlanStepRange::GetInstructionsForAddress and Process::AdvanceAddressToNextBranchInstruction (where we disassemble an address range and identify the next branching instruction) is not great, but when you strip out the correctness checks on the inputs, the total of the duplication is
   
   ExecutionContext exe_ctx (this); const char *plugin_name = nullptr; const char *flavor = nullptr; const bool prefer_file_cache = true; disassembler_sp = Disassembler::DisassembleRange(target.GetArchitecture(), plugin_name, flavor, exe_ctx, range_bounds, prefer_file_cache);

so I just left them as separate code. (over in ThreadPlanStepRange, the method to get the InstructionList is in GetInstructionsForAddress and then the logic to get the next non-branching instruction is in SetNextBranchBreakpoint. In Process, I'm doing both in one method). ThreadPlanStepRange saves the Disassemblers (one per AddressRange for the multiple parts of a source line) in a vector; I'm not caching anything over in Process because we're unlikely to use the same InstructionList again in this same context.

I can go either way on this one. It was enough of a difference in behavior, and a small enough bit of code, that I didn't try to unify them, but I'm not wedded to that.

[jingham]

I agree default arguments should be used with care. But in cases where some argument should generally be used one way and only have another value when used for special purposes by them as knows what they are doing, then the default argument expresses this.

Can you file a bug about the need to clear the instruction list. That's just a little time bomb we're leaving around. You don't need to fix it for this patch, but we should remind ourselves to fix it when we have a spare moment.

The code duplication doesn't bother me either, it is not significant.

[jasonmolenda]

Landed in r257117.

Sending include/lldb/Target/Process.h
Sending include/lldb/Target/Thread.h
Sending include/lldb/Target/ThreadPlanStepOut.h
Sending source/Target/Process.cpp
Sending source/Target/Thread.cpp
Sending source/Target/ThreadPlanShouldStopHere.cpp
Sending source/Target/ThreadPlanStepOut.cpp
Sending source/Target/ThreadPlanStepOverRange.cpp
Transmitting file data ........
Committed revision 257117.

include/lldb/Target/Process.h

     void
     ResetImageToken(size_t token);
 
+    //------------------------------------------------------------------
+    /// Find the next branch instruction to set a breakpoint on
+    ///
+    /// When instruction stepping through a source line, instead of 
+    /// stepping through each instruction, we can put a breakpoint on
+    /// the next branch instruction (within the range of instructions
+    /// we are stepping through) and continue the process to there,
+    /// yielding significant performance benefits over instruction
+    /// stepping.  
+    ///
+    /// @param[in] default_stop_addr
+    ///     The address of the instruction where lldb would put a 
+    ///     breakpoint normally.
+    ///
+    /// @param[in] range_bounds
+    ///     The range which the breakpoint must be contained within.
+    ///     Typically a source line.
+    ///
+    /// @param[in] insn_list
+    ///     A pointer to an InstructionList for the instructions of
+    ///     the range_bounds AddressRange.  The caller may be able
+    ///     to cache the InstructionList and re-use it across multiple
+    ///     invocations.  If this is NULL, this method will disassemble
+    ///     the range_bounds AddressRange instructions and delete them
+    ///     at the end of the method.
+    ///
+    /// @return
+    ///     The address of the next branch instruction, or the end of
+    ///     the range provided in range_bounds.  If there are any
+    ///     problems with the disassembly or getting the instructions,
+    ///     The original default_stop_addr will be returned.
+    //------------------------------------------------------------------
+    Address
+    AdvanceAddressToNextBranchInstruction (Address default_stop_addr, 
+                                           AddressRange range_bounds,
+                                           InstructionList *insn_list);
+
 protected:
     void
     SetState (lldb::EventSP &event_sp);

include/lldb/Target/Thread.h

     /// @param[in] run_vote
     ///    See standard meanings for the stop & run votes in ThreadPlan.h.
     ///
+    /// @param[in] private_step_out
+    ///    Normally this will enqueue a plan that will put a breakpoint on the return address and continue
+    ///    to there.  If private_step_out is true, this is an operation not involving the user -- e.g. stepping
+    ///    "next" in a source line and we instruction stepped into another function -- so instead of putting
+    ///    a breakpoint on the return address, advance the breakpoint to the end of the source line that is
+    ///    doing the call, or until the next flow control instruction.
+    ///    If the return value from the function call is to be retrieved / displayed to the user, you must stop
+    ///    on the return address.  The return value may be stored in volatile registers which are overwritten
+    ///    before the next branch instruction.
+    ///
     /// @return
     ///     A shared pointer to the newly queued thread plan, or nullptr if the plan could not be queued.
     //------------------------------------------------------------------

                                            bool stop_other_threads,
                                            Vote stop_vote, // = eVoteYes,
                                            Vote run_vote, // = eVoteNoOpinion);
-                                           uint32_t frame_idx);
+                                           uint32_t frame_idx,
+                                           bool private_step_out);
 
     //------------------------------------------------------------------
     /// Gets the plan used to step through the code that steps from a function

include/lldb/Target/ThreadPlanStepOut.h

                        Vote stop_vote,
                        Vote run_vote,
                        uint32_t frame_idx,
-                       LazyBool step_out_avoids_code_without_debug_info);
+                       LazyBool step_out_avoids_code_without_debug_info,
+                       bool private_step_out);
 
     ~ThreadPlanStepOut() override;
 

source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp

                                            m_stop_others,
                                            eVoteNoOpinion,
                                            eVoteNoOpinion,
-                                           frame_idx);
+                                           frame_idx,
+                                           false);
             m_run_to_sp->SetPrivate(true);
             return false;
         }

source/Target/Process.cpp

     if (token < m_image_tokens.size())
         m_image_tokens[token] = LLDB_INVALID_IMAGE_TOKEN;
 }
+
+Address
+Process::AdvanceAddressToNextBranchInstruction (Address default_stop_addr, AddressRange range_bounds, 
+                                               InstructionList *insn_list)
+{
+    Target &target = GetTarget();
+    DisassemblerSP disassembler_sp;
+
+    Address retval = default_stop_addr;
+
+    if (target.GetUseFastStepping() == false)
+        return retval;
+    if (default_stop_addr.IsValid() == false)
+        return retval;
+
+
+    if (insn_list == NULL)
+    {
+        ExecutionContext exe_ctx (this);
+        const char *plugin_name = nullptr;
+        const char *flavor = nullptr;
+        const bool prefer_file_cache = true;
+        disassembler_sp = Disassembler::DisassembleRange(target.GetArchitecture(),
+                                                         plugin_name,
+                                                         flavor,
+                                                         exe_ctx,
+                                                         range_bounds,
+                                                         prefer_file_cache);
+        if (disassembler_sp.get())
+            insn_list = &disassembler_sp->GetInstructionList();
+    }
+
+    if (insn_list == NULL)
+    {
+        return retval;
+    }
+
+    size_t insn_offset = insn_list->GetIndexOfInstructionAtAddress(default_stop_addr);
+    if (insn_offset == UINT32_MAX)
+    {
+        return retval;
+    }
+
+    uint32_t branch_index = insn_list->GetIndexOfNextBranchInstruction (insn_offset, target);
+    if (branch_index == UINT32_MAX)
+    {
+        return retval;
+    }
+
+    if (branch_index > insn_offset)
+    {
+        Address next_branch_insn_address = insn_list->GetInstructionAtIndex (branch_index)->GetAddress();
+        if (next_branch_insn_address.IsValid() && range_bounds.ContainsFileAddress (next_branch_insn_address))
+        {
+            retval = next_branch_insn_address;
+        }
+    }
+
+    if (disassembler_sp.get())
+    {
+        disassembler_sp->GetInstructionList().Clear(); // ThreadPlanStepRange claims there is a retain cycle
+    }
+
+    return retval;
+}

source/Target/Thread.cpp

                                                         stop_vote, 
                                                         run_vote, 
                                                         frame_idx,
-                                                        step_out_avoids_code_withoug_debug_info));
+                                                        step_out_avoids_code_withoug_debug_info, false));
     
     if (thread_plan_sp->ValidatePlan(nullptr))
     {

                                               bool stop_other_threads,
                                               Vote stop_vote,
                                               Vote run_vote,
-                                              uint32_t frame_idx)
+                                              uint32_t frame_idx,
+                                              bool private_step_out)
 {
     ThreadPlanSP thread_plan_sp(new ThreadPlanStepOut (*this,
                                                         addr_context, 

                                                         stop_vote, 
                                                         run_vote, 
                                                         frame_idx,
-                                                        eLazyBoolNo));
+                                                        eLazyBoolNo, private_step_out));
 
     ThreadPlanStepOut *new_plan = static_cast<ThreadPlanStepOut *>(thread_plan_sp.get());
     new_plan->ClearShouldStopHereCallbacks();

source/Target/ThreadPlanShouldStopHere.cpp

                                                                                          stop_others,
                                                                                          eVoteNo,
                                                                                          eVoteNoOpinion,
-                                                                                         frame_index);
+                                                                                         frame_index,
+                                                                                         true);
     return return_plan_sp;
 }
 

source/Target/ThreadPlanStepInstruction.cpp

                                                                    stop_others,
                                                                    eVoteNo,
                                                                    eVoteNoOpinion,
-                                                                   0);
+                                                                   0,
+                                                                   false);
                     return false;
                 }
                 else

source/Target/ThreadPlanStepOut.cpp

 #include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/Function.h"
+#include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/Type.h"
 #include "lldb/Target/ABI.h"
 #include "lldb/Target/Process.h"

     Vote stop_vote,
     Vote run_vote,
     uint32_t frame_idx,
-    LazyBool step_out_avoids_code_without_debug_info
+    LazyBool step_out_avoids_code_without_debug_info,
+    bool private_step_out
 ) :
     ThreadPlan (ThreadPlan::eKindStepOut, "Step out", thread, stop_vote, run_vote),
     ThreadPlanShouldStopHere (this),

                                                                      eVoteNoOpinion,
                                                                      eVoteNoOpinion,
                                                                      frame_idx - 1,
-                                                                     eLazyBoolNo));
+                                                                     eLazyBoolNo,
+                                                                     private_step_out));
             static_cast<ThreadPlanStepOut *>(m_step_out_to_inline_plan_sp.get())->SetShouldStopHereCallbacks(nullptr, nullptr);
             m_step_out_to_inline_plan_sp->SetPrivate(true);
         }

         // Find the return address and set a breakpoint there:
         // FIXME - can we do this more securely if we know first_insn?
 
-        m_return_addr = return_frame_sp->GetFrameCodeAddress().GetLoadAddress(&m_thread.GetProcess()->GetTarget());
+        Address return_address (return_frame_sp->GetFrameCodeAddress());
+        if (private_step_out)
+        {
+            SymbolContext return_address_sc;
+            AddressRange range;
+            Address return_address_decr_pc = return_address;
+            if (return_address_decr_pc.GetOffset() > 0)
+                return_address_decr_pc.Slide (-1);
+
+            return_address_decr_pc.CalculateSymbolContext (&return_address_sc, lldb::eSymbolContextLineEntry | lldb::eSymbolContextFunction | lldb::eSymbolContextSymbol);
+            if (return_address_sc.line_entry.IsValid())
+            {
+                range = return_address_sc.line_entry.GetSameLineContiguousAddressRange();
+            } 
+            else if (return_address_sc.function)
+            {
+                range = return_address_sc.function->GetAddressRange();
+            } 
+            else if (return_address_sc.symbol && return_address_sc.symbol->GetByteSizeIsValid())
+            {
+                range.GetBaseAddress() = return_address_sc.symbol->GetAddress();
+                range.SetByteSize (return_address_sc.symbol->GetByteSize());
+            }
+            if (range.GetByteSize() > 0)
+            {
+                return_address = m_thread.GetProcess()->AdvanceAddressToNextBranchInstruction (return_address, range, nullptr);
+            }
+        }
+        m_return_addr = return_address.GetLoadAddress(&m_thread.GetProcess()->GetTarget());
         
         if (m_return_addr == LLDB_INVALID_ADDRESS)
             return;

source/Target/ThreadPlanStepOverRange.cpp

                                                                              stop_others,
                                                                              eVoteNo,
                                                                              eVoteNoOpinion,
-                                                                             0);
+                                                                             0,
+                                                                             true);
                 break;
             }
             else