[lldb] Add debugger.external-editor setting

[JDevlieghere]

Summary

Add a setting (debugger.external-editor) to specify an external editor instead of having to rely on the LLDB_EXTERNAL_EDITOR environment variable.

[mib]

lldb/include/lldb/Core/Debugger.h

298

newline

lldb/source/Host/macosx/objcxx/Host.mm

394–395

Should we even check for the env variable is the user set the editor setting ?

[JDevlieghere]

lldb/include/lldb/Core/Debugger.h

298

This was intentional but I'll back it out of this patch and clean up everything in a separate commit.

lldb/source/Host/macosx/objcxx/Host.mm

394–395

I asked that question in D149472 and this was Jim's suggestion: https://reviews.llvm.org/D149472#inline-1443754

[mib]

LGTM!

[bulbazord]

lldb/source/Host/macosx/objcxx/Host.mm

394–395

This is a hard error and not a warning, which I don't think is a good idea. I think the point of a setting here is to be able to change behavior during a debugging session instead of needing to start over. If these 2 values conflict, I would have to restart my session to unset LLDB_EXTERNAL_EDITOR to actually get the desired behavior.

[JDevlieghere]

Make external-editor setting take precedence over LLDB_EXTRENAL_EDITOR

[bulbazord]

LGTM!

lldb/include/lldb/Core/Debugger.h

public:
   bool SetTerminalWidth(uint32_t term_width);
 
   llvm::StringRef GetPrompt() const;
 
   void SetPrompt(llvm::StringRef p);
   void SetPrompt(const char *) = delete;
 
   bool GetUseExternalEditor() const;
-
   bool SetUseExternalEditor(bool use_external_editor_p);
 
+  llvm::StringRef GetExternalEditor() const;
+  bool SetExternalEditor(llvm::StringRef editor);
+
   bool GetUseColor() const;
 
   bool SetUseColor(bool use_color);
 
   bool GetShowProgress() const;
 
   bool SetShowProgress(bool show_progress);
 

lldb/include/lldb/Host/Host.h

public:
   /// \arg hide_stderr If this is \b false, redirect stderr to stdout
   static Status RunShellCommand(llvm::StringRef shell, const Args &args,
                                 const FileSpec &working_dir, int *status_ptr,
                                 int *signo_ptr, std::string *command_output,
                                 const Timeout<std::micro> &timeout,
                                 bool run_in_shell = true,
                                 bool hide_stderr = false);
 
-  static llvm::Error OpenFileInExternalEditor(const FileSpec &file_spec,
+  static llvm::Error OpenFileInExternalEditor(llvm::StringRef editor,
+                                              const FileSpec &file_spec,
                                               uint32_t line_no);
 
   /// Check if we're running in an interactive graphical session.
   ///
   /// \return
   ///     True if we're running in an interactive graphical session. False if
   ///     we're not or don't know.
   static bool IsInteractiveGraphicSession();

lldb/source/Core/CoreProperties.td

let Definition = "debugger" in {
   def ThreadStopFormat: Property<"thread-stop-format", "FormatEntity">,
     Global,
     DefaultStringValue<"thread #${thread.index}{, name = '${thread.name}'}{, queue = ${ansi.fg.green}'${thread.queue}'${ansi.normal}}{, activity = ${ansi.fg.green}'${thread.info.activity.name}'${ansi.normal}}{, ${thread.info.trace_messages} messages}{, stop reason = ${ansi.fg.red}${thread.stop-reason}${ansi.normal}}{\\\\nReturn value: ${thread.return-value}}{\\\\nCompleted expression: ${thread.completed-expression}}\\\\n">,
     Desc<"The default thread format  string to use when displaying thread information as part of the stop display.">;
   def UseExternalEditor: Property<"use-external-editor", "Boolean">,
     Global,
     DefaultFalse,
     Desc<"Whether to use an external editor or not.">;
+  def ExternalEditor: Property<"external-editor", "String">,
+    Global,
+    DefaultStringValue<"">,
+    Desc<"External editor to use when use-external-editor is enabled.">;
   def UseColor: Property<"use-color", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"Whether to use Ansi color codes or not.">;
   def ShowProgress: Property<"show-progress", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"Whether to show progress or not if the debugger's output is an interactive color-enabled terminal.">;

lldb/source/Core/Debugger.cpp

return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
 }
 
 bool Debugger::SetUseExternalEditor(bool b) {
   const uint32_t idx = ePropertyUseExternalEditor;
   return m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
 }
 
+llvm::StringRef Debugger::GetExternalEditor() const {
+  const uint32_t idx = ePropertyExternalEditor;
+  return m_collection_sp->GetPropertyAtIndexAsString(nullptr, idx, "");
+}
+
+bool Debugger::SetExternalEditor(llvm::StringRef editor) {
+  const uint32_t idx = ePropertyExternalEditor;
+  return m_collection_sp->SetPropertyAtIndexAsString(nullptr, idx, editor);
+}
+
 bool Debugger::GetUseColor() const {
   const uint32_t idx = ePropertyUseColor;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_debugger_properties[idx].default_uint_value != 0);
 }
 
 bool Debugger::SetUseColor(bool b) {
   const uint32_t idx = ePropertyUseColor;

lldb/source/Host/common/Host.cpp

 #endif // !defined(__APPLE__)
 
 #ifndef _WIN32
 void Host::Kill(lldb::pid_t pid, int signo) { ::kill(pid, signo); }
 
 #endif
 
 #if !defined(__APPLE__)
-llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+llvm::Error Host::OpenFileInExternalEditor(llvm::StringRef editor,
+                                           const FileSpec &file_spec,
                                            uint32_t line_no) {
   return llvm::errorCodeToError(
       std::error_code(ENOTSUP, std::system_category()));
 }
 
 bool Host::IsInteractiveGraphicSession() { return false; }
 #endif
 

lldb/source/Host/macosx/objcxx/Host.mm

LaunchInNewTerminalWithAppleScript(const char *exe_path,
   [applescript release];
   if (pid != LLDB_INVALID_PROCESS_ID)
     launch_info.SetProcessID(pid);
   return error;
 }
 
 #endif // TARGET_OS_OSX
 
-llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+llvm::Error Host::OpenFileInExternalEditor(llvm::StringRef editor,
+                                           const FileSpec &file_spec,
                                            uint32_t line_no) {
 #if !TARGET_OS_OSX
   return llvm::errorCodeToError(
       std::error_code(ENOTSUP, std::system_category()));
 #else // !TARGET_OS_OSX
   Log *log = GetLog(LLDBLog::Host);
 
   const std::string file_path = file_spec.GetPath();
if (error != noErr)
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),
         llvm::formatv("creating Apple Event descriptor failed: error {0}",
                       error));
 
   // Deallocate the descriptor on exit.
   auto on_exit = llvm::make_scope_exit(
       [&]() { AEDisposeDesc(&(file_and_line_desc.descContent)); });
 
-  static std::optional<FSRef> g_app_fsref;
-  static std::string g_app_error;
-  static std::once_flag g_once_flag;
-  std::call_once(g_once_flag, [&]() {
-    if (const char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
-      LLDB_LOG(log, "Looking for external editor: {0}", external_editor);
+  if (editor.empty()) {
+    if (const char *lldb_external_editor = ::getenv("LLDB_EXTERNAL_EDITOR"))
+      editor = lldb_external_editor;
+  }
+
+  std::optional<FSRef> app_fsref;
+  if (!editor.empty()) {
+    LLDB_LOG(log, "Looking for external editor: {0}", editor);
 
-      FSRef app_fsref;
-      CFCString editor_name(external_editor, kCFStringEncodingUTF8);
-      long app_error = ::LSFindApplicationForInfo(
-          /*inCreator=*/kLSUnknownCreator, /*inBundleID=*/NULL,
-          /*inName=*/editor_name.get(), /*outAppRef=*/&app_fsref,
-          /*outAppURL=*/NULL);
-      if (app_error == noErr) {
-        g_app_fsref = app_fsref;
-      } else {
-        g_app_error =
-            llvm::formatv("could not find external editor \"{0}\": "
-                          "LSFindApplicationForInfo returned error {1}",
-                          external_editor, app_error)
-                .str();
-      }
-    }
-  });
-
-  if (!g_app_error.empty())
-    return llvm::createStringError(llvm::inconvertibleErrorCode(), g_app_error);
+    app_fsref.emplace();
+    CFCString editor_name(editor.data(), kCFStringEncodingUTF8);
+        /*inName=*/editor_name.get(), /*outAppRef=*/&(*app_fsref),
+    if (app_error != noErr)
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+                        editor, app_error));
 
   // Build app launch parameters.
   LSApplicationParameters app_params;
   ::memset(&app_params, 0, sizeof(app_params));
   app_params.flags =
       kLSLaunchDefaults | kLSLaunchDontAddToRecents | kLSLaunchDontSwitch;
-  if (g_app_fsref)
-    app_params.application = &(g_app_fsref.value());
+  if (app_fsref)
+    app_params.application = &(*app_fsref);
 
   ProcessSerialNumber psn;
   std::array<CFURLRef, 1> file_array = {file_URL.get()};
   CFCReleaser<CFArrayRef> cf_array(
       CFArrayCreate(/*allocator=*/NULL, /*values=*/(const void **)&file_array,
                     /*numValues*/ 1, /*callBacks=*/NULL));
   error = ::LSOpenURLsWithRole(
       /*inURLs=*/cf_array.get(), /*inRole=*/kLSRolesEditor,

lldb/source/Interpreter/CommandInterpreter.cpp

bool CommandInterpreter::SaveTranscript(
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
   result.AppendMessageWithFormat("Session's transcripts saved to %s\n",
                                  output_file->c_str());
 
   if (GetOpenTranscriptInEditor() && Host::IsInteractiveGraphicSession()) {
     const FileSpec file_spec;
     error = file->GetFileSpec(const_cast<FileSpec &>(file_spec));
     if (error.Success()) {
-      if (llvm::Error e = Host::OpenFileInExternalEditor(file_spec, 1))
+      if (llvm::Error e = Host::OpenFileInExternalEditor(
+              m_debugger.GetExternalEditor(), file_spec, 1))
         result.AppendError(llvm::toString(std::move(e)));
     }
   }
 
   return true;
 }
 
 bool CommandInterpreter::IsInteractive() {

lldb/source/Target/Thread.cpp

bool Thread::SetSelectedFrameByIndexNoisily(uint32_t frame_idx,
   const bool broadcast = true;
   bool success = SetSelectedFrameByIndex(frame_idx, broadcast);
   if (success) {
     StackFrameSP frame_sp = GetSelectedFrame(DoNoSelectMostRelevantFrame);
     if (frame_sp) {
       bool already_shown = false;
       SymbolContext frame_sc(
           frame_sp->GetSymbolContext(eSymbolContextLineEntry));
-      if (GetProcess()->GetTarget().GetDebugger().GetUseExternalEditor() &&
-          frame_sc.line_entry.file && frame_sc.line_entry.line != 0) {
+      const Debugger &debugger = GetProcess()->GetTarget().GetDebugger();
+      if (debugger.GetUseExternalEditor() && frame_sc.line_entry.file &&
+          frame_sc.line_entry.line != 0) {
         if (llvm::Error e = Host::OpenFileInExternalEditor(
-                frame_sc.line_entry.file, frame_sc.line_entry.line)) {
+                debugger.GetExternalEditor(), frame_sc.line_entry.file,
+                frame_sc.line_entry.line)) {
           LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
                          "OpenFileInExternalEditor failed: {0}");
         } else {
           already_shown = true;
         }
       }
 
       bool show_frame_info = true;
if (!only_stacks) {
     strm.Printf("%c ", is_selected ? '*' : ' ');
     if (target && target->GetDebugger().GetUseExternalEditor()) {
       StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
       if (frame_sp) {
         SymbolContext frame_sc(
             frame_sp->GetSymbolContext(eSymbolContextLineEntry));
         if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
           if (llvm::Error e = Host::OpenFileInExternalEditor(
+                  target->GetDebugger().GetExternalEditor(),
                   frame_sc.line_entry.file, frame_sc.line_entry.line)) {
             LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
                            "OpenFileInExternalEditor failed: {0}");
           }
         }
       }
     }
 

lldb/test/Shell/Settings/TestExternalEditor.test

+REQUIRES: system-darwin
+RUN: %lldb -o 'settings set use-external-editor true' -o 'setting set external-editor foo' -o 'session save' -b 2>&1 | FileCheck %s
+RUN: LLDB_EXTERNAL_EDITOR="foo" %lldb -o 'settings set use-external-editor true' -o 'session save' -b 2>&1 | FileCheck %s
+CHECK: error: could not find external editor "foo": LSFindApplicationForInfo returned error