[lldb/crashlog] Surface error using SBCommandReturnObject argument

[mib]

Summary

This patch allows the crashlog script to surface its errors to lldb by
using the provided SBCommandReturnObject argument.

rdar://95048193

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

[JDevlieghere]

lldb/examples/python/crashlog.py

1011–1014

Can we do this with a try-catch + an exception?

[kastiglione]

lldb/examples/python/crashlog.py

1012–1014

Is this error function needed? SetError prepends the string "error: ".

[mib]

lldb/examples/python/crashlog.py

1011–1014

@JDevlieghere As mentioned on D129611, if we raise an exception we won't be able to surface the error to lldb or even to IDEs. I think it's better to use the SBCommandReturnObject

@kastiglione true! I'll remove the "error: " prefix, but I think it's reasonable to keep this helper function to avoid code duplication.

[JDevlieghere]

lldb/examples/python/crashlog.py

1011–1014

Couldn't you still raise the error where it happens and then catch it and then take it's value and put it into the SBCommandReturnObject?

[mib]

Address @JDevlieghere comments:

• Use exception approach / Remove error helper function
• Add test

[kastiglione]

lgtm

lldb/examples/python/crashlog.py

1251–1253

minor but I think this can be a single statement: result.SetError(str(e))

[JDevlieghere]

lldb/examples/python/crashlog.py

1012–1014

Rather than raising a generic Exception, we should define our own. Now there's no way to differentiate between these issues and any other exception that was thrown. Maybe that's the goal, in which case you can still catch the generic Exception, but by defining our own we leave the option open to be more specific.

[mib]

• Add custom InteractiveCrashLogException
• Simplify the except block

[JDevlieghere]

LGTM

lldb/examples/python/crashlog.py

else:
         result.PutCString("error: invalid target")
 
 
 class Symbolicate:
     def __init__(self, debugger, internal_dict):
         pass
 
     def __call__(self, debugger, command, exe_ctx, result):
-        SymbolicateCrashLogs(debugger, shlex.split(command))
+        SymbolicateCrashLogs(debugger, shlex.split(command), result)
 
     def get_short_help(self):
         return "Symbolicate one or more darwin crash log files."
 
     def get_long_help(self):
         option_parser = CrashLogOptionParser()
         return option_parser.format_help()
 
for thread in crash_log.threads:
         thread.dump_symbolicated(crash_log, options)
         print()
 
     if crash_log.errors:
         print("Errors:")
         for error in crash_log.errors:
             print(error)
 
-def load_crashlog_in_scripted_process(debugger, crash_log_file, options):
-    result = lldb.SBCommandReturnObject()
-
+def load_crashlog_in_scripted_process(debugger, crash_log_file, options, result):
     crashlog_path = os.path.expanduser(crash_log_file)
     if not os.path.exists(crashlog_path):
-        result.PutCString("error: crashlog file %s does not exist" % crashlog_path)
+        raise Exception("crashlog file %s does not exist" % crashlog_path)
 
     crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
 
     target = lldb.SBTarget()
     # 1. Try to use the user-provided target
     if options.target_path:
         target = debugger.CreateTarget(options.target_path)
         if not target:
-            result.PutCString("error: couldn't create target provided by the user ({option.target_path})")
-            return
+            raise Exception(f"couldn't create target provided by the user ({options.target_path})")
+
     # 2. If the user didn't provide a target, try to create a target using the symbolicator
     if not target or not target.IsValid():
         target = crashlog.create_target()
     # 3. If that didn't work, and a target is already loaded, use it
     if (target is None  or not target.IsValid()) and debugger.GetNumTargets() > 0:
         target = debugger.GetTargetAtIndex(0)
     # 4. Fail
     if target is None or not target.IsValid():
-        result.PutCString("error: couldn't create target")
-        return
+        raise Exception("couldn't create target")
 
     ci = debugger.GetCommandInterpreter()
     if not ci:
-        result.PutCString("error: couldn't get command interpreter")
-        return
+        raise Exception("couldn't get command interpreter")
 
-    res = lldb.SBCommandReturnObject()
-    ci.HandleCommand('script from lldb.macosx import crashlog_scripted_process', res)
-    if not res.Succeeded():
-        result.PutCString("error: couldn't import crashlog scripted process module")
-        return
+    ci.HandleCommand('script from lldb.macosx import crashlog_scripted_process', result)
+    if not result.Succeeded():
+        raise Exception("couldn't import crashlog scripted process module")
 
     structured_data = lldb.SBStructuredData()
     structured_data.SetFromJSON(json.dumps({ "crashlog_path" : crashlog_path,
                                              "load_all_images": options.load_all_images }))
     launch_info = lldb.SBLaunchInfo(None)
     launch_info.SetProcessPluginName("ScriptedProcess")
     launch_info.SetScriptedProcessClassName("crashlog_scripted_process.CrashLogScriptedProcess")
     launch_info.SetScriptedProcessDictionary(structured_data)
     error = lldb.SBError()
     process = target.Launch(launch_info, error)
 
     if not process or error.Fail():
-        return
+        raise Exception("couldn't launch Scripted Process", error)
 
     @contextlib.contextmanager
     def synchronous(debugger):
         async_state = debugger.GetAsync()
         debugger.SetAsync(False)
         try:
             yield
         finally:

 for the first frame of the crashed thread.
 If this script is imported into the LLDB command interpreter, a "crashlog" command will be added to the interpreter
 for use at the LLDB command line. After a crash log has been parsed and symbolicated, a target will have been
 created that has all of the shared libraries loaded at the load addresses found in the crash log file. This allows
 you to explore the program as if it were stopped at the locations described in the crash log and functions can
 be disassembled and lookups can be performed using the addresses found in the crash log.'''
     return CreateSymbolicateCrashLogOptions('crashlog', description, True)
 
-def SymbolicateCrashLogs(debugger, command_args):
+def SymbolicateCrashLogs(debugger, command_args, result):
     option_parser = CrashLogOptionParser()
 
     if not len(command_args):
         option_parser.print_help()
         return
 
     try:
         (options, args) = option_parser.parse_args(command_args)
def should_run_in_interactive_mode(options, ci):
         else:
             return False
 
     ci = debugger.GetCommandInterpreter()
 
     if args:
         for crash_log_file in args:
             if should_run_in_interactive_mode(options, ci):
+                try:
                     load_crashlog_in_scripted_process(debugger, crash_log_file,
-                                                  options)
+                                                      options, result)
+                except Exception as e:
+                    err = lldb.SBError()
+                    err.SetErrorString(str(e))
+                    result.SetError(err)
             else:
                 crash_log = CrashLogParser().parse(debugger, crash_log_file, options.verbose)
-                SymbolicateCrashLog(crash_log, options)
+                SymbolicateCrashLog(crash_log, options, result)
 
 if __name__ == '__main__':
     # Create a new debugger instance
     debugger = lldb.SBDebugger.Create()
-    SymbolicateCrashLogs(debugger, sys.argv[1:])
+    result = lldb.SBCommandReturnObject()
+    SymbolicateCrashLogs(debugger, sys.argv[1:], result)
     lldb.SBDebugger.Destroy(debugger)
 
 def __lldb_init_module(debugger, internal_dict):
     debugger.HandleCommand(
         'command script add -c lldb.macosx.crashlog.Symbolicate crashlog')
     debugger.HandleCommand(
         'command script add -f lldb.macosx.crashlog.save_crashlog save_crashlog')
     print('"crashlog" and "save_crashlog" commands have been installed, use '
           'the "--help" options on these commands for detailed help.')

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

std::string err_msg;
     std::string args_str = args.str();
     ret_val = LLDBSwigPythonCallCommand(
         impl_function, m_dictionary_name.c_str(), debugger_sp, args_str.c_str(),
         cmd_retobj, exe_ctx_ref_sp);
   }
 
   if (!ret_val)
     error.SetErrorString("unable to execute script function");
-  else
-    error.Clear();
+  else if (cmd_retobj.GetStatus() == eReturnStatusFailed)
+    return false;
 
+  error.Clear();
   return ret_val;
 }
 
 bool ScriptInterpreterPythonImpl::RunScriptBasedCommand(
     StructuredData::GenericSP impl_obj_sp, llvm::StringRef args,
     ScriptedCommandSynchronicity synchronicity,
     lldb_private::CommandReturnObject &cmd_retobj, Status &error,
     const lldb_private::ExecutionContext &exe_ctx) {
std::string err_msg;
     std::string args_str = args.str();
     ret_val = LLDBSwigPythonCallCommandObject(
         static_cast<PyObject *>(impl_obj_sp->GetValue()), debugger_sp,
         args_str.c_str(), cmd_retobj, exe_ctx_ref_sp);
   }
 
   if (!ret_val)
     error.SetErrorString("unable to execute script function");
-  else
-    error.Clear();
+  else if (cmd_retobj.GetStatus() == eReturnStatusFailed)
+    return false;
 
+  error.Clear();
   return ret_val;
 }
 
 /// In Python, a special attribute __doc__ contains the docstring for an object
 /// (function, method, class, ...) if any is defined Otherwise, the attribute's
 /// value is None.
 bool ScriptInterpreterPythonImpl::GetDocumentationForItem(const char *item,
                                                           std::string &dest) {

lldb/test/Shell/ScriptInterpreter/Python/Crashlog/interactive_crashlog_invalid_target.test

+# REQUIRES: python, native && target-aarch64 && system-darwin
+
+# RUN: %lldb -o 'command script import lldb.macosx.crashlog' \
+# RUN: -o 'crashlog -a -i -t /this_file_does_not_exist %S/Inputs/interactive_crashlog/multithread-test.ips' 2>&1 | FileCheck %s
+
+# CHECK: "crashlog" {{.*}} commands have been installed, use the "--help" options on these commands
+
+# CHECK: error: couldn't create target provided by the user (/this_file_does_not_exist)