[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.

387–388

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?

411–412

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.

385

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.

387–388

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?

411–412

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.

385

Because it is cached across calls.

[mib]

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

343–344

+1

411–412

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

420

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

411–412

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);

387–388

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

387–388

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 ↗

(On Diff #518007)

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/source/Host/macosx/objcxx/Host.mm

 
 #endif // TARGET_OS_OSX
 
 bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
                                     uint32_t line_no) {
 #if !TARGET_OS_OSX
   return false;
 #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, line_no);
+
+  if (file_path.empty())
+    return false;
+
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser<CFURLRef> file_URL(::CFURLCreateWithFileSystemPath(
+      NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
+
+  if (!file_URL.get())
+    return false;
+
+  // 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;
-
-  error = ::AECreateDesc(typeUTF8Text, &file_and_line_info,
-                         sizeof(file_and_line_info),
-                         &(file_and_line_desc.descContent));
+  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) {
-    LLDB_LOGF(log, "Error creating AEDesc: %ld.\n", error);
+    LLDB_LOG(log, "Creating Apple Event descriptor failed: error {0}", error);
     return false;
   }
 
-  file_and_line_desc.descKey = keyAEPosition;
+  // 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::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);
 
-  static std::string g_app_name;
-  static FSRef g_app_fsref;
+      FSRef app_fsref;
+      CFCString editor_name(external_editor, kCFStringEncodingUTF8);
+      error = ::LSFindApplicationForInfo(
+          /*inCreator=*/kLSUnknownCreator, /*inBundleID=*/NULL,
+          /*inName=*/editor_name.get(), /*outAppRef=*/&app_fsref,
+          /*outAppURL=*/NULL);
+      if (error == noErr) {
+        g_app_fsref = app_fsref;
+      } else {
+        LLDB_LOG(log, "Could not find external editor \"{0}\": error {1}",
+                 external_editor, 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));
+  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=*/kLSRolesAll,
+      /*inAEParam=*/&file_and_line_desc,
+      /*inAppParams=*/&app_params, /*outPSNs=*/&psn, /*inMaxPSNCount=*/1);
 
   if (error != noErr) {
-    LLDB_LOGF(log, "LSOpenURLsWithRole failed, error: %ld.\n", error);
-
+    LLDB_LOG(log, "LSOpenURLsWithRole failed: error {0}", error);
     return false;
   }
 
   return true;
 #endif // TARGET_OS_OSX
 }
 
 bool Host::IsInteractiveGraphicSession() {