[lldb/api] Improve error reporting in SBBreakpoint::AddName (NFCI)

[mib]

Summary

This patch improves the error reporting for SBBreakpoint::AddName by
adding a new method SBBreakpoint::AddNameWithErrorHandling that returns
a SBError instead of a boolean.

This way, if the breakpoint naming failed in the backend, the client
(i.e. Xcode), will be able to report the reason of that failure to the
user.

rdar://64765461

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

We can't break the SB API, it's unfortunate but this should be an overload instead...

[mib]

Introduced SBBreakpoint::AddNameWithError to avoid API stability breakage.

[JDevlieghere]

lldb/bindings/interface/SBBreakpoint.i

210

In SBDebugger we have InitializeWithErrorHandling, maybe use the same suffix here for consistency?

lldb/test/API/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py

103

Seems like a good use case for Pavel's new assertSuccess helper? (https://reviews.llvm.org/D82759)

[mib]

• Use new assertSuccess method in test.
• Rename the method to AddNameWithErrorHandling for consistency.

[teemperor]

LGTM with some minor nits.

lldb/include/lldb/API/SBBreakpoint.h

108

I wish we had some documentation when adding a name could fail (e.g. empty name string?) But we can also add this all later as most of this stuff needs documentation.

lldb/test/API/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py

146

I think we usually put spaces around operators/string literals.

lldb/bindings/interface/SBBreakpoint.i

public:
 
     void SetCommandLineCommands(SBStringList &commands);
 
     bool GetCommandLineCommands(SBStringList &commands);
 
     bool
     AddName (const char *new_name);
 
+    SBError
+    AddNameWithErrorHandling (const char *new_name);
+
     void
     RemoveName (const char *name_to_remove);
 
     bool
     MatchesName (const char *name);
 
     void
     GetNames (SBStringList &names);

lldb/include/lldb/API/SBBreakpoint.h

 //===-- SBBreakpoint.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_API_SBBREAKPOINT_H
 #define LLDB_API_SBBREAKPOINT_H
 
 #include "lldb/API/SBDefines.h"
 
 class SBBreakpointListImpl;
 
 namespace lldb {
 
 class LLDB_API SBBreakpoint {
 public:
 
public:
   void SetCommandLineCommands(SBStringList &commands);
 
   bool GetCommandLineCommands(SBStringList &commands);
 
   SBError SetScriptCallbackBody(const char *script_body_text);
 
   bool AddName(const char *new_name);
 
+  SBError AddNameWithErrorHandling(const char *new_name);
+
   void RemoveName(const char *name_to_remove);
 
   bool MatchesName(const char *name);
 
   void GetNames(SBStringList &names);
 
   size_t GetNumResolvedLocations() const;
 

lldb/source/API/SBBreakpoint.cpp

 //===-- SBBreakpoint.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/API/SBBreakpoint.h"
 #include "SBReproducerPrivate.h"
 #include "lldb/API/SBBreakpointLocation.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBProcess.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/API/SBStringList.h"
 #include "lldb/API/SBStructuredData.h"
if (bkpt_sp) {
     sb_error.SetErrorString("invalid breakpoint");
 
   return LLDB_RECORD_RESULT(sb_error);
 }
 
 bool SBBreakpoint::AddName(const char *new_name) {
   LLDB_RECORD_METHOD(bool, SBBreakpoint, AddName, (const char *), new_name);
 
+  SBError status = AddNameWithErrorHandling(new_name);
+  return status.Success();
+}
+
+SBError SBBreakpoint::AddNameWithErrorHandling(const char *new_name) {
+  LLDB_RECORD_METHOD(SBError, SBBreakpoint, AddNameWithErrorHandling,
+                     (const char *), new_name);
+
   BreakpointSP bkpt_sp = GetSP();
 
+  SBError status;
   if (bkpt_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         bkpt_sp->GetTarget().GetAPIMutex());
-    Status error; // Think I'm just going to swallow the error here, it's
-                  // probably more annoying to have to provide it.
+    Status error;
     bkpt_sp->GetTarget().AddNameToBreakpoint(bkpt_sp, new_name, error);
-    if (error.Fail())
-      return false;
+    status.SetError(error);
+  } else {
+    status.SetErrorString("invalid breakpoint");
   }
 
-  return true;
+  return status;
 }
 
 void SBBreakpoint::RemoveName(const char *name_to_remove) {
   LLDB_RECORD_METHOD(void, SBBreakpoint, RemoveName, (const char *),
                      name_to_remove);
 
   BreakpointSP bkpt_sp = GetSP();
 
LLDB_REGISTER_METHOD(lldb::SBError, SBBreakpoint, AddLocation,
                        (lldb::SBAddress &));
   LLDB_REGISTER_METHOD(void, SBBreakpoint, SetScriptCallbackFunction,
                        (const char *));
   LLDB_REGISTER_METHOD(lldb::SBError, SBBreakpoint, SetScriptCallbackFunction,
                        (const char *, SBStructuredData &));
   LLDB_REGISTER_METHOD(lldb::SBError, SBBreakpoint, SetScriptCallbackBody,
                        (const char *));
   LLDB_REGISTER_METHOD(bool, SBBreakpoint, AddName, (const char *));
+  LLDB_REGISTER_METHOD(lldb::SBError, SBBreakpoint, AddNameWithErrorHandling,
+                       (const char *));
   LLDB_REGISTER_METHOD(void, SBBreakpoint, RemoveName, (const char *));
   LLDB_REGISTER_METHOD(bool, SBBreakpoint, MatchesName, (const char *));
   LLDB_REGISTER_METHOD(void, SBBreakpoint, GetNames, (lldb::SBStringList &));
   LLDB_REGISTER_STATIC_METHOD(bool, SBBreakpoint, EventIsBreakpointEvent,
                               (const lldb::SBEvent &));
   LLDB_REGISTER_STATIC_METHOD(lldb::BreakpointEventType, SBBreakpoint,
                               GetBreakpointEventTypeFromEvent,
                               (const lldb::SBEvent &));

lldb/test/API/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py

class BreakpointNames(TestBase):
 
     def do_check_names(self):
         """Use Python APIs to check that we can set & retrieve breakpoint names"""
         bkpt = self.target.BreakpointCreateByLocation(self.main_file_spec, 10)
         bkpt_name = "ABreakpoint"
         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.")
+        success = bkpt.AddNameWithErrorHandling(bkpt_name)
+        self.assertSuccess(success, "We couldn't add a legal name to a breakpoint.")
 
         matches = bkpt.MatchesName(bkpt_name)
         self.assertTrue(matches, "We didn't match the name we just set")
 
         # Make sure we don't match irrelevant names:
         matches = bkpt.MatchesName("NotABreakpoint")
         self.assertTrue(not matches, "We matched a name we didn't set.")
 
         # Make sure the name is also in the target:
         self.check_name_in_target(bkpt_name)
 
         # Add another name, make sure that works too:
-        bkpt.AddName(other_bkpt_name)
+        bkpt.AddNameWithErrorHandling(other_bkpt_name)
 
         matches = bkpt.MatchesName(bkpt_name)
         self.assertTrue(matches, "Adding a name means we didn't match the name we just set")
         self.check_name_in_target(other_bkpt_name)
 
         # Remove the name and make sure we no longer match it:
         bkpt.RemoveName(bkpt_name)
         matches = bkpt.MatchesName(bkpt_name)
def do_check_illegal_names(self):
         """Use Python APIs to check that we reject illegal names."""
         bkpt = self.target.BreakpointCreateByLocation(self.main_file_spec, 10)
         bad_names = ["-CantStartWithADash",
                      "1CantStartWithANumber",
                      "^CantStartWithNonAlpha",
                      "CantHave-ADash",
                      "Cant Have Spaces"]
         for bad_name in bad_names:
-            success = bkpt.AddName(bad_name)
-            self.assertTrue(not success,"We allowed an illegal name: %s"%(bad_name))
+            success = bkpt.AddNameWithErrorHandling(bad_name)
+            self.assertTrue(success.Fail(),"We allowed an illegal name: %s"%(bad_name))
             bp_name = lldb.SBBreakpointName(self.target, bad_name)
             self.assertFalse(bp_name.IsValid(), "We made a breakpoint name with an illegal name: %s"%(bad_name));
 
             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 breakpoint 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.")
+        success = bkpt.AddNameWithErrorHandling(bkpt_name)
+        self.assertSuccess(success, "We couldn't add a legal name to a breakpoint.")
 
         bkpts = lldb.SBBreakpointList(self.target)
         self.target.FindBreakpointsByName(bkpt_name, bkpts)
 
         self.assertEquals(bkpts.GetSize(), 1, "One breakpoint matched.")
         found_bkpt = bkpts.GetBreakpointAtIndex(0)
         self.assertEquals(bkpt.GetID(), found_bkpt.GetID(),"The right breakpoint.")
         self.assertEquals(bkpt.GetID(), bkpt_id,"With the same ID as before.")
def do_check_configuring_names(self):
 
         # Now find the version copied in from the dummy target, and make sure these settings took:
         bp_name = lldb.SBBreakpointName(self.target, self.bp_name_string)
         self.assertTrue(bp_name.IsValid(), "Failed to make breakpoint name.")
         self.check_option_values(bp_name)
 
         # Now add this name to a breakpoint, and make sure it gets configured properly
         bkpt = self.target.BreakpointCreateByLocation(self.main_file_spec, 10)
-        success = bkpt.AddName(self.bp_name_string)
-        self.assertTrue(success, "Couldn't add this name to the breakpoint")
+        success = bkpt.AddNameWithErrorHandling(self.bp_name_string)
+        self.assertSuccess(success, "Couldn't add this name to the breakpoint")
         self.check_option_values(bkpt)
 
         # Now make a name from this breakpoint, and make sure the new name is properly configured:
         new_name = lldb.SBBreakpointName(bkpt, other_bp_name_string)
         self.assertTrue(new_name.IsValid(), "Couldn't make a valid bp_name from a breakpoint.")
         self.check_option_values(bkpt)
 
         # Now change the name's option and make sure it gets propagated to
class BreakpointNames(TestBase):
     def check_permission_results(self, bp_name):
         self.assertEqual(bp_name.GetAllowDelete(), False, "Didn't set allow delete.")
         protected_bkpt = self.target.BreakpointCreateByLocation(self.main_file_spec, 10)
         protected_id = protected_bkpt.GetID()
 
         unprotected_bkpt = self.target.BreakpointCreateByLocation(self.main_file_spec, 10)
         unprotected_id = unprotected_bkpt.GetID()
 
-        success = protected_bkpt.AddName(self.bp_name_string)
-        self.assertTrue(success, "Couldn't add this name to the breakpoint")
+        success = protected_bkpt.AddNameWithErrorHandling(self.bp_name_string)
+        self.assertSuccess(success, "Couldn't add this name to the breakpoint")
 
         self.target.DisableAllBreakpoints()
         self.assertEqual(protected_bkpt.IsEnabled(), True, "Didnt' keep breakpoint from being disabled")
         self.assertEqual(unprotected_bkpt.IsEnabled(), False, "Protected too many breakpoints from disabling.")
 
         # Try from the command line too:
         unprotected_bkpt.SetEnabled(True)
         result = lldb.SBCommandReturnObject()