[Lldb-commits] [lldb] 13dbc16 - [lldb] Refactor host::OpenFileInExternalEditor
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Fri Apr 28 18:13:03 PDT 2023
Author: Jonas Devlieghere
Date: 2023-04-28T18:12:41-07:00
New Revision: 13dbc16b4d82b9adc98c0919873b2b59ccc46afd
URL: https://github.com/llvm/llvm-project/commit/13dbc16b4d82b9adc98c0919873b2b59ccc46afd
DIFF: https://github.com/llvm/llvm-project/commit/13dbc16b4d82b9adc98c0919873b2b59ccc46afd.diff
LOG: [lldb] Refactor host::OpenFileInExternalEditor
This patch refactors the macOS implementation of
OpenFileInExternalEditor. It fixes an AppleEvent memory leak, the
caching of LLDB_EXTERNAL_EDITOR and speculatively fixes a crash when
CFURL is NULL (rdar://108633464). The new code also improves error
handling, readability and documents calls to the CoreFoundation Launch
Services APIs.
A bunch of the Launch Services APIs have been deprecated
(LSFindApplicationForInfo, LSOpenURLsWithRole). The preferred API is
LSOpenCFURLRef but it doesn't specifying the "location" Apple Event
which is used to highlight the current line and switching over would
regress the existing behavior.
rdar://108633464
Differential revision: https://reviews.llvm.org/D149482
Added:
Modified:
lldb/include/lldb/Host/Host.h
lldb/source/Host/common/Host.cpp
lldb/source/Host/macosx/objcxx/Host.mm
lldb/source/Interpreter/CommandInterpreter.cpp
lldb/source/Target/Thread.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Host/Host.h b/lldb/include/lldb/Host/Host.h
index 4fc2bd128b025..3fdf59dfb2324 100644
--- a/lldb/include/lldb/Host/Host.h
+++ b/lldb/include/lldb/Host/Host.h
@@ -236,8 +236,8 @@ class Host {
bool run_in_shell = true,
bool hide_stderr = false);
- static bool OpenFileInExternalEditor(const FileSpec &file_spec,
- uint32_t line_no);
+ static llvm::Error OpenFileInExternalEditor(const FileSpec &file_spec,
+ uint32_t line_no);
/// Check if we're running in an interactive graphical session.
///
diff --git a/lldb/source/Host/common/Host.cpp b/lldb/source/Host/common/Host.cpp
index 33b550008b746..c8ebb6f84c004 100644
--- a/lldb/source/Host/common/Host.cpp
+++ b/lldb/source/Host/common/Host.cpp
@@ -546,9 +546,10 @@ 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;
+llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+ uint32_t line_no) {
+ return llvm::errorCodeToError(
+ std::error_code(ENOTSUP, std::system_category()));
}
bool Host::IsInteractiveGraphicSession() { return false; }
diff --git a/lldb/source/Host/macosx/objcxx/Host.mm b/lldb/source/Host/macosx/objcxx/Host.mm
index 303a138559e93..848fa0d79f4da 100644
--- a/lldb/source/Host/macosx/objcxx/Host.mm
+++ b/lldb/source/Host/macosx/objcxx/Host.mm
@@ -325,12 +325,36 @@ repeat with the_window in (get windows)\n\
#endif // TARGET_OS_OSX
-bool Host::OpenFileInExternalEditor(const FileSpec &file_spec,
- uint32_t line_no) {
+llvm::Error Host::OpenFileInExternalEditor(const FileSpec &file_spec,
+ uint32_t line_no) {
#if !TARGET_OS_OSX
- return false;
+ return llvm::errorCodeToError(
+ 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;
@@ -340,18 +364,7 @@ repeat with the_window in (get windows)\n\
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)
@@ -362,64 +375,74 @@ repeat with the_window in (get windows)\n\
};
AEKeyDesc file_and_line_desc;
-
- 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;
- }
-
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);
+
+ 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();
+ }
+ }
+ });
- 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);
-
- return false;
- }
-
- return true;
+ 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 llvm::Error::success();
#endif // TARGET_OS_OSX
}
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index cee17df7a4b9d..f89cff49f0aeb 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -3271,8 +3271,10 @@ bool CommandInterpreter::SaveTranscript(
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;
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index 98d05fa292a40..fa4c2c6a3557c 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -305,8 +305,13 @@ bool Thread::SetSelectedFrameByIndexNoisily(uint32_t frame_idx,
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;
@@ -1725,8 +1730,11 @@ size_t Thread::GetStatus(Stream &strm, uint32_t start_frame,
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}");
+ }
}
}
}
More information about the lldb-commits
mailing list