[lldb] Don't use hardware index to determine whether a breakpoint site is hardware

[tatyana-krasnukha]

Summary

Most process plugins (if not all) don't set hardware index for breakpoints. They even are not able to determine this index.
This patch makes StoppointLocation::IsHardware pure virtual and lets BreakpointSite override it using more accurate BreakpointSite::Type.

It also adds assertions to be sure that a breakpoint site is hardware when this is required.

[tatyana-krasnukha]

Probably fixes llvm.org/PR44659, though I cannot check on arm/aarch64.

[JDevlieghere]

lldb/source/Breakpoint/BreakpointLocation.cpp

73

Should we sanity check that this is true when m_hardware_index != LLDB_INVALID_INDEX32?

lldbassert(m_hardware_index == LLDB_INVALID_INDEX32 || m_bp_site_sp->IsHardware());

[labath]

Could you clarify what's the functional change here? Does this change the kind of breakpoint that lldb sets, or just how it gets reported? E.g., does the ProcessGDBRemote acutally cause lldb to send a different packet, or is that just reacting to the change in the semantics of other code?

[labath]

In D84257#2164967, @tatyana-krasnukha wrote:

Probably fixes llvm.org/PR44659, though I cannot check on arm/aarch64.

@omjavaid might be interested in checking that.

[tatyana-krasnukha]

lldb/source/Breakpoint/BreakpointLocation.cpp

73

This is a good point. Though, should we check BreakpointLocation's m_hardware_index or BreakpointSite's m_hardware_index? Both of them inherit this member from StoppointLocation. To be honest, I don't think that BreakpointLocation should have software/hardware trait at all.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

3208

In D84257#2166081, @labath wrote:

Could you clarify what's the functional change here? Does this change the kind of breakpoint that lldb sets, or just how it gets reported? E.g., does the ProcessGDBRemote acutally cause lldb to send a different packet, or is that just reacting to the change in the semantics of other code?

In the ProcessGDBRemote::EnableBreakpointSite (lines 3098 - 3103) it always creates external breakpoint with eBreakpointSoftware, so nothing is going to be changed in ProcessGDBRemote behavior. And now, when bp_site->IsHardware() checks whether BreakpontSite::Type is BreakpointSite::eHardware , the condition if (bp_site->IsHardware()) doesn't make sense here anymore.

[jingham]

lldb/source/Breakpoint/BreakpointLocation.cpp

73

I hadn't noticed that StoppointLocation had picked up the m_hardware_index, that seems the wrong place for it. StoppointLocation is a shared base class between BreakpointLocations, which don't know how the breakpoint got set, and BreakpointSite's which do. Seems like StoppointLocation picked up some features really proper to BreakpointSites.

The BreakpointLocation does need to say whether it requested a hardware breakpoint or not (StoppointLocation::m_hardware). But the location can't know whether it IS hardware or not.

For instance, I could set two breakpoints that both have a location pointing to the same address, one requesting Hardware and one not. In that case, I would hope the two Locations would resolve to a single breakpoint site that is hardware. It would be silly to do otherwise. But that means the second location could reasonably think it was a software breakpoint, but it would be wrong.

Seems to me m_hardware_index should move out of StoppointLocation and into BreakpointSite. You'll also have to move the field to Watchpoint as well - it doesn't have a notion of locations and sites and so was relying on StoppointLocation to store the hardware index.

[omjavaid]

In D84257#2166215, @labath wrote:

In D84257#2164967, @tatyana-krasnukha wrote:

Probably fixes llvm.org/PR44659, though I cannot check on arm/aarch64.

@omjavaid might be interested in checking that.

Yes, I have verified this also fixes llvm.org/PR44659

[tatyana-krasnukha]

@omjavaid, thank you for verifying this!

Added the checks that IsHardware is consistent with m_hardware_index.

[tatyana-krasnukha]

I went further than just moving m_hardware_index from StoppointLocation. Please, take a look at D84527 and let me know what you think about that.

[tatyana-krasnukha]

Ups... Pushed it occationaly with the other patchs.

lldb/include/lldb/Breakpoint/BreakpointLocation.h

 //===-- BreakpointLocation.h ------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_BREAKPOINT_BREAKPOINTLOCATION_H
 #define LLDB_BREAKPOINT_BREAKPOINTLOCATION_H
 
 #include <memory>
 #include <mutex>
 
 #include "lldb/Breakpoint/BreakpointOptions.h"
 #include "lldb/Breakpoint/StoppointLocation.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 /// \class BreakpointLocation BreakpointLocation.h
public:
   void SetEnabled(bool enabled);
 
   /// Check the Enable/Disable state.
   ///
   /// \return
   ///     \b true if the breakpoint is enabled, \b false if disabled.
   bool IsEnabled() const;
 
+  /// Check if it is a hardware breakpoint.
+  ///
+  /// \return
+  ///     \b true if the breakpoint is a harware breakpoint.
+  bool IsHardware() const override;
+
   /// If \a auto_continue is \b true, set the breakpoint to continue when hit.
   void SetAutoContinue(bool auto_continue);
 
   /// Check the AutoContinue state.
   ///
   /// \return
   ///     \b true if the breakpoint is set to auto-continue, \b false if not.
   bool IsAutoContinue() const;

lldb/include/lldb/Breakpoint/BreakpointSite.h

 //===-- BreakpointSite.h ----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_BREAKPOINT_BREAKPOINTSITE_H
 #define LLDB_BREAKPOINT_BREAKPOINTSITE_H
 
 #include <list>
 #include <mutex>
 
 
 #include "lldb/Breakpoint/BreakpointLocationCollection.h"
 #include "lldb/Breakpoint/StoppointLocation.h"
+#include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-forward.h"
 
 namespace lldb_private {
 
 /// \class BreakpointSite BreakpointSite.h "lldb/Breakpoint/BreakpointSite.h"
 /// Class that manages the actual breakpoint that will be inserted into the
 /// running program.
public:
   /// Tell whether ALL the breakpoints in the location collection are
   /// internal.
   ///
   /// \result
   ///     \b true if all breakpoint locations are owned by internal breakpoints,
   ///     \b false otherwise.
   bool IsInternal() const;
 
+  bool IsHardware() const override {
+    lldbassert(BreakpointSite::Type::eHardware == GetType() ||
+               !HardwareRequired());
+    return BreakpointSite::Type::eHardware == GetType();
+  }
+
   BreakpointSite::Type GetType() const { return m_type; }
 
   void SetType(BreakpointSite::Type type) { m_type = type; }
 
 private:
   friend class Process;
   friend class BreakpointLocation;
   // The StopInfoBreakpoint knows when it is processing a hit for a thread for

lldb/include/lldb/Breakpoint/StoppointLocation.h

 //===-- StoppointLocation.h -------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_BREAKPOINT_STOPPOINTLOCATION_H
 #define LLDB_BREAKPOINT_STOPPOINTLOCATION_H
 
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-private.h"
 // #include "lldb/Breakpoint/BreakpointOptions.h"
 
 namespace lldb_private {
 
 class StoppointLocation {
 public:
   // Constructors and Destructors
public:
   uint32_t GetByteSize() const { return m_byte_size; }
 
   uint32_t GetHitCount() const { return m_hit_count; }
 
   uint32_t GetHardwareIndex() const { return m_hardware_index; }
 
   bool HardwareRequired() const { return m_hardware; }
 
-  virtual bool IsHardware() const {
-    return m_hardware_index != LLDB_INVALID_INDEX32;
-  }
+  virtual bool IsHardware() const = 0;
 
   virtual bool ShouldStop(StoppointCallbackContext *context) { return true; }
 
   virtual void Dump(Stream *stream) const {}
 
   virtual void SetHardwareIndex(uint32_t index) { m_hardware_index = index; }
 
   lldb::break_id_t GetID() const { return m_loc_id; }

lldb/source/Breakpoint/BreakpointLocation.cpp

 //===-- BreakpointLocation.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/BreakpointID.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/UserExpression.h"

 }
 
 Address &BreakpointLocation::GetAddress() { return m_address; }
 
 Breakpoint &BreakpointLocation::GetBreakpoint() { return m_owner; }
 
 Target &BreakpointLocation::GetTarget() { return m_owner.GetTarget(); }
 
+bool BreakpointLocation::IsHardware() const {
+  if (m_bp_site_sp)
+    return m_bp_site_sp->IsHardware();
+
+  // If breakpoint location is not resolved yet, it cannot be hardware.
+  return false;
+}
+
 bool BreakpointLocation::IsEnabled() const {
   if (!m_owner.IsEnabled())
     return false;
   else if (m_options_up != nullptr)
     return m_options_up->IsEnabled();
   else
     return true;
 }

lldb/source/Breakpoint/Watchpoint.cpp

 //===-- Watchpoint.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Breakpoint/Watchpoint.h"
 
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectMemory.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/Process.h"

 std::string Watchpoint::GetWatchSpec() { return m_watch_spec_str; }
 
 void Watchpoint::SetWatchSpec(const std::string &str) {
   m_watch_spec_str = str;
 }
 
 // Override default impl of StoppointLocation::IsHardware() since m_is_hardware
 // member field is more accurate.
-bool Watchpoint::IsHardware() const { return m_is_hardware; }
+bool Watchpoint::IsHardware() const {
+  lldbassert(m_is_hardware || !HardwareRequired());
+  return m_is_hardware;
+}
 
 bool Watchpoint::IsWatchVariable() const { return m_is_watch_variable; }
 
 void Watchpoint::SetWatchVariable(bool val) { m_is_watch_variable = val; }
 
 bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) {
   ConstString watch_name("$__lldb__watch_value");
   m_old_value_sp = m_new_value_sp;

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 //===-- ProcessGDBRemote.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/Config.h"
 
 #include <errno.h>
 #include <stdlib.h>
 #if LLDB_ENABLE_POSIX
 #include <netinet/in.h>
 #include <sys/mman.h>
 #include <sys/socket.h>
 #include <unistd.h>
if (bp_site->IsEnabled()) {
 
     case BreakpointSite::eHardware:
       if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointHardware, false,
                                                 addr, bp_op_size))
         error.SetErrorToGenericError();
       break;
 
     case BreakpointSite::eExternal: {
-      GDBStoppointType stoppoint_type;
-      if (bp_site->IsHardware())
-        stoppoint_type = eBreakpointHardware;
-      else
-        stoppoint_type = eBreakpointSoftware;
-
-      if (m_gdb_comm.SendGDBStoppointTypePacket(stoppoint_type, false, addr,
-                                                bp_op_size))
+      if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointSoftware, false,
+                                                addr, bp_op_size))
         error.SetErrorToGenericError();
     } break;
     }
     if (error.Success())
       bp_site->SetEnabled(false);
   } else {
     LLDB_LOGF(log,
               "ProcessGDBRemote::DisableBreakpointSite (site_id = %" PRIu64