[LLDB][breakpoints] ArgInfo::count -> ArgInfo::max_positional_args

[lawrence_danna]

Summary

Move breakpoints from the old, bad ArgInfo::count to the new, better
ArgInfo::max_positional_args. Soon ArgInfo::count will be no more.

It looks like this functionality is already well tested by
TestBreakpointCommandsFromPython.py, so there's no need to write
additional tests for it.

[labath]

lldb/scripts/Python/python-wrapper.swig

67

Is there any case where fetching the argument info will fail, but we still can successfully call the target object? Should we just bail out here?

[lawrence_danna]

lldb/scripts/Python/python-wrapper.swig

67

probably not? My thinking is that since there's no meaningful way to return an error from this function we may as well try to call it and let the exception get logged. But I dunno. Should I change it?

[labath]

lldb/scripts/Python/python-wrapper.swig

67

Hmm... In that case, I think it would be better to print the error which caused the ArgInfo fetching to fail. Given that, in the new way of doing things, the PyErr_Cleaner object is not going to work anyway, we might as well use this opportunity to create a different mechanism for printing exceptions.

[lawrence_danna]

improved error handling

[labath]

Looks fine, though I'd try to remove the extern "C" thingy to avoid the need to supress warnings and stuff...

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

76–79

How sure are we that these functions need to be extern "C" ? AFAICT, the only requirement is that they match the declarations in ScriptInterpreterPython.cpp. That can be easily achieved by just removing extern "C" from both declarations.

2283

Random idea: Should we make PythonException::message() include the backtrace?

[lawrence_danna]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

76–79

Looks like SWIG forces them to be extern "C".

2283

Huh, I don't know. Maybe?

lldb/include/lldb/Interpreter/ScriptInterpreter.h

virtual Status SetBreakpointCommandCallback(
       std::unique_ptr<BreakpointOptions::CommandData> &data_up) {
     Status error;
     error.SetErrorString("unimplemented");
     return error;
   }
 
   Status SetBreakpointCommandCallbackFunction(
       std::vector<BreakpointOptions *> &bp_options_vec,
-      const char *function_name, 
-      StructuredData::ObjectSP extra_args_sp);
+      const char *function_name, StructuredData::ObjectSP extra_args_sp);
 
   /// Set a script function as the callback for the breakpoint.
   virtual Status
   SetBreakpointCommandCallbackFunction(
       BreakpointOptions *bp_options,
       const char *function_name,
       StructuredData::ObjectSP extra_args_sp) {
     Status error;
public:
 
   virtual bool IsReservedWord(const char *word) { return false; }
 
   virtual std::unique_ptr<ScriptInterpreterLocker> AcquireInterpreterLock();
 
   const char *GetScriptInterpreterPtyName();
 
   int GetMasterFileDescriptor();
-  
-  virtual llvm::Expected<size_t> 
-  GetNumFixedArgumentsForCallable(const llvm::StringRef &callable_name) { 
+  virtual llvm::Expected<unsigned>
+  GetMaxPositionalArgumentsForCallable(const llvm::StringRef &callable_name) {
     return llvm::createStringError(
     llvm::inconvertibleErrorCode(), "Unimplemented function");
   }
 
   static std::string LanguageToString(lldb::ScriptLanguage language);
 
   static lldb::ScriptLanguage StringToLanguage(const llvm::StringRef &string);
 

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommandsFromPython.py

def do_set_python_command_from_python(self):
         self.process = self.target.LaunchSimple(
             None, None, self.get_process_working_directory())
 
         self.assertTrue(self.process, PROCESS_IS_VALID)
 
         # Now finish, and make sure the return value is correct.
         threads = lldbutil.get_threads_stopped_at_breakpoint(
             self.process, body_bkpt)
-        self.assertTrue(len(threads) == 1, "Stopped at inner breakpoint.")
+        self.assertEquals(len(threads), 1, "Stopped at inner breakpoint.")
         self.thread = threads[0]
 
         self.assertEquals("callback was here", side_effect.callback)
         self.assertEquals("function was here", side_effect.bktptcmd)
         self.assertEquals("I am fancy", side_effect.fancy)
         self.assertEquals("I am fancier", side_effect.fancier)
         self.assertEquals("Not so fancy", side_effect.not_so_fancy)
 

lldb/scripts/Python/python-wrapper.swig

 // foo.bar.baz.Foobar to an actual Python object
 // if pmodule is NULL, the __main__ module will be used
 // as the starting point for the search
 
 
 // This function is called by lldb_private::ScriptInterpreterPython::BreakpointCallbackFunction(...)
 // and is used when a script command is attached to a breakpoint for execution.
 
-SWIGEXPORT bool
+SWIGEXPORT llvm::Expected<bool>
 LLDBSwigPythonBreakpointCallbackFunction
 (
     const char *python_function_name,
     const char *session_dictionary_name,
     const lldb::StackFrameSP& frame_sp,
     const lldb::BreakpointLocationSP& bp_loc_sp,
     lldb_private::StructuredDataImpl *args_impl
 )
 {
+    using namespace llvm;
+
     lldb::SBFrame sb_frame (frame_sp);
     lldb::SBBreakpointLocation sb_bp_loc(bp_loc_sp);
 
-    bool stop_at_breakpoint = true;
-
     PyErr_Cleaner py_err_cleaner(true);
     auto dict = PythonModule::MainModule().ResolveName<PythonDictionary>(session_dictionary_name);
     auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(python_function_name, dict);
 
-    if (!pfunc.IsAllocated())
-        return stop_at_breakpoint;
+    unsigned max_positional_args;
+    if (auto arg_info = pfunc.GetArgInfo())
+        max_positional_args = arg_info.get().max_positional_args;
+    else
+        return arg_info.takeError();
 
     PythonObject frame_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_frame));
     PythonObject bp_loc_arg(PyRefType::Owned, SBTypeToSWIGWrapper(sb_bp_loc));
 
-    PythonObject result;
+    auto result = [&] () -> Expected<PythonObject> {
         // If the called function doesn't take extra_args, drop them here:
-    auto arg_info = pfunc.GetNumArguments();
-    if (arg_info.count == 3)
-      result = pfunc(frame_arg, bp_loc_arg, dict);
-    else if (arg_info.count == 4) {
+        if (max_positional_args < 4) {
+            return pfunc.Call(frame_arg, bp_loc_arg, dict);
+        } else {
             lldb::SBStructuredData *args_value = new lldb::SBStructuredData(args_impl);
             PythonObject args_arg(PyRefType::Owned, SBTypeToSWIGWrapper(args_value));
-        result = pfunc(frame_arg, bp_loc_arg, args_arg, dict);      
-    } else {
-      return stop_at_breakpoint;
+            return pfunc.Call(frame_arg, bp_loc_arg, args_arg, dict);
         }
+    } ();
 
-    if (result.get() == Py_False)
-        stop_at_breakpoint = false;
+    if (!result)
+        return result.takeError();
 
-    return stop_at_breakpoint;
+    // Only False counts as false!
+    return result.get().get() != Py_False;
 }
 
 // This function is called by lldb_private::ScriptInterpreterPython::WatchpointCallbackFunction(...)
 // and is used when a script command is attached to a watchpoint for execution.
 
 SWIGEXPORT bool
 LLDBSwigPythonWatchpointCallbackFunction
 (

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

 extern "C" void init_lldb(void);
 
 #define LLDBSwigPyInit init_lldb
 #endif
 
 // These prototypes are the Pythonic implementations of the required callbacks.
 // Although these are scripting-language specific, their definition depends on
 // the public API.
-extern "C" bool LLDBSwigPythonBreakpointCallbackFunction(
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
+
+extern "C" llvm::Expected<bool> LLDBSwigPythonBreakpointCallbackFunction(
     const char *python_function_name, const char *session_dictionary_name,
     const lldb::StackFrameSP &sb_frame,
-    const lldb::BreakpointLocationSP &sb_bp_loc,
-    StructuredDataImpl *args_impl);
+    const lldb::BreakpointLocationSP &sb_bp_loc, StructuredDataImpl *args_impl);
+
+#pragma clang diagnostic pop
 
 extern "C" bool LLDBSwigPythonWatchpointCallbackFunction(
     const char *python_function_name, const char *session_dictionary_name,
     const lldb::StackFrameSP &sb_frame, const lldb::WatchpointSP &sb_wp);
 
 extern "C" bool LLDBSwigPythonCallTypeScript(
     const char *python_function_name, void *session_dictionary,
     const lldb::ValueObjectSP &valobj_sp, void **pyfunct_wrapper,

 PythonDictionary &ScriptInterpreterPythonImpl::GetSysModuleDictionary() {
   if (m_sys_module_dict.IsValid())
     return m_sys_module_dict;
   PythonModule sys_module = unwrapIgnoringErrors(PythonModule::Import("sys"));
   m_sys_module_dict = sys_module.GetDictionary();
   return m_sys_module_dict;
 }
 
-llvm::Expected<size_t>
-ScriptInterpreterPythonImpl::GetNumFixedArgumentsForCallable(
-    const llvm::StringRef &callable_name)
-{
+llvm::Expected<unsigned>
+ScriptInterpreterPythonImpl::GetMaxPositionalArgumentsForCallable(
+    const llvm::StringRef &callable_name) {
   if (callable_name.empty()) {
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),
         "called with empty callable name.");
   }
   Locker py_lock(this, Locker::AcquireLock |
                  Locker::InitSession |
                  Locker::NoSTDIN);
   auto dict = PythonModule::MainModule()
       .ResolveName<PythonDictionary>(m_dictionary_name);
-  auto pfunc 
-     = PythonObject::ResolveNameWithDictionary<PythonCallable>(callable_name, 
-                                                               dict);
+  auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
+      callable_name, dict);
   if (!pfunc.IsAllocated()) {
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),
         "can't find callable: %s", callable_name.str().c_str());
   }
   PythonCallable::ArgInfo arg_info = pfunc.GetNumArguments();
-  return arg_info.count;
+  return arg_info.max_positional_args;
 }
 
 static std::string GenerateUniqueName(const char *base_name_wanted,
                                       uint32_t &functions_counter,
                                       const void *name_token = nullptr) {
   StreamString sstr;
 
   if (!base_name_wanted)

 
 Status ScriptInterpreterPythonImpl::SetBreakpointCommandCallbackFunction(
     BreakpointOptions *bp_options, const char *function_name,
     StructuredData::ObjectSP extra_args_sp) {
   Status error;
   // For now just cons up a oneliner that calls the provided function.
   std::string oneliner("return ");
   oneliner += function_name;
-  
-  llvm::Expected<size_t> maybe_args 
-      = GetNumFixedArgumentsForCallable(function_name);
+  llvm::Expected<unsigned> maybe_args =
+      GetMaxPositionalArgumentsForCallable(function_name);
   if (!maybe_args) {
-    error.SetErrorStringWithFormat("could not get num args: %s", 
+    error.SetErrorStringWithFormat(
+        "could not get num args: %s",
         llvm::toString(maybe_args.takeError()).c_str());
     return error;
   }
-  size_t num_args = *maybe_args;
-  
+  size_t max_args = *maybe_args;
   bool uses_extra_args = false;
-  if (num_args == 4) {
+  if (max_args >= 4) {
     uses_extra_args = true;
     oneliner += "(frame, bp_loc, extra_args, internal_dict)";
-  }
-  else if (num_args == 3) {
+  } else if (max_args >= 3) {
     if (extra_args_sp) {
       error.SetErrorString("cannot pass extra_args to a three argument callback"
                           );
       return error;
     }
     uses_extra_args = false;
     oneliner += "(frame, bp_loc, internal_dict)";
   } else {
     error.SetErrorStringWithFormat("expected 3 or 4 argument "
-                                   "function, %s has %zu",
-                                   function_name, num_args);
+                                   "function, %s can only take %zu",
+                                   function_name, max_args);
     return error;
   }
-  
-  SetBreakpointCommandCallback(bp_options, oneliner.c_str(), extra_args_sp, 
+  SetBreakpointCommandCallback(bp_options, oneliner.c_str(), extra_args_sp,
                                uses_extra_args);
   return error;
 }
 
 Status ScriptInterpreterPythonImpl::SetBreakpointCommandCallback(
     BreakpointOptions *bp_options,
     std::unique_ptr<BreakpointOptions::CommandData> &cmd_data_up) {
   Status error;
if (py_return.get()) {
     PythonDictionary result_dict(PyRefType::Borrowed, py_return.get());
     return result_dict.CreateStructuredDictionary();
   }
   return StructuredData::DictionarySP();
 }
 
 StructuredData::ObjectSP ScriptInterpreterPythonImpl::CreateScriptedThreadPlan(
     const char *class_name, StructuredDataImpl *args_data,
-    std::string &error_str, 
-    lldb::ThreadPlanSP thread_plan_sp) {
+    std::string &error_str, lldb::ThreadPlanSP thread_plan_sp) {
   if (class_name == nullptr || class_name[0] == '\0')
     return StructuredData::ObjectSP();
 
   if (!thread_plan_sp.get())
     return {};
 
   Debugger &debugger = thread_plan_sp->GetTarget().GetDebugger();
   ScriptInterpreter *script_interpreter = debugger.GetScriptInterpreter();
Status ScriptInterpreterPythonImpl::GenerateBreakpointCommandCallbackData(
   Status error;
   if (user_input.GetSize() == 0) {
     error.SetErrorString("No input data.");
     return error;
   }
 
   std::string auto_generated_function_name(GenerateUniqueName(
       "lldb_autogen_python_bp_callback_func_", num_created_functions));
-  if (has_extra_args) 
+  if (has_extra_args)
     sstr.Printf("def %s (frame, bp_loc, extra_args, internal_dict):",
                 auto_generated_function_name.c_str());
   else
     sstr.Printf("def %s (frame, bp_loc, internal_dict):",
                 auto_generated_function_name.c_str());
 
   error = GenerateFunction(sstr.GetData(), user_input);
   if (!error.Success())
if (breakpoint_sp) {
           breakpoint_sp->FindLocationByID(break_loc_id));
 
       if (stop_frame_sp && bp_loc_sp) {
         bool ret_val = true;
         {
           Locker py_lock(python_interpreter, Locker::AcquireLock |
                                                  Locker::InitSession |
                                                  Locker::NoSTDIN);
-          ret_val = LLDBSwigPythonBreakpointCallbackFunction(
+          Expected<bool> maybe_ret_val =
+              LLDBSwigPythonBreakpointCallbackFunction(
                   python_function_name,
                   python_interpreter->m_dictionary_name.c_str(), stop_frame_sp,
-              bp_loc_sp,
-              bp_option_data->m_extra_args_up.get());
+                  bp_loc_sp, bp_option_data->m_extra_args_up.get());
+
+          if (!maybe_ret_val) {
+
+            llvm::handleAllErrors(
+                maybe_ret_val.takeError(),
+                [&](PythonException &E) {
+                  debugger.GetErrorStream() << E.ReadBacktrace();
+                },
+                [&](const llvm::ErrorInfoBase &E) {
+                  debugger.GetErrorStream() << E.message();
+                });
+
+          } else {
+            ret_val = maybe_ret_val.get();
+          }
         }
         return ret_val;
       }
     }
   }
   // We currently always true so we stop in case anything goes wrong when
   // trying to call the script function
   return true;

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

Status SetBreakpointCommandCallbackFunction(
       const char *function_name,
       StructuredData::ObjectSP extra_args_sp) override;
 
   /// This one is for deserialization:
   Status SetBreakpointCommandCallback(
       BreakpointOptions *bp_options,
       std::unique_ptr<BreakpointOptions::CommandData> &data_up) override;
 
-  Status SetBreakpointCommandCallback(
-      BreakpointOptions *bp_options, 
+  Status SetBreakpointCommandCallback(BreakpointOptions *bp_options,
                                       const char *command_body_text,
                                       StructuredData::ObjectSP extra_args_sp,
                                       bool uses_extra_args);
 
   /// Set a one-liner as the callback for the watchpoint.
   void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
                                     const char *oneliner) override;
 
   const char *GetDictionaryName() { return m_dictionary_name.c_str(); }
 
   PyThreadState *GetThreadState() { return m_command_thread_state; }
enum ActiveIOHandler {
     eIOHandlerWatchpoint
   };
 
   python::PythonModule &GetMainModule();
 
   python::PythonDictionary &GetSessionDictionary();
 
   python::PythonDictionary &GetSysModuleDictionary();
-  
-  llvm::Expected<size_t> 
-  GetNumFixedArgumentsForCallable(const llvm::StringRef &callable_name) 
-      override;
+  llvm::Expected<unsigned> GetMaxPositionalArgumentsForCallable(
+      const llvm::StringRef &callable_name) override;
 
   bool GetEmbeddedInterpreterModuleObjects();
 
   bool SetStdHandle(lldb::FileSP file, const char *py_name,
                     python::PythonObject &save_file, const char *mode);
 
   python::PythonObject m_saved_stdin;
   python::PythonObject m_saved_stdout;

lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

 #if PY_MAJOR_VERSION >= 3
 extern "C" PyObject *PyInit__lldb(void) { return nullptr; }
 #define LLDBSwigPyInit PyInit__lldb
 #else
 extern "C" void init_lldb(void) {}
 #define LLDBSwigPyInit init_lldb
 #endif
 
-extern "C" bool LLDBSwigPythonBreakpointCallbackFunction(
+extern "C" llvm::Expected<bool> LLDBSwigPythonBreakpointCallbackFunction(
     const char *python_function_name, const char *session_dictionary_name,
     const lldb::StackFrameSP &sb_frame,
     const lldb::BreakpointLocationSP &sb_bp_loc,
     StructuredDataImpl *args_impl) {
   return false;
 }
 
 extern "C" bool LLDBSwigPythonWatchpointCallbackFunction(