This is an archive of the discontinued LLVM Phabricator instance.

Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets
ClosedPublic

Authored by omjavaid on Jun 13 2016, 1:56 AM.

Details

Summary

This patch adds logic to make sure we can install watchpoints at 1,2 and 4 byte alligned addresses.

ptrace interface allows watchpoints at 8-byte alligned addresses. Therefor for all lower allignment levels we have to watch full 8-bytes.
We will ignore all irrelevant watchpoint trigger exceptions and will continue the target after stepping over the watchpoint instruction.

In worst case while watching a 8-byte array like byteArr[8] we may have to ignore 7 false watchpoint hits if we install watchpoint at the last byte in the array.

However overall advantage of this solution overwhelms this disadvantage. We now have all watchpoint tests passing on AArch64 Linux (HiKey board) and Arm64 Android (Nexus9).

Diff Detail

Event Timeline

omjavaid updated this revision to Diff 60493.Jun 13 2016, 1:56 AM
omjavaid retitled this revision from to Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets.
omjavaid updated this object.
omjavaid added reviewers: labath, clayborg.
omjavaid added a subscriber: lldb-commits.
clayborg accepted this revision.Jun 13 2016, 1:26 PM
clayborg edited edge metadata.

Does this patch handle being able to share an 8 byte watchpoint between two watchpoints? Lets say you have an 8 byte array named "a" and watch to watch a[0] and a[3] and a[7]. You should be able to allow the watchpoints to share the 1 watchpoint. This patch is fine as is, just something to think about for a future patch.

This revision is now accepted and ready to land.Jun 13 2016, 1:26 PM
labath requested changes to this revision.Jun 13 2016, 8:44 PM
labath edited edge metadata.

The overall change looks good, but please also add a test which specifically tests for watchpoints at unaligned addresses. Last time I checked, we all watchpoint tests were passing (at least on android) even without this patch, so it looks like our tests coverage is not sufficient here.

This revision now requires changes to proceed.Jun 13 2016, 8:44 PM

The overall change looks good, but please also add a test which specifically tests for watchpoints at unaligned addresses. Last time I checked, we all watchpoint tests were passing (at least on android) even without this patch, so it looks like our tests coverage is not sufficient here.

Patch causes no regressions and following tests fail without this patch on Nexus 9 and arm-linux hikey board:

No test currently fully tests the functionality supported by this patch. So I ll add a new test that tests different watchpoint sizes.

UNEXPECTED SUCCESS: test_watchpoint_command_can_disable_a_watchpoint_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_command_can_disable_a_watchpoint_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py)
UNEXPECTED SUCCESS: test_watchpoint_command_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_api_dwarf (python_api/watchpoint/condition/TestWatchpointConditionAPI.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_api_dwo (python_api/watchpoint/condition/TestWatchpointConditionAPI.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_dwarf (functionalities/watchpoint/watchpoint_commands/condition/TestWatchpointConditionCmd.py)
UNEXPECTED SUCCESS: test_watchpoint_cond_dwo (functionalities/watchpoint/watchpoint_commands/condition/TestWatchpointConditionCmd.py)
UNEXPECTED SUCCESS: test_with_python_api_dwarf (functionalities/watchpoint/watchpoint_events/TestWatchpointEvents.py)
UNEXPECTED SUCCESS: test_with_python_api_dwo (functionalities/watchpoint/watchpoint_events/TestWatchpointEvents.py)

Does this patch handle being able to share an 8 byte watchpoint between two watchpoints? Lets say you have an 8 byte array named "a" and watch to watch a[0] and a[3] and a[7]. You should be able to allow the watchpoints to share the 1 watchpoint. This patch is fine as is, just something to think about for a future patch.

This functionality may not work with current patch and may require more work. I will follow up with another patch after adding support for this kind of scenario.

omjavaid updated this revision to Diff 60942.Jun 15 2016, 6:48 PM
omjavaid edited edge metadata.

I have added a test cases that tests all possibilities supported by current configuration.

Tests pass on Nexus 9 and aarch64-linux-gnu (hikey board).

LGTM?

labath accepted this revision.Jun 15 2016, 9:07 PM
labath edited edge metadata.

Thanks for adding the test. I have some nits about some details of the test. If you agree with them, you can commit the updated version without additional review.

packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py
2 ↗(On Diff #60942)

Add: "Also make sure that the watchpoints work when the variables are not placed at 8-byte aligned addresses." or something to that effect. Otherwise, someone in the future will wonder why the test is so complicated.

Also, you don't seem to test 8-byte watchpoints (which is fine, but the comment is not correct then).

15 ↗(On Diff #60942)

How about adding NO_DEBUG_INFO_TESTCASE = True here? We are not testing debug info, so it should be enough to run the test once.

49 ↗(On Diff #60942)

this would a lot more natural if you renamed this into array_size or something and used 0-based indexes.

66 ↗(On Diff #60942)

range(array_size)

73 ↗(On Diff #60942)

str(i)

104 ↗(On Diff #60942)

I guess the intention here is to test both read and write watchpoints. If so, could you state that somewhere (in a comment at least, if it's possible to somehow verify the watchpoint type in code, even better).

packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/main.c
46 ↗(On Diff #60942)

shouldn't (typo)

This revision is now accepted and ready to land.Jun 15 2016, 9:07 PM
omjavaid marked 3 inline comments as done.Jun 16 2016, 9:37 AM
omjavaid added inline comments.
packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_size/TestWatchpointSizes.py
104 ↗(On Diff #60942)

We cant possibly figure out the watchpoint type read or write based on output string emitted when watchpoint is hit.

I have added a comment here which tells the user about why hit count should be updated here.

This revision was automatically updated to reflect the committed changes.