Change Target::FindBreakpointsByName to return Expected<vector>

[JosephTremoulet]

Summary

Using a BreakpointList corrupts the breakpoints' IDs because
BreakpointList::Add sets the ID, so use a vector instead, and
update the signature to return the vector wrapped in an
llvm::Expected which can propagate any error from the inner
call to StringIsBreakpointName.

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.

[JDevlieghere]

lldb/include/lldb/Breakpoint/BreakpointList.h

70–73

I think the API would look nicer if we returned an llvm::Optional<std::vector>> where None means an invalid breakpoint name and an empty list no matches. What do you think?

[JosephTremoulet]

lldb/include/lldb/Breakpoint/BreakpointList.h

70–73

I think I'd go with Expected<> over Optional<>, since the false return indicates invalid input.

I actually originally considered different signatures for this change. My first inclination was to switch the populated list from a BreakpointList to a BreakpointIDList, but it seemed that would be inefficient for the call from Target::ApplyNameToBreakpoints that needs the actual breakpoints. So then I went down the route of Expected<iterator_range<breakpoint iterator>>, but it was quickly becoming more code (and more convoluted) than seemed warranted. So I looked around, saw std::vectors being populated by e.g. Breakpoint::GetNames and SourceManager::FindLinesMatchingRegex, and decided to follow suit.

Which is a long way to say: populating a std::vector seems to "fit in" with surrounding code better, but aside from that, yes I think returning Expected<std::vector> would be a more natural fit. I don't know which of those concerns to prefer in this code; LMK and I'm happy to switch it if that seems best.

[JDevlieghere]

lldb/include/lldb/Breakpoint/BreakpointList.h

70–73

I think an Expected is perfect. I proposed Optional because the way things are, the call sites would have to discard the error anyway. However, given that FindBreakpointsByName takes a Status, I think it would be good to propagate that up.

[jingham]

In other places, I've used "XList" to mean the container that manages the things contained, and "XCollection" to be a random possibly unrelated collection of items. It doesn't make any sense to have a collection of breakpoints from more than one target, so the collection class could ensure that, whereas a std::vector is just a random assortment. But I'm not sure that adds enough value to warrant adding a collection container here. std::vector is fine.

[JosephTremoulet]

• Change signature to return Expected<vector<BreakpointSP>>

[JDevlieghere]

LGTM

lldb/source/Breakpoint/BreakpointList.cpp

136

I think llvm::errc::invalid_argument would fit.

[JosephTremoulet]

• Use invalid_argument error code

[JosephTremoulet]

lldb/source/Breakpoint/BreakpointList.cpp

136

SGTM; updated.

lldb/include/lldb/Breakpoint/BreakpointList.h

public:
   lldb::BreakpointSP GetBreakpointAtIndex(size_t i) const;
 
   /// Find all the breakpoints with a given name
   ///
   /// \param[in] name
   ///   The breakpoint name for which to search.
   ///
   /// \result
-  ///   \bfalse if the input name was not a legal breakpoint name.
-  bool FindBreakpointsByName(const char *name, BreakpointList &matching_bps);
+  ///   error if the input name was not a legal breakpoint name, vector
+  ///   of breakpoints otherwise.
+  llvm::Expected<std::vector<lldb::BreakpointSP>>
+  FindBreakpointsByName(const char *name);
 
   /// Returns the number of elements in this breakpoint list.
   ///
   /// \result
   ///   The number of elements.
   size_t GetSize() const {
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
     return m_breakpoints.size();

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py

def do_check_illegal_names(self):
 
             retval =lldb.SBCommandReturnObject()
             self.dbg.GetCommandInterpreter().HandleCommand("break set -n whatever -N '%s'"%(bad_name), retval)
             self.assertTrue(not retval.Succeeded(), "break set succeeded with: illegal name: %s"%(bad_name))
 
     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:
         success = bkpt.AddName(bkpt_name)
         self.assertTrue(success, "We couldn't add a legal name to a breakpoint.")
 
         bkpts = lldb.SBBreakpointList(self.target)
         self.target.FindBreakpointsByName(bkpt_name, bkpts)
 
         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)
         self.assertTrue(retval.Succeeded(), "break disable failed with: %s."%(retval.GetError()))
         self.assertTrue(not bkpt.IsEnabled(), "We didn't disable the breakpoint.")
 
         # Also make sure we don't apply commands to non-matching names:
         self.dbg.GetCommandInterpreter().HandleCommand("break modify --one-shot 1 %s"%(other_bkpt_name), retval)

lldb/source/API/SBTarget.cpp

 bool SBTarget::FindBreakpointsByName(const char *name,
                                      SBBreakpointList &bkpts) {
   LLDB_RECORD_METHOD(bool, SBTarget, FindBreakpointsByName,
                      (const char *, lldb::SBBreakpointList &), name, bkpts);
 
   TargetSP target_sp(GetSP());
   if (target_sp) {
     std::lock_guard<std::recursive_mutex> guard(target_sp->GetAPIMutex());
-    BreakpointList bkpt_list(false);
-    bool is_valid =
-        target_sp->GetBreakpointList().FindBreakpointsByName(name, bkpt_list);
-    if (!is_valid)
+    llvm::Expected<std::vector<BreakpointSP>> expected_vector =
+        target_sp->GetBreakpointList().FindBreakpointsByName(name);
+    if (!expected_vector) {
+      LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS),
+               "invalid breakpoint name: {}",
+               llvm::toString(expected_vector.takeError()));
       return false;
-    for (BreakpointSP bkpt_sp : bkpt_list.Breakpoints()) {
+    }
+    for (BreakpointSP bkpt_sp : *expected_vector) {
       bkpts.AppendByID(bkpt_sp->GetID());
     }
   }
   return true;
 }
 
 void SBTarget::GetBreakpointNames(SBStringList &names) {
   LLDB_RECORD_METHOD(void, SBTarget, GetBreakpointNames, (lldb::SBStringList &),

lldb/source/Breakpoint/BreakpointList.cpp

BreakpointSP BreakpointList::FindBreakpointByID(break_id_t break_id) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
   auto it = GetBreakpointIDConstIterator(break_id);
   if (it != m_breakpoints.end())
     return *it;
   return {};
 }
 
-bool BreakpointList::FindBreakpointsByName(const char *name,
-                                           BreakpointList &matching_bps) {
-  Status error;
+llvm::Expected<std::vector<lldb::BreakpointSP>>
+BreakpointList::FindBreakpointsByName(const char *name) {
   if (!name)
-    return false;
+    return llvm::make_error<llvm::StringError>(
+        "FindBreakpointsByName requires a name",
+        llvm::inconvertibleErrorCode());
 
+  Status error;
   if (!BreakpointID::StringIsBreakpointName(llvm::StringRef(name), error))
-    return false;
+    return error.ToError();
 
+  std::vector<lldb::BreakpointSP> matching_bps;
   for (BreakpointSP bkpt_sp : Breakpoints()) {
     if (bkpt_sp->MatchesName(name)) {
-      matching_bps.Add(bkpt_sp, false);
+      matching_bps.push_back(bkpt_sp);
     }
   }
 
-  return true;
+  return matching_bps;
 }
 
 void BreakpointList::Dump(Stream *s) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   s->Printf("%p: ", static_cast<const void *>(this));
   s->Indent();
   s->Printf("BreakpointList with %u Breakpoints:\n",
             (uint32_t)m_breakpoints.size());

lldb/source/Target/Target.cpp

void Target::ConfigureBreakpointName(
     BreakpointName &bp_name, const BreakpointOptions &new_options,
     const BreakpointName::Permissions &new_permissions) {
   bp_name.GetOptions().CopyOverSetOptions(new_options);
   bp_name.GetPermissions().MergeInto(new_permissions);
   ApplyNameToBreakpoints(bp_name);
 }
 
 void Target::ApplyNameToBreakpoints(BreakpointName &bp_name) {
-  BreakpointList bkpts_with_name(false);
-  m_breakpoint_list.FindBreakpointsByName(bp_name.GetName().AsCString(),
-                                          bkpts_with_name);
+  llvm::Expected<std::vector<BreakpointSP>> expected_vector =
+      m_breakpoint_list.FindBreakpointsByName(bp_name.GetName().AsCString());
 
-  for (auto bp_sp : bkpts_with_name.Breakpoints())
+  if (!expected_vector) {
+    LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS),
+             "invalid breakpoint name: {}",
+             llvm::toString(expected_vector.takeError()));
+    return;
+  }
+
+  for (auto bp_sp : *expected_vector)
     bp_name.ConfigureBreakpoint(bp_sp);
 }
 
 void Target::GetBreakpointNames(std::vector<std::string> &names) {
   names.clear();
   for (auto bp_name : m_breakpoint_names) {
     names.push_back(bp_name.first.AsCString());
   }