[lldb] Refactor host::OpenFileInExternalEditor

[JDevlieghere]

Summary

This patch refactors the macOS implementation of OpenFileInExternalEditor:

• Fix AppleEvent memory leak
• Fix caching of LLDB_EXTERNAL_EDITOR
• Fix (speculatively) crash when CFURL is NULL.
• Improve readability
• Improve documentation
• Improve logging

rdar://108633464

PS: A bunch of the Launch Services APIs have been deprecated (LSFindApplicationForInfo, LSOpenURLsWithRole). The preferred API is LSOpenCFURLRef but it doesn't allow you to specify the "location" Apple Event which is used to highlight the current line. Interestingly enough, both Xcode (the default) and TextEdit both ignore that. Sublime Text on the other hand does honor it. Since the old APIs are deprecated but still available, I prefer not to regress the current functionality.

[bulbazord]

LGTM overall. A few nits but overall an excellent cleanup! :)

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

337

nit: Move log line below checking if the file_path is empty? If the file_spec is empty we may see strange log lines like "Sending :0 to external editor" which is just noise.

343–344

Could you document what the NULL and false refer to? I think they're for allocator and isDirectory or something like that.

419–420

I know you're preserving existing behavior here (or rather, fixing its faulty implementation), but I think it would be nice if you didn't have to restart your session and add an environment variable to get this working with an existing debug session. Or maybe make it a setting?

425–427

Should we exit early if we don't have g_app_fsref filled in? The application field of app_params won't be filled in so what will LSOpenURLsWithRole do?

[mib]

LGTM with some comments

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

335

I guess this doesn't need to be a const std::string, a llvm::StringRef would have been nice but looking at SBFileSpec::GetPath, there is unfortunately no overload for that.

417–418

why is this static ?

[JDevlieghere]

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

337

I actually did that on purpose, so we could tell from the logs that it's empty. It didn't seem worth adding a whole separate log line for, but if you think :10 looks weird, I'm happy to do it.

419–420

I think we're trying to mimic the EDITOR and VISUAL environment variables but I agree a setting would be better. I can add that in a follow-up. Which one should take preference if you have both?

425–427

No, the application is optional. If none is specified we use the default. You can't pass a default constructor one though, it has to be NULL, which is why we have the check here.

[JDevlieghere]

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

335

Given how FileSpecs stores path components, you basically always need an owning string.

417–418

Because it is cached across calls.

[mib]

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

343–344

+1

425–427

Looking at the documentation (https://developer.apple.com/documentation/coreservices/1448184-lsopenurlswithrole), if app_params.application is NULL, LSOpenURLsWithRole makes use of kLSRolesAll (https://developer.apple.com/documentation/coreservices/lsrolesmask?language=objc) which I believe open the default application for that file.

[mib]

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

433

I guess we could narrow down the inRole argument: https://developer.apple.com/documentation/coreservices/lsrolesmask/klsroleseditor?language=objc`

[jingham]

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

425–427

LSOpenURLsWithRole will use the default application for the extension that the file path we pass it has, however, that's likely to be a different app than the one the user dialed up with LLDB_EXTERNAL_EDITOR.

If the user told us to use an external editor at some path, but we can't actually find that application, then letting the default application open seems wrong. Ideally, we would raise an error saying "External Editor: ... not found" but it doesn't look like there's any way to report errors from this code.

[bulbazord]

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

337

I think a separate log line would be easier to read for somebody not familiar with this codepath. It would be confusing otherwise. Something like "OpenFileInExternalEditor called with empty path"? Or maybe you can keep the log line but change it to:

LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path.empty() ? "<invalid>" : file_path, line_no);

419–420

I would say a setting should probably take priority? That way you can change it during your session if desired.

[jingham]

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

419–420

If both are set and not the same, maybe we should flag that. Maybe we can check when the setting is set, and if the environment variable is also set and different, we can warn at that point.

But the intention of the setting is to change what the current behavior is, no matter how we got to the current behavior, so the setting should take precedence.

[JDevlieghere]

• Return an llvm::Erorr and percolate errors up
• Don't fall back to the default editor if the one specified can't be found
• Limit roles to editors only

[bulbazord]

LGTM! Thanks for cleaning this up.

lldb/source/Target/Thread.cpp

1762–1763

Not related to this change but I don't think I like LLDB_LOG_ERROR... I read this and I think "Oh, if logging is disabled, will this actually consume the error?". After all, the other logging macros do not have a side effect. It'd be nice if we could create a function and name it "ConsumeErrorAndLog" or something to be clearer that it actually consumes it.

[JDevlieghere]

13dbc16b4d82

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 bool OpenFileInExternalEditor(const FileSpec &file_spec,
+  static llvm::Error OpenFileInExternalEditor(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/Host/common/Host.cpp

 #ifndef _WIN32
 void Host::Kill(lldb::pid_t pid, int signo) { ::kill(pid, signo); }
 
 #endif
 
 #if !defined(__APPLE__)
 bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
                                     uint32_t line_no) {
-  return false;
+  return llvm::formErrorCode(std::error_code(ENOTSUP, std::system_category()));
 }
 
 bool Host::IsInteractiveGraphicSession() { return false; }
 #endif
 
 std::unique_ptr<Connection> Host::CreateDefaultConnection(llvm::StringRef url) {
 #if defined(_WIN32)
   if (url.startswith("file://"))

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
 
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
                                            uint32_t line_no) {
 #if !TARGET_OS_OSX
-  return false;
+  return llvm::formErrorCode(std::error_code(ENOTSUP, std::system_category()));
 #else // !TARGET_OS_OSX
-  // We attach this to an 'odoc' event to specify a particular selection
+  Log *log = GetLog(LLDBLog::Host);
+
+  const std::string file_path = file_spec.GetPath();
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor",
+           file_path.empty() ? "<invalid>" : file_path, line_no);
+
+  if (file_path.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "no file specified");
+
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser<CFURLRef> file_URL = ::CFURLCreateWithFileSystemPath(
+      /*allocator=*/NULL,
+      /*filePath*/ file_cfstr.get(),
+      /*pathStyle=*/kCFURLPOSIXPathStyle,
+      /*isDirectory=*/false);
+
+  if (!file_URL.get())
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        llvm::formatv("could not create CFURL from path \"{0}\"", file_path));
+
+  // Create a new Apple Event descriptor.
   typedef struct {
     int16_t reserved0; // must be zero
     int16_t fLineNumber;
     int32_t fSelStart;
     int32_t fSelEnd;
     uint32_t reserved1; // must be zero
     uint32_t reserved2; // must be zero
   } BabelAESelInfo;
 
-  Log *log = GetLog(LLDBLog::Host);
-  char file_path[PATH_MAX];
-  file_spec.GetPath(file_path, PATH_MAX);
-  CFCString file_cfstr(file_path, kCFStringEncodingUTF8);
-  CFCReleaser<CFURLRef> file_URL(::CFURLCreateWithFileSystemPath(
-      NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
-
-  LLDB_LOGF(log,
-            "Sending source file: \"%s\" and line: %d to external editor.\n",
-            file_path, line_no);
-
-  long error;
+  // We attach this to an 'odoc' event to specify a particular selection.
   BabelAESelInfo file_and_line_info = {
       0,                      // reserved0
       (int16_t)(line_no - 1), // fLineNumber (zero based line number)
       1,                      // fSelStart
       1024,                   // fSelEnd
       0,                      // reserved1
       0                       // reserved2
   };
 
   AEKeyDesc file_and_line_desc;
+  file_and_line_desc.descKey = keyAEPosition;
+  long error = ::AECreateDesc(/*typeCode=*/typeUTF8Text,
+                              /*dataPtr=*/&file_and_line_info,
+                              /*dataSize=*/sizeof(file_and_line_info),
+                              /*result=*/&(file_and_line_desc.descContent));
+
+  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);
 
-  error = ::AECreateDesc(typeUTF8Text, &file_and_line_info,
-                         sizeof(file_and_line_info),
-                         &(file_and_line_desc.descContent));
-
-  if (error != noErr) {
-    LLDB_LOGF(log, "Error creating AEDesc: %ld.\n", error);
-    return false;
+      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}\": error {1}",
+                          external_editor, app_error)
+                .str();
       }
+    }
+  });
 
-  file_and_line_desc.descKey = keyAEPosition;
-
-  static std::string g_app_name;
-  static FSRef g_app_fsref;
+  if (!g_app_error.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(), g_app_error);
 
+  // Build app launch parameters.
   LSApplicationParameters app_params;
   ::memset(&app_params, 0, sizeof(app_params));
   app_params.flags =
       kLSLaunchDefaults | kLSLaunchDontAddToRecents | kLSLaunchDontSwitch;
-
-  char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR");
-
-  if (external_editor) {
-    LLDB_LOGF(log, "Looking for external editor \"%s\".\n", external_editor);
-
-    if (g_app_name.empty() ||
-        strcmp(g_app_name.c_str(), external_editor) != 0) {
-      CFCString editor_name(external_editor, kCFStringEncodingUTF8);
-      error = ::LSFindApplicationForInfo(kLSUnknownCreator, NULL,
-                                         editor_name.get(), &g_app_fsref, NULL);
-
-      // If we found the app, then store away the name so we don't have to
-      // re-look it up.
-      if (error != noErr) {
-        LLDB_LOGF(log,
-                  "Could not find External Editor application, error: %ld.\n",
-                  error);
-        return false;
-      }
-    }
-    app_params.application = &g_app_fsref;
-  }
+  if (g_app_fsref)
+    app_params.application = &(g_app_fsref.value());
 
   ProcessSerialNumber psn;
-  CFCReleaser<CFArrayRef> file_array(
-      CFArrayCreate(NULL, (const void **)file_URL.ptr_address(false), 1, NULL));
-  error = ::LSOpenURLsWithRole(file_array.get(), kLSRolesAll,
-                               &file_and_line_desc, &app_params, &psn, 1);
-
-  AEDisposeDesc(&(file_and_line_desc.descContent));
-
-  if (error != noErr) {
-    LLDB_LOGF(log, "LSOpenURLsWithRole failed, error: %ld.\n", error);
+  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,
+      /*inAEParam=*/&file_and_line_desc,
+      /*inAppParams=*/&app_params, /*outPSNs=*/&psn, /*inMaxPSNCount=*/1);
+
+  if (error != noErr)
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        llvm::formatv("LSOpenURLsWithRole failed: error {0}", error));
 
-    return false;
-  }
-
-  return true;
+  return llvm::Error::success();
 #endif // TARGET_OS_OSX
 }
 
 bool Host::IsInteractiveGraphicSession() {
 #if !TARGET_OS_OSX
   return false;
 #else
   auditinfo_addr_t info;

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())
-      Host::OpenFileInExternalEditor(file_spec, 1);
+    if (error.Success()) {
+      if (llvm::Error e = Host::OpenFileInExternalEditor(file_spec, 1))
+        result.AppendError(llvm::toString(std::move(e)));
+    }
   }
 
   return true;
 }
 
 bool CommandInterpreter::IsInteractive() {
   return (GetIOHandler() ? GetIOHandler()->GetIsInteractive() : false);
 }

lldb/source/Target/Thread.cpp

bool Thread::SetSelectedFrameByIndexNoisily(uint32_t frame_idx,
   if (success) {
     StackFrameSP frame_sp = GetSelectedFrame();
     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) {
-        already_shown = Host::OpenFileInExternalEditor(
-            frame_sc.line_entry.file, frame_sc.line_entry.line);
+        if (llvm::Error e = Host::OpenFileInExternalEditor(
+                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;
       bool show_source = !already_shown;
       FrameSelectedCallback(frame_sp.get());
       return frame_sp->GetStatus(output_stream, show_frame_info, show_source);
     }
     return false;
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) {
-          Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
-                                         frame_sc.line_entry.line);
+          if (llvm::Error e = Host::OpenFileInExternalEditor(
+                  frame_sc.line_entry.file, frame_sc.line_entry.line)) {
+            LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(e),
+                           "OpenFileInExternalEditor failed: {0}");
+          }
         }
       }
     }
 
     DumpUsingSettingsFormat(strm, start_frame, stop_format);
   }
 
   size_t num_frames_shown = 0;