[Lldb-commits] [PATCH] D70907: Change Target::FindBreakpointsByName to use a vector

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Using a BreakpointList corrupts the breakpoints' IDs because
BreakpointList::Add sets the ID, so use a vector instead.

Note that, despite the similar name, SBTarget::FindBreakpointsByName
doesn't suffer the same problem, because it uses a SBBreakpointList,
which is more like a BreakpointIDList than a BreakpointList under the
covers.

Add a check to TestBreakpointNames that, without this fix, notices the
ID getting mutated and fails.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70907

Files:
  lldb/include/lldb/Breakpoint/BreakpointList.h
  lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
  lldb/source/API/SBTarget.cpp
  lldb/source/Breakpoint/BreakpointList.cpp
  lldb/source/Target/Target.cpp


Index: lldb/source/Target/Target.cpp
===================================================================
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -728,11 +728,11 @@
 }
 
 void Target::ApplyNameToBreakpoints(BreakpointName &bp_name) {
-  BreakpointList bkpts_with_name(false);
+  std::vector<lldb::BreakpointSP> bkpts_with_name;
   m_breakpoint_list.FindBreakpointsByName(bp_name.GetName().AsCString(),
                                           bkpts_with_name);
 
-  for (auto bp_sp : bkpts_with_name.Breakpoints())
+  for (auto bp_sp : bkpts_with_name)
     bp_name.ConfigureBreakpoint(bp_sp);
 }
 
Index: lldb/source/Breakpoint/BreakpointList.cpp
===================================================================
--- lldb/source/Breakpoint/BreakpointList.cpp
+++ lldb/source/Breakpoint/BreakpointList.cpp
@@ -129,7 +129,7 @@
 }
 
 bool BreakpointList::FindBreakpointsByName(const char *name,
-                                           BreakpointList &matching_bps) {
+                                           std::vector<lldb::BreakpointSP> &matching_bps) {
   Status error;
   if (!name)
     return false;
@@ -139,7 +139,7 @@
 
   for (BreakpointSP bkpt_sp : Breakpoints()) {
     if (bkpt_sp->MatchesName(name)) {
-      matching_bps.Add(bkpt_sp, false);
+      matching_bps.push_back(bkpt_sp);
     }
   }
 
Index: lldb/source/API/SBTarget.cpp
===================================================================
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -1176,12 +1176,12 @@
   TargetSP target_sp(GetSP());
   if (target_sp) {
     std::lock_guard<std::recursive_mutex> guard(target_sp->GetAPIMutex());
-    BreakpointList bkpt_list(false);
+    std::vector<lldb::BreakpointSP> bkpt_list;
     bool is_valid =
         target_sp->GetBreakpointList().FindBreakpointsByName(name, bkpt_list);
     if (!is_valid)
       return false;
-    for (BreakpointSP bkpt_sp : bkpt_list.Breakpoints()) {
+    for (BreakpointSP bkpt_sp : bkpt_list) {
       bkpts.AppendByID(bkpt_sp->GetID());
     }
   }
Index: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
+++ lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
@@ -155,8 +155,13 @@
     def do_check_using_names(self):
         """Use Python APIs to check names work in place of breakpoint ID's."""
 
+        # Create a dummy breakpoint to use up ID 1
+        _ = self.target.BreakpointCreateByLocation(self.main_file_spec, 30)
+
+        # Create a breakpiont to test with
         bkpt = self.target.BreakpointCreateByLocation(self.main_file_spec, 10)
         bkpt_name = "ABreakpoint"
+        bkpt_id = bkpt.GetID()
         other_bkpt_name= "_AnotherBreakpoint"
 
         # Add a name and make sure we match it:
@@ -169,6 +174,7 @@
         self.assertTrue(bkpts.GetSize() == 1, "One breakpoint matched.")
         found_bkpt = bkpts.GetBreakpointAtIndex(0)
         self.assertTrue(bkpt.GetID() == found_bkpt.GetID(),"The right breakpoint.")
+        self.assertTrue(bkpt.GetID() == bkpt_id,"With the same ID as before.")
 
         retval = lldb.SBCommandReturnObject()
         self.dbg.GetCommandInterpreter().HandleCommand("break disable %s"%(bkpt_name), retval)
Index: lldb/include/lldb/Breakpoint/BreakpointList.h
===================================================================
--- lldb/include/lldb/Breakpoint/BreakpointList.h
+++ lldb/include/lldb/Breakpoint/BreakpointList.h
@@ -68,7 +68,7 @@
   ///
   /// \result
   ///   \bfalse if the input name was not a legal breakpoint name.
-  bool FindBreakpointsByName(const char *name, BreakpointList &matching_bps);
+  bool FindBreakpointsByName(const char *name, std::vector<lldb::BreakpointSP> &matching_bps);
 
   /// Returns the number of elements in this breakpoint list.
   ///


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70907.231715.patch
Type: text/x-patch
Size: 4045 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20191202/1a196dfe/attachment.bin>