This is an archive of the discontinued LLVM Phabricator instance.

Adds support for ARM hardware watchpoints
ClosedPublic

Authored by omjavaid on May 12 2015, 6:01 AM.

Details

Summary

This patch adds support for setting hardware watchpoints on arm.

There are still some unfixed issues which results in lldb not hitting hardware watchpoints set/clear functions in NativeLinuxRegisterContextLinus_arm.

I am presenting this code for review meanwhile I ll fix the reminaing issues.

I have added hardware breakpoint set/clear functions for future use.

Diff Detail

Event Timeline

omjavaid updated this revision to Diff 25583.May 12 2015, 6:01 AM
omjavaid retitled this revision from to Adds support for ARM hardware watchpoints .
omjavaid updated this object.
omjavaid edited the test plan for this revision. (Show Details)
omjavaid added a subscriber: Unknown Object (MLST).
omjavaid updated this revision to Diff 25590.May 12 2015, 7:16 AM

Adding full context diff.

clayborg accepted this revision.May 12 2015, 10:39 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.May 12 2015, 10:39 AM
tberghammer requested changes to this revision.May 14 2015, 10:18 AM
tberghammer edited edge metadata.

Generally looks good but please fix the broken condition (see inline)

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
861–862

This condition is always true. I am not sure what you try to check but the current condition is definitely broken.

source/Plugins/Process/Linux/ProcessMonitor.cpp
606

Please change it to an enum or at least add a comment for what the values mean.

This revision now requires changes to proceed.May 14 2015, 10:18 AM
omjavaid updated this revision to Diff 26669.May 28 2015, 3:21 AM
omjavaid edited edge metadata.

Updated patch with recent changes incorporated.

I tried to apply this patch to TOT but it wasn't compiling because NativeProcessLinux::ReadHardwareDebugInfo (and a few similar function) is missing. You should create an operation for it in NativeRegsiterContextLinux_arm and execute it through NativeProcessLinux::DoOperation.

Other note is that you made changes in Linux/ProcessMonitor.{h,cpp}. I have no problem with it, but it isn't used at the moment and hopefully it will be deleted within a few month as that one is only used when debugging an application without lldb-server what is already deprecated on Linux.

tberghammer requested changes to this revision.May 29 2015, 2:54 AM
tberghammer edited edge metadata.
This revision now requires changes to proceed.May 29 2015, 2:54 AM
omjavaid updated this revision to Diff 26884.Jun 1 2015, 3:13 AM
omjavaid edited edge metadata.

Made corrections in previous patch.

@tamas
Kindly check build now. Thanks!

It is compiling and the concepts are looking good, but when I tried it out I haven't managed to hit any hardware watchpoint.

I tried to verify that PTRACE_SETHBPREGS works as intended but when I did a PTRACE_GETHBPREGS immediately after it I read back a different value for the control register then the one I written there before (tested with Android 5.1 on a Nexus 5)

omjavaid updated this revision to Diff 33075.Aug 25 2015, 7:19 AM
omjavaid edited edge metadata.

This updated patches correct problems in arm hardware watchpoint support patch posted earlier.

This patch has been tested on samsung chromebook (ARM - Linux) and PandaBoard using basic watchpoint test application.

Also it was tested on Nexus 7 Android device.

On chromebook linux we are able to set and clear all types of watchpoints but on android we end up getting a watchpoint packet error because we are not able to call hardware watchpoint ptrace functions successfully.

I still dont have a android device on me that has hardware watchpoints enable but fact that this functionality now works on linux means that we should get this into our source code.

I am trying to write a test to check hardware watchpoint capabilities of a platform so we are pretty sure what works on which platform.

Is this good to commit?

tberghammer accepted this revision.Aug 25 2015, 8:37 AM
tberghammer edited edge metadata.

Looks good with a few minor comments inline

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
397

(nit): Please initialize these variables

434

(nit): Indentation

444

Please check for error

484

I think a return true is missing here

484

Please check for error

525

(nit): Please initialize

599

Please check for error

640

Please check for error

673

Please check for error

This revision is now accepted and ready to land.Aug 25 2015, 8:37 AM
omjavaid closed this revision.Feb 9 2016, 12:08 AM