[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

[Jason Molenda via lldb-commits]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D15708.44189.patch
Type: text/x-patch
Size: 14560 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160107/3aa5bae3/attachment-0001.bin>