jasonmolenda updated this revision to Diff 44189.
jasonmolenda added a comment.
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.
Repository:
rL LLVM
http://reviews.llvm.org/D15708
Files:
include/lldb/Target/Process.h
include/lldb/Target/Thread.h
include/lldb/Target/ThreadPlanStepOut.h
source/Target/Process.cpp
source/Target/Thread.cpp
source/Target/ThreadPlanShouldStopHere.cpp
source/Target/ThreadPlanStepOut.cpp
source/Target/ThreadPlanStepOverRange.cpp
Index: source/Target/ThreadPlanStepOverRange.cpp
===================================================================
--- source/Target/ThreadPlanStepOverRange.cpp
+++ source/Target/ThreadPlanStepOverRange.cpp
@@ -185,7 +185,8 @@
stop_others,
eVoteNo,
eVoteNoOpinion,
- 0);
+ 0,
+ true);
break;
}
else
Index: source/Target/ThreadPlanStepOut.cpp
===================================================================
--- source/Target/ThreadPlanStepOut.cpp
+++ source/Target/ThreadPlanStepOut.cpp
@@ -18,6 +18,7 @@
#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"
@@ -44,7 +45,8 @@
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 continue_to_next_branch
) :
ThreadPlan (ThreadPlan::eKindStepOut, "Step out", thread, stop_vote, run_vote),
ThreadPlanShouldStopHere (this),
@@ -86,7 +88,8 @@
eVoteNoOpinion,
eVoteNoOpinion,
frame_idx - 1,
- eLazyBoolNo));
+ eLazyBoolNo,
+ continue_to_next_branch));
static_cast<ThreadPlanStepOut *>(m_step_out_to_inline_plan_sp.get())->SetShouldStopHereCallbacks(nullptr, nullptr);
m_step_out_to_inline_plan_sp->SetPrivate(true);
}
@@ -101,7 +104,35 @@
// 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 (continue_to_next_branch)
+ {
+ 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;
Index: source/Target/ThreadPlanShouldStopHere.cpp
===================================================================
--- source/Target/ThreadPlanShouldStopHere.cpp
+++ source/Target/ThreadPlanShouldStopHere.cpp
@@ -135,7 +135,8 @@
stop_others,
eVoteNo,
eVoteNoOpinion,
- frame_index);
+ frame_index,
+ true);
return return_plan_sp;
}
Index: source/Target/Thread.cpp
===================================================================
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -1591,7 +1591,7 @@
Vote stop_vote,
Vote run_vote,
uint32_t frame_idx,
- LazyBool step_out_avoids_code_withoug_debug_info)
+ LazyBool step_out_avoids_code_without_debug_info)
{
ThreadPlanSP thread_plan_sp (new ThreadPlanStepOut (*this,
addr_context,
@@ -1600,7 +1600,7 @@
stop_vote,
run_vote,
frame_idx,
- step_out_avoids_code_withoug_debug_info));
+ step_out_avoids_code_without_debug_info));
if (thread_plan_sp->ValidatePlan(nullptr))
{
@@ -1620,7 +1620,8 @@
bool stop_other_threads,
Vote stop_vote,
Vote run_vote,
- uint32_t frame_idx)
+ uint32_t frame_idx,
+ bool continue_to_next_branch)
{
ThreadPlanSP thread_plan_sp(new ThreadPlanStepOut (*this,
addr_context,
@@ -1629,7 +1630,8 @@
stop_vote,
run_vote,
frame_idx,
- eLazyBoolNo));
+ eLazyBoolNo,
+ continue_to_next_branch));
ThreadPlanStepOut *new_plan = static_cast<ThreadPlanStepOut *>(thread_plan_sp.get());
new_plan->ClearShouldStopHereCallbacks();
Index: source/Target/Process.cpp
===================================================================
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -6515,3 +6515,69 @@
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())
+ {
+ // FIXME: The DisassemblerLLVMC has a reference cycle and won't go away if it has any active instructions.
+ disassembler_sp->GetInstructionList().Clear();
+ }
+
+ return retval;
+}
Index: include/lldb/Target/ThreadPlanStepOut.h
===================================================================
--- include/lldb/Target/ThreadPlanStepOut.h
+++ include/lldb/Target/ThreadPlanStepOut.h
@@ -31,7 +31,8 @@
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 continue_to_next_branch = false);
~ThreadPlanStepOut() override;
Index: include/lldb/Target/Thread.h
===================================================================
--- include/lldb/Target/Thread.h
+++ include/lldb/Target/Thread.h
@@ -888,6 +888,16 @@
/// @param[in] run_vote
/// See standard meanings for the stop & run votes in ThreadPlan.h.
///
+ /// @param[in] continue_to_next_branch
+ /// Normally this will enqueue a plan that will put a breakpoint on the return address and continue
+ /// to there. If continue_to_next_branch 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.
//------------------------------------------------------------------
@@ -898,7 +908,8 @@
bool stop_other_threads,
Vote stop_vote, // = eVoteYes,
Vote run_vote, // = eVoteNoOpinion);
- uint32_t frame_idx);
+ uint32_t frame_idx,
+ bool continue_to_next_branch = false);
//------------------------------------------------------------------
/// Gets the plan used to step through the code that steps from a function
Index: include/lldb/Target/Process.h
===================================================================
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3149,6 +3149,43 @@
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);
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
