[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for Arm/AArch64 targets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL296119: Hardware breakpoints for Linux on Arm/AArch64 
targets (authored by omjavaid).

Changed prior to commit:
  https://reviews.llvm.org/D29669?vs=89642&id=89654#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29669

Files:
  lldb/trunk/include/lldb/Host/common/NativeBreakpointList.h
  lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/trunk/include/lldb/Host/common/NativeRegisterContext.h
  lldb/trunk/include/lldb/Host/common/NativeThreadProtocol.h
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/Makefile
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/main.cpp
  
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
  
lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
  lldb/trunk/source/Host/common/NativeRegisterContext.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h
  lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.h
  
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -2534,12 +2534,14 @@
 packet, "Too short z packet, missing software/hardware specifier");
 
   bool want_breakpoint = true;
+  bool want_hardware = false;
 
   const GDBStoppointType stoppoint_type =
   GDBStoppointType(packet.GetS32(eStoppointInvalid));
   switch (stoppoint_type) {
   case eBreakpointHardware:
 want_breakpoint = true;
+want_hardware = true;
 break;
   case eBreakpointSoftware:
 want_breakpoint = true;
@@ -2582,7 +2584,8 @@
 
   if (want_breakpoint) {
 // Try to clear the breakpoint.
-const Error error = m_debugged_process_sp->RemoveBreakpoint(addr);
+const Error error =
+m_debugged_process_sp->RemoveBreakpoint(addr, want_hardware);
 if (error.Success())
   return SendOKResponse();
 Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
@@ -3040,9 +3043,14 @@
   if (packet.GetChar() != ':')
 return SendErrorResponse(67);
 
-  uint32_t num = m_debugged_process_sp->GetMaxWatchpoints();
+  auto hw_debug_cap = m_debugged_process_sp->GetHardwareDebugSupportInfo();
+
   StreamGDBRemote response;
-  response.Printf("num:%d;", num);
+  if (hw_debug_cap == llvm::None)
+response.Printf("num:0;");
+  else
+response.Printf("num:%d;", hw_debug_cap->second);
+
   return SendPacketNoLock(response.GetString());
 }
 
Index: lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
===
--- lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
+++ lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
@@ -130,6 +130,7 @@
   ::memset(&m_fpr, 0, sizeof(m_fpr));
   ::memset(&m_gpr_arm, 0, sizeof(m_gpr_arm));
   ::memset(&m_hwp_regs, 0, sizeof(m_hwp_regs));
+  ::memset(&m_hbr_regs, 0, sizeof(m_hbr_regs));
 
   // 16 is just a maximum value, query hardware for actual watchpoint count
   m_max_hwp_supported = 16;
@@ -354,10 +355,28 @@
   return (m_reg_info.first_fpr <= reg && reg <= m_reg_info.last_fpr);
 }
 
+uint32_t NativeRegisterContextLinux_arm::NumSupportedHardwareBreakpoints() {
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_BREAKPOINTS));
+
+  if (log)
+log->Printf("NativeRegisterContextLinux_arm::%s()", __FUNCTION__);
+
+  Error error;
+
+  // Read hardware breakpoint and watchpoint information.
+  error = ReadHardwareDebugInfo();
+
+  if (error.Fail())
+return 0;
+
+  LLDB_LOG(log, "{0}", m_max_hbp_supported);
+  return m_max_hbp_supported;
+}
+
 uint32_t
 NativeRegisterContextLinux_arm::SetHardwareBreakpoint(lldb::addr_t addr,
   size_t size) {
-  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSe

[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for Arm/AArch64 targets

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

lgtm, thanks.




Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/main.cpp:30
+
+  hw_break_mutex.lock();
+  

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

`std::lock_guard guard(mutex)`


https://reviews.llvm.org/D29669



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for Arm/AArch64 targets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 89642.
omjavaid edited the summary of this revision.
omjavaid added a comment.
Herald added a subscriber: srhines.

@labath

Hi I have updated diff with corrections.

Thanks!


https://reviews.llvm.org/D29669

Files:
  include/lldb/Host/common/NativeBreakpointList.h
  include/lldb/Host/common/NativeProcessProtocol.h
  include/lldb/Host/common/NativeRegisterContext.h
  include/lldb/Host/common/NativeThreadProtocol.h
  
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/Makefile
  
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py
  
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/main.cpp
  packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
  packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  source/Host/common/NativeProcessProtocol.cpp
  source/Host/common/NativeRegisterContext.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
  source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.h
  source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  source/Plugins/Process/Linux/NativeThreadLinux.cpp
  source/Plugins/Process/Linux/NativeThreadLinux.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -2531,12 +2531,14 @@
 packet, "Too short z packet, missing software/hardware specifier");
 
   bool want_breakpoint = true;
+  bool want_hardware = false;
 
   const GDBStoppointType stoppoint_type =
   GDBStoppointType(packet.GetS32(eStoppointInvalid));
   switch (stoppoint_type) {
   case eBreakpointHardware:
 want_breakpoint = true;
+want_hardware = true;
 break;
   case eBreakpointSoftware:
 want_breakpoint = true;
@@ -2579,7 +2581,8 @@
 
   if (want_breakpoint) {
 // Try to clear the breakpoint.
-const Error error = m_debugged_process_sp->RemoveBreakpoint(addr);
+const Error error =
+m_debugged_process_sp->RemoveBreakpoint(addr, want_hardware);
 if (error.Success())
   return SendOKResponse();
 Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
@@ -3037,9 +3040,14 @@
   if (packet.GetChar() != ':')
 return SendErrorResponse(67);
 
-  uint32_t num = m_debugged_process_sp->GetMaxWatchpoints();
+  auto hw_debug_cap = m_debugged_process_sp->GetHardwareDebugSupportInfo();
+
   StreamGDBRemote response;
-  response.Printf("num:%d;", num);
+  if (hw_debug_cap == llvm::None)
+response.Printf("num:0;");
+  else
+response.Printf("num:%d;", hw_debug_cap->second);
+
   return SendPacketNoLock(response.GetString());
 }
 
Index: source/Plugins/Process/Linux/NativeThreadLinux.h
===
--- source/Plugins/Process/Linux/NativeThreadLinux.h
+++ source/Plugins/Process/Linux/NativeThreadLinux.h
@@ -46,6 +46,10 @@
 
   Error RemoveWatchpoint(lldb::addr_t addr) override;
 
+  Error SetHardwareBreakpoint(lldb::addr_t addr, size_t size) override;
+
+  Error RemoveHardwareBreakpoint(lldb::addr_t addr) override;
+
 private:
   // -
   // Interface for friend classes
@@ -102,6 +106,7 @@
   std::string m_stop_description;
   using WatchpointIndexMap = std::map;
   WatchpointIndexMap m_watchpoint_index_map;
+  WatchpointIndexMap m_hw_break_index_map;
   std::unique_ptr m_step_workaround;
 };
 
Index: source/Plugins/Process/Linux/NativeThreadLinux.cpp
===
--- source/Plugins/Process/Linux/NativeThreadLinux.cpp
+++ source/Plugins/Process/Linux/NativeThreadLinux.cpp
@@ -190,6 +190,38 @@
   return Error("Clearing hardware watchpoint failed.");
 }
 
+Error NativeThreadLinux::SetHardwareBreakpoint(lldb::addr_t addr, size_t size) {
+  if (m_state == eStateLaunching)
+return Error();
+
+  Error error = RemoveHardwareBreakpoint(addr);
+  if (error.Fail())
+return error;
+
+  NativeRegisterContextSP reg_ctx = GetRegisterContext();
+  uint32_t bp_index = reg_ctx->SetHardwareBreakpoint(addr, size);
+
+  if (bp_index == LLDB_INVALID_INDEX32)
+return Error("Setting hardware breakpoint failed.");
+
+  m_hw_break_index_map.insert({addr, bp_index});
+  return Error();
+}
+
+Error NativeThreadLinux::RemoveHardwareBreakpoint(lldb::addr_t addr) {
+  auto

[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for Arm/AArch64 targets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py:78
+
+while count < 4 :
+ 

labath wrote:
> 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.
Agreed!! I ll post an alternative then.

Thanks!


https://reviews.llvm.org/D29669



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for Arm/AArch64 targets

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

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.




Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py:78
+
+while count < 4 :
+ 

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.



Comment at: 
packages/Python/lldbsuite/test/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py:108
+# Continue. Program should exit without stopping anywhere.
+self.runCmd("process continue")

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



Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:362
+  if (log)
+log->Printf("NativeRegisterContextLinux_arm::%s()", __FUNCTION__);
+

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



Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:474
+  if (log)
+log->Printf("NativeRegisterContextLinux_arm64::%s()", __FUNCTION__);
 

LLDB_LOG (and in below function as well)


https://reviews.llvm.org/D29669



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29669: Hardware breakpoints implementation for Arm/AArch64 targets

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

LGTM, but Pavel should give the ok as well.


https://reviews.llvm.org/D29669



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits