Page MenuHomePhabricator

Fix AArch64 watchpoint handlers in NativeRegisterContextLinux_arm64
ClosedPublic

Authored by omjavaid on Aug 10 2015, 5:16 AM.

Details

Reviewers
tberghammer
Summary

There were some bugs that needed to be fixed in watchpoint handling code on arm64.

Watchpoints were being written to all watchpoint registers and cache was not being maintained correctly.

This patch fixes up all these issues. Watchpoints now work on arm64 except that there is race condition which is not allowing lldb to step out of a watchpoint hit before we can continue again so inferior gets stuck at that point. Manually disabling the watchpoint then issuing a step and then enabling the watchpoint again fixes shows that watchpoints are being installed and removed correctly.

Diff Detail

Event Timeline

omjavaid updated this revision to Diff 31656.Aug 10 2015, 5:16 AM
omjavaid retitled this revision from to Fix AArch64 watchpoint handlers in NativeRegisterContextLinux_arm64.
omjavaid updated this object.
omjavaid added reviewers: tberghammer, clayborg.
omjavaid added a subscriber: lldb-commits.
tberghammer edited edge metadata.Aug 10 2015, 6:43 AM

Generally looks good to me. I am happy to push the 2 cleanup change to a separate CL but please check that the read/write flag calculation is correct.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
514–519

You use this pattern several times in this class. It would be nice if you can move it out to a separate function to avoid code duplication.

537

I think this expression is incorrect. Based on http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500d/CEGDIEBB.html I don't see why you need this at all and if watch_flags==0x3 then it will set watch_flags to 0x7 what looks wrong for me.

753

The 0/1 for the type isn't too descriptive. Please use an enum here (or the NT_ARM_HW_WATCH/NT_ARM_HW_BREAK constants)

clayborg resigned from this revision.Aug 10 2015, 10:03 AM
clayborg removed a reviewer: clayborg.

I will defer to Tamas Berghammer since I have no expertise is arm64 watchpoints.

omjavaid updated this revision to Diff 31934.Aug 12 2015, 6:11 AM
omjavaid edited edge metadata.

I have updated this patch after incorporating suggestions.

Is it good for commit now?

tberghammer accepted this revision.Aug 12 2015, 6:16 AM
tberghammer edited edge metadata.

Looks good. Thank you for fixing it.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
755

Please use eDREGTypeWATCH instead of '0'

This revision is now accepted and ready to land.Aug 12 2015, 6:16 AM
omjavaid closed this revision.Feb 9 2016, 12:07 AM