Page MenuHomePhabricator

Hardware breakpoints implementation for Arm/AArch64 targets
ClosedPublic

Authored by omjavaid on Feb 7 2017, 12:10 PM.

Details

Summary

This patch implements hardware breakpoint functionality in LLDB for AArch64 targets. AArch64 targets supports hardware breakpoints via ptrace interface similar to hardware watchpoints. Hardware breakpoints provide fast way to stop target not requiring any memory read/write like software breakpoints.

This patch fixes areas which required tweaking to put hardware breakpoints implementation in place via process gdb remote. We are able to test these breakpoints via a simple application on Android Nexux5x devices.

I am in process of testing on various other platforms and also writing lldb testsuite test cases.

Also added in final revision.

  1. LLDB Testsuite testcases for hardware breakpoints testing. (Done)
  2. Similar implementation for Arm targets.

Looking forward to upstream comments on this.

Test code in case someone wants to test:

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>

#define THREAD_COUNT 8

void *thread_function( void *ptr )
{

int *argument = (int *) ptr;

printf("Thread #%i \n", *argument);

}

int main()
{

int argument[THREAD_COUNT];
pthread_t thread_handle[THREAD_COUNT];

int i;
for (i = 0; i < THREAD_COUNT; i++)
{ 
  argument[i] = i;
  if (pthread_create( &thread_handle[i], NULL, thread_function, (void*) &argument[i]))
  {
    printf("Error - pthread_create() failed\n");
    exit(EXIT_FAILURE);
  }
}

for (i = 0; i < THREAD_COUNT; i++)
  pthread_join( thread_handle[i], NULL);

exit(EXIT_SUCCESS);

return 0;

}

Diff Detail

Repository
rL LLVM

Event Timeline

omjavaid created this revision.Feb 7 2017, 12:10 PM
labath edited edge metadata.Feb 7 2017, 2:40 PM

Don't be intimidated by the number of comments :) - I think the change looks reasonable as a whole - most of them are just about style and come from the fact you were probably working on the patch while I was refactoring this code. As for tests, what I'd definitely want to see is a lldb-server-style test, which tests just the server changes without the client part (maybe you we're already talking about those - I just want to make sure we're on the same page).

As for the stepping speed, do you have any data which shows that this is actually a bottleneck in those operations? (I'm not against doing that, I'm just curious).

include/lldb/Host/common/HardwareBreakpointList.h
24 ↗(On Diff #87493)

What's the benefit of this class over using std::map<lldb::addr_t, HardwareBreakpoint> directly?

source/Host/common/NativeProcessProtocol.cpp
142 ↗(On Diff #87493)

llvm::Optional<std::pair<uint32_t, uint32_t>> GetHardwareDebugSupportInfo()

145 ↗(On Diff #87493)

This comment needs update

source/Plugins/Process/Linux/NativeProcessLinux.cpp
875 ↗(On Diff #87493)

Please rename the wp_index variable to something more generic if you're going to use it here.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
367 ↗(On Diff #87493)

Please use LLDB_LOG here.

384 ↗(On Diff #87493)

Please use POSIX_LOG_BREAKPOINTS here (the code in the NativeProcessLinux plugin was using a combination of posix and lldb log channels, so i've standardized it on posix).

414 ↗(On Diff #87493)

s/watchpoints/breakpoints/ ?

source/Plugins/Process/Linux/NativeThreadLinux.cpp
246 ↗(On Diff #87493)

This doesn't appear to be setting the breakpoint on *all* threads (and it probably shouldnt)

clayborg requested changes to this revision.Feb 8 2017, 10:11 AM

I would prefer to see NativeBreakpoint struct expanded to have more member variables instead of adding a new hardware breakpoint list. Then you just ask any breakpoint to enable/disable/remove itself and the structure contains all of the info we need. Keeping two lists means we have to check two lists. Let me know if any of my inline comments weren't clear?

include/lldb/Host/common/HardwareBreakpointList.h
24 ↗(On Diff #87493)

There is no real error you can return from Add. Remove can just return a bool if it succeeds or not. So we can probably just use a std::map directly like Pavel suggests.

include/lldb/Host/common/NativeProcessProtocol.h
318–320 ↗(On Diff #87493)

It would be nice to just expand the contents of NativeBreakpoint and remove the HardwareBreakpoint and HardwareBreakpointList. No need for another list to check. I would suggest just adding a "bool m_hardware;" to NativeBreakpoint. Or add a new NativeBreakpoint::Type enum in NativeBreakpoint.h that is:

NativeBreakpoint
{
  enum class Type
  {
    Software,
    Hardware
  };
  Type m_type;
};
include/lldb/Host/common/NativeRegisterContext.h
78–81 ↗(On Diff #87493)

These would go away if we expand NativeBreakpoint to include new fields to support hardware breakpoints?

include/lldb/Host/common/NativeThreadProtocol.h
59–64 ↗(On Diff #87493)

Would we want to pass in the NativeBreakpoint class here instead of individual arguments? Maybe each thread might want to store some extra data inside the NativeBreakpoint class (like the index of the hardware breakpoint, which means we might need to also add more fields to NativeBreakpoint).

source/Host/common/HardwareBreakpointList.cpp
1–30 ↗(On Diff #87493)

Remove this if we just end up using the one breakpoint list and expand NativeBreakpoint to include fields to support hardware breakpoints.

source/Host/common/NativeProcessProtocol.cpp
275–276 ↗(On Diff #87493)

Change this to take the "NativeBreakpoint" structure instead of manual arguments?

329 ↗(On Diff #87493)

change argument to take NativeBreakpoint struct?

430–436 ↗(On Diff #87493)

Switch to use NativeBreakpoint struct as argument? Then we won't need extra args since the NativeBreakpoint will have all the info inside of it.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1738 ↗(On Diff #87493)

Use NativeBreakpoint struct as arg?

source/Plugins/Process/Linux/NativeProcessLinux.h
89 ↗(On Diff #87493)

Use NativeBreakpoint struct instead of manual args?

This revision now requires changes to proceed.Feb 8 2017, 10:11 AM

I would prefer to see NativeBreakpoint struct expanded to have more member variables instead of adding a new hardware breakpoint list. Then you just ask any breakpoint to enable/disable/remove itself and the structure contains all of the info we need. Keeping two lists means we have to check two lists. Let me know if any of my inline comments weren't clear?

So I thought about it but gave up on the idea for two reasons:

  1. Hardware breakpoints implementation has more similarities with hardware watchpoints than software breakpoints and I plan to consolidate both functionalities onces hardware brekapoints start working.
  1. Software breakpoint are implemented with process wide scope and hardware breakpoints are thread specific which means that we add existing hw breakpoint on all new threads. On resume we ll be traversing through a breakpoint list which predominantly has only software breakpoints in most cases a overhead will be added in case we have 20 software breakpoint and just 1 hardware breakpoint installed by user.

Let me know what do you think about above two ppoints and I ll clear up all other comments in following revision.

If it isn't making too much extra code I am fine with it being separate if you believe this is the right way forward. Give it some thought and if you still believe they should be separate, I won't object. It might be nice to send the HardwareBreakpoint structure down to the functions that are going to enable/disable the HW breakpoints instead of individual arguments as you may end up adding members to the HardwareBreakpoint in the future and this will keep you from having to update all of the signatures.

omjavaid updated this revision to Diff 88505.Feb 15 2017, 3:09 AM
omjavaid edited edge metadata.

This patch has following updates:

  1. Support for Arm hardware breakpoints
  2. LLDBServer Test cases for testing Z1 z1 packets.
  3. Test case to test hardware breakpoint command and multi-threaded hardware breakpoints.
  4. Get rid of HardwareBreakpointList class and replaced it with simple map and struct.
  5. Fix LLDB Log mismtach to POSIX_LOG
  6. Fix arm hardware breakpoint register read/write function.

Kindly provide your feedback. Is this all good to go in now?

omjavaid retitled this revision from Hardware breakpoints implementation for AArch64 targets to Hardware breakpoints implementation for Arm/AArch64 targets.Feb 20 2017, 10:56 AM

LGTM, but Pavel should give the ok as well.

labath requested changes to this revision.Feb 21 2017, 1:59 AM

I am sorry about the delay - I was busy last week and then this kinda fell off my radar.

The change looks good, but I want to make the test more stable - we are running these in CI and I've already seen some very unlikely things happen, so I don't want this to be flaky.

Also, it looks like you didn't catch all instances of using printf-style logging.

packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py
78 ↗(On Diff #88505)

This is quite racy.

There is nothing that guarantees that we will stop on the breakpoint at least four times. In particular, it can happen that all 8 threads hit the breakpoint simultaneously, and then you have only one stop of the whole process. If you want to have more than one stop you need to either:

  • make sure that no two threads can hit the breakpoint simultaneously (e.g., add some kind of a mutex in the inferior),
  • or count the stops more diligently (after each stop, count the number of threads stopped at the location, as in e.g. TestConcurrentEvents)

However, if the purpose of this test is to make sure the breakpoint applies to newly-created threads, then I think you don't need than many threads, and one thread would suffice. Then the test could be:

  • set a breakpoint
  • inferior spins up a thread and hits it
  • remove/disable the breakpoint
  • inferior joins the first thread
  • inferior spins up another thread which does *not* hit the breakpoint

This should make debugging any failures easier as the test is more deterministic.

108 ↗(On Diff #88505)

It looks like you are missing an assertion to verify that the exit actually happened here.

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
362 ↗(On Diff #88505)

LLDB_LOG (or just remove the log statement as it does not convey much information).

474 ↗(On Diff #88505)

LLDB_LOG (and in below function as well)

This revision now requires changes to proceed.Feb 21 2017, 1:59 AM
omjavaid added inline comments.Feb 22 2017, 7:44 PM
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py
78 ↗(On Diff #88505)

Agreed!! I ll post an alternative then.

Thanks!

omjavaid updated this revision to Diff 89642.Feb 24 2017, 4:00 AM
omjavaid edited edge metadata.
omjavaid edited the summary of this revision. (Show Details)

@labath

Hi I have updated diff with corrections.

Thanks!

labath accepted this revision.Feb 24 2017, 5:21 AM

lgtm, thanks.

packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/main.cpp
30 ↗(On Diff #89642)

I know it's only a test program, but you generally should never lock the mutex manually.

std::lock_guard<std::mutex> guard(mutex)

This revision was automatically updated to reflect the committed changes.