[Lldb-commits] [PATCH] D12184: [MIPS] Avoid breakpoint in delay slot

[Jim Ingham via lldb-commits]

jingham added a subscriber: jingham.
jingham requested changes to this revision.
jingham added a reviewer: jingham.
jingham added a comment.
This revision now requires changes to proceed.

I wish we had a better abstraction for machine-specific behaviors than ArchSpec, since it seems sad to put this machine specific a bit of business in Target.cpp, but I don't think you should have to tackle that to get this fix in.

However, I think you came in at too low a level by making the interface about delay slots.  Rather, there should be a function Target::GetBreakableLoadAddress (analogous to GetOpcodeLoadAddress & GetCallableLoadAddress) that encapsulated calculating the new breakpoint address and CreateBreakpoint should call that.  After all, maybe there will be other reasons why "you want to set a breakpoint on address X but we know Y is a better address" might happen and it would be better to hide the specific reason away as much as possible.

Also, since this will get called for every breakpoint address we set, please don't do any work in this function till you've decided you are on a machine that would require it.

Also, for you own sanity, I would suggest putting some logging (under the breakpoint channel) summarizing what you did to the address.  Someday, you'll get some weird bug report from somebody whose breakpoints are going wrong, and then you'll wish too late that you knew what you thought you were doing...


Repository:
  rL LLVM

http://reviews.llvm.org/D12184