Page MenuHomePhabricator

[LLDB][MIPS] Fix TestWatchpointIter failure
ClosedPublic

Authored by nitesh.jain on Nov 24 2016, 11:16 PM.

Details

Summary

In case of MIPS, the watchpoint exception occurred before the associated instruction is executed. Whenever watchpoint is hit , we first preserve the watchpoint index before it is disable and then update the index after enabling it. Thus preserving watchpoint index.

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] Fix TestWatchpointIter failure.
nitesh.jain updated this object.
nitesh.jain added reviewers: clayborg, jingham, labath.

Although this patch fixes the test case in question but in theory EphemeralMode watchpoint enable/disable cycles should be independent of step-over watchpoint enable disable cycle.

On gdb-remote type targets we only update hardware_watch_id when a watchpoint is hit so when we clear hw_watch_id in Watchpoint::SetEnabled (source/Breakpoint/Watchpoint.cpp line 238) then information about last hardware watchpoint id that was hit also gets removed.

To update hardware watchpoint id on gdb-remote targets we ll have to send/recv an extra query packet asking about hardware watchpoint id corresponding to particular watchpoint we just enabled.

One possible solution could be to preserver the hardware watchpint id like way we are preserving stop info in this case. But there no harm in even not clearing watchpoint hardware id on disable rather leave that job for the target to decide if it can update hardware id upon every enable disable or just when watchpoint is hit.

labath resigned from this revision.Nov 25 2016, 2:10 AM
labath removed a reviewer: labath.

Jim will be a better person to review this. However, this feels like a hack to me.

jingham requested changes to this revision.Nov 28 2016, 11:42 AM
jingham edited edge metadata.

I agree with Pavel, it looks like you're using a side-effect of the Ephemeral mode to preserve something you would be better off preserving explicitly.

This revision now requires changes to proceed.Nov 28 2016, 11:42 AM
nitesh.jain updated this object.
nitesh.jain edited edge metadata.

Updated diff as per suggestion.

clayborg resigned from this revision.Dec 1 2016, 10:31 AM
clayborg removed a reviewer: clayborg.

I let Jim OK this patch.

This looks fine. Can you add a comment explaining why this is necessary, it isn't obvious right off the bat?

If this is fixing a test case, then just add a comment and this change is fine. If the fix is test-suite neutral, then please add a test case.

nitesh.jain updated this revision to Diff 80414.Dec 6 2016, 7:01 AM
nitesh.jain edited edge metadata.

Update diff as per suggestion

jingham accepted this revision.Dec 6 2016, 8:49 AM
jingham edited edge metadata.

Great, thanks!

This revision is now accepted and ready to land.Dec 6 2016, 8:49 AM
This revision was automatically updated to reflect the committed changes.