Page MenuHomePhabricator

[LLDB][MIPS] Fix TestStepOverBreakpoint.py failure
ClosedPublic

Authored by nitesh.jain on Apr 18 2017, 8:55 AM.

Details

Summary

In case of MIPS, we never set breakpoint in the delay slot. Hence while doing stepping, the delay slot instruction will be skipped.

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain created this revision.Apr 18 2017, 8:55 AM
nitesh.jain edited the summary of this revision. (Show Details)Apr 18 2017, 8:56 AM
clayborg resigned from this revision.Apr 18 2017, 9:06 AM
clayborg added a reviewer: jingham.

Jim, can you take a look at this and see if this could be fixed in a nicer way? I would prefer to not see anything related to delay slots in a test. Can we abstract this better? Maybe ask the SBInstruction() if you can set a breakpoint on it instead of speaking in delay slot terms? Or maybe we ask the SBInstructionList to count the number of instructions between two addresses? Maybe something like:

uint32_t SBInstructionList::GetInstructionCount(const SBAddress &start, const SBAddress &end);

Or improve the test so it doesn't rely on instruction count??

Update diff as per suggestion

jingham requested changes to this revision.Apr 26 2017, 11:04 AM

The substance seems fine.

I'm not sure I would guess what GetInstructionsCount with canSetBreakpoint == true would do without reading the code. You could fix this with a more explicit parameter name, but I can't think of a good name that's short enough not to be awful, something like: excludeInstructionsWhereYouCantSetBreakpoints would get it but yuck... Better to just add a header comment explaining what that parameter actually does.

It's also odd to have the SBInstructionList hold the logic determining whether you can set a breakpoint on an instruction. That should really be in SBInstruction, or even better should be in the lldb_private::Instruction class, since this seems like a generally useful bit of knowledge, and then routed through SBInstruction.

This revision now requires changes to proceed.Apr 26 2017, 11:04 AM
nitesh.jain edited edge metadata.

Update Diff as per suggestion

clayborg added inline comments.Apr 27 2017, 8:01 AM
include/lldb/API/SBInstructionList.h
36–38 ↗(On Diff #96891)

comment boundaries? Either extend the '-' or format within

jingham requested changes to this revision.Apr 27 2017, 10:33 AM

Couple tiny tweaks to the comment, and this is good to go.

include/lldb/API/SBInstructionList.h
36–38 ↗(On Diff #96891)

Also grammar. Like:

Returns the number of instructions between the start and end address. If canSetBreakpoint is true then the count will be the number of instructions on which a breakpoint can be set.

This revision now requires changes to proceed.Apr 27 2017, 10:33 AM
nitesh.jain updated this revision to Diff 97415.May 2 2017, 3:28 AM
nitesh.jain edited edge metadata.

Update diff as per suggestion.

jingham accepted this revision.May 2 2017, 9:01 AM

Looks good, thanks!

This revision is now accepted and ready to land.May 2 2017, 9:01 AM
This revision was automatically updated to reflect the committed changes.