Re: [Lldb-commits] [PATCH] D15708: 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 closed this revision. jasonmolenda added a comment. Landed in r257117. Sendinginclude/lldb/Target/Process.h Sendinginclude/lldb/Target/Thread.h Sendinginclude/lldb/Target/ThreadPlanStepOut.h Sendingsource/Target/Process.cpp Sendingsource/Target/Thread.cpp Sendingsource/Target/ThreadPlanShouldStopHere.cpp Sendingsource/Target/ThreadPlanStepOut.cpp Sendingsource/Target/ThreadPlanStepOverRange.cpp Transmitting file data Committed revision 257117. Repository: rL LLVM http://reviews.llvm.org/D15708 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15708: 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 added a comment.
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.
Repository:
rL LLVM
http://reviews.llvm.org/D15708
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D15708: 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 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(m_step_out_to_inline_plan_sp.get())->SetShouldStopHereCallbacks(nullptr, nullptr);
Re: [Lldb-commits] [PATCH] D15708: 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
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
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.
Comment at: include/lldb/Target/Process.h:3182-3183
@@ +3181,4 @@
+/// problems with the disassembly or getting the instructions,
+/// The original default_stop_addr will be returned.
+//--
+Address
"The" should be "the".
Comment at: include/lldb/Target/Thread.h:912-913
@@ -902,2 +911,4 @@
+ uint32_t frame_idx,
+ bool private_step_out);
//--
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?
Comment at: source/Target/Process.cpp:6578-6579
@@ +6577,4 @@
+{
+disassembler_sp->GetInstructionList().Clear(); // ThreadPlanStepRange
claims there is a retain cycle
+}
+
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?
Repository:
rL LLVM
http://reviews.llvm.org/D15708
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
