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

[Greg Clayton via Phabricator via lldb-commits]

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

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?



================
Comment at: include/lldb/Host/common/HardwareBreakpointList.h:24
+
+class HardwareBreakpointList {
+public:
----------------
labath wrote:
> What's the benefit of this class over using `std::map<lldb::addr_t, HardwareBreakpoint>` directly?
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.


================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:318-320
   NativeBreakpointList m_breakpoint_list;
   NativeWatchpointList m_watchpoint_list;
+  HardwareBreakpointList m_hw_breakpoint_list;
----------------
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;
};
```




================
Comment at: include/lldb/Host/common/NativeRegisterContext.h:78-81
+  virtual Error ClearAllHardwareBreakpoints();
+
+  virtual Error GetHardwareBreakHitIndex(uint32_t &bp_index,
+                                         lldb::addr_t trap_addr);
----------------
These would go away if we expand NativeBreakpoint to include new fields to support hardware breakpoints?


================
Comment at: include/lldb/Host/common/NativeThreadProtocol.h:59-64
+  // ---------------------------------------------------------------------
+  // Thread-specific Hardware Breakpoint routines
+  // ---------------------------------------------------------------------
+  virtual Error SetHardwareBreakpoint(lldb::addr_t addr, size_t size) = 0;
+
+  virtual Error RemoveHardwareBreakpoint(lldb::addr_t addr) = 0;
----------------
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).


================
Comment at: source/Host/common/HardwareBreakpointList.cpp:1-30
+//===-- HardwareBreakpointList.cpp ------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
Remove this if we just end up using the one breakpoint list and expand NativeBreakpoint to include fields to support hardware breakpoints.


================
Comment at: source/Host/common/NativeProcessProtocol.cpp:275-276
+
+Error NativeProcessProtocol::SetHardwareBreakpoint(lldb::addr_t addr,
+                                                   size_t size) {
+  // This default implementation assumes setting a hardware breakpoint for
----------------
Change this to take the "NativeBreakpoint" structure instead of manual arguments?


================
Comment at: source/Host/common/NativeProcessProtocol.cpp:329
+
+Error NativeProcessProtocol::RemoveHardwareBreakpoint(lldb::addr_t addr) {
+  // Update the thread list
----------------
change argument to take NativeBreakpoint struct?


================
Comment at: source/Host/common/NativeProcessProtocol.cpp:430-436
+Error NativeProcessProtocol::RemoveBreakpoint(lldb::addr_t addr,
+                                              bool hardware) {
+  if (hardware)
+    return RemoveHardwareBreakpoint(addr);
+  else
+    return m_breakpoint_list.DecRef(addr);
 }
----------------
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.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:1738
 
+Error NativeProcessLinux::RemoveBreakpoint(lldb::addr_t addr, bool hardware) {
+  if (hardware)
----------------
Use NativeBreakpoint struct as arg?


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:89
 
+  Error RemoveBreakpoint(lldb::addr_t addr, bool hardware = false) override;
+
----------------
Use NativeBreakpoint struct instead of manual args?


https://reviews.llvm.org/D29669