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}"); + } } } } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
