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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to