llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> A userland or kernel corefile can list hundreds of images, and searching for one can shell out to a symbol server or fetch over the network. Searching for them one at a time is where loading such a corefile spends its time. Add a batch form of SymbolLocator::Locate that runs the searches on the debugger's thread pool, gated on target.parallel-module-load. Results come back in the order the requests were given, since that order decides the Target's module order. Only the results are ordered, and anything a search reports to the user arrives in whatever order the searches finish in. Only the plugin searches run concurrently, so a platform hook does not have to be thread safe to take part, and reading a binary's UUID out of memory stays on the calling thread. Setting up a platform binary can replace the Target's platform and dynamic loader, and now happens for every image before any of them is searched for, so the platform a corefile asks for is the one all of its images are searched with. Previously the images listed ahead of a platform binary were searched with whatever platform preceded it. Assisted-by: Claude --- <sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub> --- Patch is 30.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/215393.diff 6 Files Affected: - (modified) lldb/include/lldb/Symbol/SymbolLocator.h (+30) - (modified) lldb/source/Core/DynamicLoader.cpp (+43-19) - (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+84-64) - (modified) lldb/source/Symbol/SymbolLocator.cpp (+82-11) - (modified) lldb/test/API/macosx/lc-note/multiple-binary-corefile/TestMultipleBinaryCorefile.py (+25) - (modified) lldb/unittests/Symbol/SymbolLocatorTest.cpp (+142-4) ``````````diff diff --git a/lldb/include/lldb/Symbol/SymbolLocator.h b/lldb/include/lldb/Symbol/SymbolLocator.h index d86538fd9a2ce..244b43946381e 100644 --- a/lldb/include/lldb/Symbol/SymbolLocator.h +++ b/lldb/include/lldb/Symbol/SymbolLocator.h @@ -15,9 +15,12 @@ #include "lldb/Utility/Status.h" #include "lldb/Utility/UUID.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/Support/Error.h" +#include <string> #include <system_error> +#include <vector> namespace lldb_private { @@ -48,6 +51,9 @@ class SymbolLocator : public PluginInterface { /// Allow contacting an external symbol server when the local searches come /// up empty. bool external_lookup = false; + + /// How to name this binary in a progress report. + std::string description; }; /// What a search found. @@ -80,12 +86,36 @@ class SymbolLocator : public PluginInterface { static llvm::Expected<Result> Locate(const Request &request, const FileSpecList &search_paths); + /// Search for a batch of binaries. + /// + /// Each request's platform hook runs on the calling thread, in order, so that + /// a platform does not have to be thread safe to take part. The plugin + /// searches may run concurrently, which is where the time goes: a single one + /// of them can shell out to a symbol server or fetch over the network. + /// + /// Blocks until every search has finished. + /// + /// \return + /// One result per request, in the order the requests were given, each as + /// described for the single request above. Only the results are ordered. + /// Anything a search reports to the user arrives in whatever order the + /// searches finish in. + static std::vector<llvm::Expected<Result>> + Locate(llvm::ArrayRef<Request> requests, const FileSpecList &search_paths, + bool parallel); + /// Locate the symbol file for the given UUID on a background thread. This /// function returns immediately. Under the hood it uses the debugger's /// thread pool to call DownloadObjectAndSymbolFile. If a symbol file is /// found, this will notify all target which contain the module with the /// given UUID. static void DownloadSymbolFileAsync(const UUID &uuid); + +private: + /// The symbol locator plugins, without the platform hook. This is the half + /// that is safe to run for several binaries at once. + static llvm::Expected<Result> + LocateWithPlugins(const Request &request, const FileSpecList &search_paths); }; } // namespace lldb_private diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp index 09f66e38f2097..2a5852054e5f6 100644 --- a/lldb/source/Core/DynamicLoader.cpp +++ b/lldb/source/Core/DynamicLoader.cpp @@ -13,7 +13,6 @@ #include "lldb/Core/ModuleList.h" #include "lldb/Core/ModuleSpec.h" #include "lldb/Core/PluginManager.h" -#include "lldb/Core/Progress.h" #include "lldb/Core/Section.h" #include "lldb/Symbol/ObjectFile.h" #include "lldb/Symbol/SymbolLocator.h" @@ -26,10 +25,13 @@ #include "lldb/Utility/Log.h" #include "lldb/lldb-private-interfaces.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include <memory> +#include <optional> #include <string> #include <cassert> @@ -245,13 +247,16 @@ GetBinaryNotFoundMessage(const DynamicLoader::BinarySpec &bin_spec) { return msg.GetString().str(); } -/// Search for a binary with a known UUID, and create a module for it. +/// Work out what to search for, and answer the question the Target can answer +/// on its own. /// /// Does not mutate the Target, but does read from it, and reaches the global -/// shared module list, the symbol locator plugins, and a locate module callback -/// the user may have installed. -static void SearchForBinary(Target &target, DynamicLoader::BinarySpec &bin_spec, - const FileSpecList &search_paths) { +/// shared module list and a locate module callback the user may have installed, +/// so it has to be called for one binary at a time. +/// +/// \return What to search for, or nothing when the binary is already in hand. +static std::optional<SymbolLocator::Request> +PrepareSearch(Target &target, DynamicLoader::BinarySpec &bin_spec) { ModuleSpec module_spec; module_spec.SetTarget(target.shared_from_this()); module_spec.GetUUID() = bin_spec.uuid; @@ -266,19 +271,24 @@ static void SearchForBinary(Target &target, DynamicLoader::BinarySpec &bin_spec, /*invoke_locate_callback=*/true, /*invoke_symbol_locators=*/false); if (bin_spec.module_sp && bin_spec.module_sp->GetSymbolFileFileSpec()) - return; + return std::nullopt; - // Search for the binary and its symbols. SymbolLocator::Request request; request.module_spec = module_spec; request.platform = target.GetPlatform(); request.external_lookup = bin_spec.force_symbol_search; + request.description = GetBinaryDescription(bin_spec); + return request; +} - llvm::Expected<SymbolLocator::Result> located = - SymbolLocator::Locate(request, search_paths); +/// Create a module for what the search found. +/// +/// The module is not registered with the Target until LoadBinaryInTarget. +static void FinishSearch(DynamicLoader::BinarySpec &bin_spec, + llvm::Expected<SymbolLocator::Result> located) { if (!located) { - // This function's caller names the binary it could not find, so a plain - // miss needs nothing added to it. An explanation from a symbol server does. + // LoadBinaryInTarget names the binary it could not find, so a plain miss + // needs nothing added to it. An explanation from a symbol server does. llvm::Error error = located.takeError(); if (error.isA<SymbolLocator::NotFound>()) llvm::consumeError(std::move(error)); @@ -287,13 +297,12 @@ static void SearchForBinary(Target &target, DynamicLoader::BinarySpec &bin_spec, return; } - // A binary was found. Its symbols are another matter, and the caller reports - // that in its own order. + // A binary was found. Its symbols are another matter, and LoadBinaryInTarget + // reports that in its own order. bin_spec.error = std::move(located->symbol_error); - // Create a module for what was found, sharing it with any other Target that - // asks for the same binary. The module is not registered with this Target - // until LoadBinaryInTarget. The locators have run, so don't run them again. + // Share the module with any other Target that asks for the same binary. The + // locators have run, so don't run them again. ModuleSP located_module_sp; ModuleList::GetSharedModule(located->module_spec, located_module_sp, nullptr, nullptr, /*invoke_locate_callback=*/false, @@ -323,14 +332,29 @@ void DynamicLoader::LocateBinaries( Target &target = process->GetTarget(); const FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths(); + // Reading a binary's UUID out of memory has to happen on this thread, and + // before the searches, so that every binary that has a UUID to find is part + // of the batch. + llvm::SmallVector<BinarySpec *> to_search; + std::vector<SymbolLocator::Request> requests; for (BinarySpec &bin_spec : bin_specs) { if (!bin_spec.uuid.IsValid() && !bin_spec.value_is_offset) FindBinaryUUIDInMemory(process, bin_spec); if (!bin_spec.uuid.IsValid()) continue; - Progress progress("Locating binary", GetBinaryDescription(bin_spec)); - SearchForBinary(target, bin_spec, search_paths); + if (std::optional<SymbolLocator::Request> request = + PrepareSearch(target, bin_spec)) { + to_search.push_back(&bin_spec); + requests.push_back(std::move(*request)); + } } + + std::vector<llvm::Expected<SymbolLocator::Result>> located = + SymbolLocator::Locate(requests, search_paths, + target.GetParallelModuleLoad()); + + for (auto [bin_spec, result] : llvm::zip_equal(to_search, located)) + FinishSearch(*bin_spec, std::move(result)); } llvm::Expected<ModuleSP> diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index 5a31f16c9a729..e852777039ac5 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -6674,17 +6674,26 @@ ObjectFileMachO::GetCorefileAllImageInfos() { bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) { MachOCorefileAllImageInfos image_infos = GetCorefileAllImageInfos(); Log *log = GetLog(LLDBLog::Object | LLDBLog::DynamicLoader); - Status error; bool found_platform_binary = false; ModuleList added_modules; - for (MachOCorefileImageEntry &image : image_infos.all_image_infos) { - ModuleSP module_sp, local_filesystem_module_sp; + // Searching for a binary is slow, so gather them up and search for a whole + // batch at once. The images are installed in the order the corefile lists + // them, whatever order the searches finish in. + llvm::SmallVector<const MachOCorefileImageEntry *> pending_images; + std::vector<DynamicLoader::BinarySpec> pending_specs; + + for (MachOCorefileImageEntry &image : image_infos.all_image_infos) { // If this is a platform binary, it has been loaded (or registered with // the DynamicLoader to be loaded), we don't need to do any further // processing. We're not going to call ModulesDidLoad on this in this // method, so notify==true. + // + // Setting one up can replace the Target's platform and dynamic loader, and + // no image is searched for until this loop has run to the end, so the + // platform a corefile asks for is the one all of its images are searched + // with. if (process.GetTarget() .GetDebugger() .GetPlatformList() @@ -6708,74 +6717,85 @@ bool ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) { // We have either a UUID, or we have a load address which // and can try to read load commands and find a UUID. - if (image.uuid.IsValid() || - (!value_is_offset && value != LLDB_INVALID_ADDRESS)) { - DynamicLoader::BinarySpec bin_spec; - bin_spec.name = image.filename; - bin_spec.uuid = image.uuid; - bin_spec.value = value; - bin_spec.value_is_offset = value_is_offset; - bin_spec.force_symbol_search = image.currently_executing; - bin_spec.notify = false; - // Userland Darwin binaries will have segment load addresses via - // the `all image infos` LC_NOTE. - bin_spec.set_address_in_target = image.segment_load_addresses.empty(); - bin_spec.allow_memory_image_last_resort = - !image.segment_load_addresses.empty(); - if (llvm::Expected<ModuleSP> located = - DynamicLoader::LocateAndLoadBinary(&process, bin_spec)) { - module_sp = *located; - } else if (bin_spec.force_symbol_search) { - *process.GetTarget().GetDebugger().GetAsyncErrorStream() - << llvm::toString(located.takeError()) << "\n"; - } else { - // A corefile image that isn't on this machine is routine, and - // LocateAndLoadBinary has already logged it. - llvm::consumeError(located.takeError()); - } + if (!image.uuid.IsValid() && + (value_is_offset || value == LLDB_INVALID_ADDRESS)) + continue; + + DynamicLoader::BinarySpec bin_spec; + bin_spec.name = image.filename; + bin_spec.uuid = image.uuid; + bin_spec.value = value; + bin_spec.value_is_offset = value_is_offset; + bin_spec.force_symbol_search = image.currently_executing; + bin_spec.notify = false; + // Userland Darwin binaries will have segment load addresses via + // the `all image infos` LC_NOTE. + bin_spec.set_address_in_target = image.segment_load_addresses.empty(); + bin_spec.allow_memory_image_last_resort = + !image.segment_load_addresses.empty(); + + pending_images.push_back(&image); + pending_specs.push_back(std::move(bin_spec)); + } + + DynamicLoader::LocateBinaries(&process, pending_specs); + + for (auto [image, bin_spec] : + llvm::zip_equal(pending_images, pending_specs)) { + ModuleSP module_sp; + if (llvm::Expected<ModuleSP> loaded = + DynamicLoader::LoadBinaryInTarget(&process, bin_spec)) { + module_sp = *loaded; + } else if (bin_spec.force_symbol_search) { + *process.GetTarget().GetDebugger().GetAsyncErrorStream() + << llvm::toString(loaded.takeError()) << "\n"; + } else { + // A corefile image that isn't on this machine is routine, and + // LoadBinaryInTarget has already logged it. + llvm::consumeError(loaded.takeError()); } - // We have a ModuleSP to load in the Target. Load it at the - // correct address/slide and notify/load scripting resources. - if (module_sp) { - added_modules.Append(module_sp, false /* notify */); - - // We have a list of segment load address - if (image.segment_load_addresses.size() > 0) { - if (log) { - std::string uuidstr = image.uuid.GetAsString(); - log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " - "UUID %s with section load addresses", - module_sp->GetFileSpec().GetPath().c_str(), - uuidstr.c_str()); - } - ObjectFile *objfile = module_sp->GetObjectFile(); - SectionList *sectlist = objfile ? objfile->GetSectionList() : nullptr; - for (auto name_vmaddr_tuple : image.segment_load_addresses) { - if (sectlist) { - SectionSP sect_sp = - sectlist->FindSectionByName(std::get<0>(name_vmaddr_tuple)); - if (sect_sp) { - process.GetTarget().SetSectionLoadAddress( - sect_sp, std::get<1>(name_vmaddr_tuple)); - } + if (!module_sp) + continue; + + added_modules.Append(module_sp, false /* notify */); + + // We have a list of segment load address + if (image->segment_load_addresses.size() > 0) { + if (log) { + std::string uuidstr = image->uuid.GetAsString(); + log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " + "UUID %s with section load addresses", + module_sp->GetFileSpec().GetPath().c_str(), + uuidstr.c_str()); + } + ObjectFile *objfile = module_sp->GetObjectFile(); + SectionList *sectlist = objfile ? objfile->GetSectionList() : nullptr; + for (auto name_vmaddr_tuple : image->segment_load_addresses) { + if (sectlist) { + SectionSP sect_sp = + sectlist->FindSectionByName(std::get<0>(name_vmaddr_tuple)); + if (sect_sp) { + process.GetTarget().SetSectionLoadAddress( + sect_sp, std::get<1>(name_vmaddr_tuple)); } } - } else { - if (log) { - std::string uuidstr = image.uuid.GetAsString(); - log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " - "UUID %s with %s 0x%" PRIx64, - module_sp->GetFileSpec().GetPath().c_str(), - uuidstr.c_str(), - value_is_offset ? "slide" : "load address", value); - } - bool changed; - module_sp->SetLoadAddress(process.GetTarget(), value, value_is_offset, - changed); } + } else { + if (log) { + std::string uuidstr = image->uuid.GetAsString(); + log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' " + "UUID %s with %s 0x%" PRIx64, + module_sp->GetFileSpec().GetPath().c_str(), uuidstr.c_str(), + bin_spec.value_is_offset ? "slide" : "load address", + bin_spec.value); + } + bool changed; + module_sp->SetLoadAddress(process.GetTarget(), bin_spec.value, + bin_spec.value_is_offset, changed); } } + if (added_modules.GetSize() > 0) { process.GetTarget().ModulesDidLoad(added_modules); process.Flush(); diff --git a/lldb/source/Symbol/SymbolLocator.cpp b/lldb/source/Symbol/SymbolLocator.cpp index 1f32d69a0b618..8eb120e2bf5a6 100644 --- a/lldb/source/Symbol/SymbolLocator.cpp +++ b/lldb/source/Symbol/SymbolLocator.cpp @@ -10,10 +10,12 @@ #include "lldb/Core/Debugger.h" #include "lldb/Core/PluginManager.h" +#include "lldb/Core/Progress.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/Host.h" #include "lldb/Target/Platform.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallSet.h" #include "llvm/Support/ThreadPool.h" @@ -31,22 +33,13 @@ std::error_code SymbolLocator::NotFound::convertToErrorCode() const { } llvm::Expected<SymbolLocator::Result> -SymbolLocator::Locate(const Request &request, - const FileSpecList &search_paths) { +SymbolLocator::LocateWithPlugins(const Request &request, + const FileSpecList &search_paths) { FileSystem &fs = FileSystem::Instance(); Result result; ModuleSpec &module_spec = result.module_spec; module_spec = request.module_spec; - // The locator plugins have no Platform to consult, so ask it here. - if (request.platform) { - if (std::optional<ModuleSpec> found = request.platform->FindModuleFiles( - module_spec, search_paths, result.statistics)) { - result.module_spec = *found; - return result; - } - } - // Can lldb's symbol and executable location schemes find them locally? module_spec.GetSymbolFileSpec() = PluginManager::LocateExecutableSymbolFile( module_spec, search_paths, result.statistics); @@ -77,6 +70,84 @@ SymbolLocator::Locate(const Request &request, return result; } +/// Ask the request's platform where the binary is, if it has one. Answering +/// with std::nullopt leaves the search to the plugins. +static std::optional<SymbolLocator::Result> +AskPlatform(const SymbolLocator::Request &request, + const FileSpecList &search_paths) { + if (!request.platform) + return std::nullopt; + + SymbolLocator::Result result; + std::optional<ModuleSpec> found = request.platform->FindModuleFiles( + request.module_spec, search_paths, result.statistics); + if (!found) + return std::nullopt; + + result.module_spec = *found; + return result; +} + +llvm::Expected<SymbolLocator::Result> +SymbolLocator::Locate(const Request &request, + const FileSpecList &search_paths) { + if (std::optional<Result> answer = AskPlatform(request, search_paths)) + return std::move(*answer); + return LocateWithPlugins(request, search_paths); +} + +std::vector<llvm::Expected<SymbolLocator::Result>> +SymbolLocator::Locate(llvm::ArrayRef<Request> requests, + const FileSpecList &search_paths, bool parallel) { + // One slot per request, so that the searches can fill them in without + // contending, and so that the results come back in the caller's order. + std::vector<std::optional<llvm::Expected<Result>>> slots(requests.size()); + std::vector<size_t> remaining; + + if (!requests.empty()) { + // A search is slow enough that reporting every one of them individually is + // not worth the contention between the threads running them. + Progress progress("Locating binaries", "", requests.size(), + /*debugger=*/nullptr, + Progress::kDefaultHighFrequencyReportTime); + + // The platform hooks run here, on this thread and in order. + for (auto [i, request] : llvm::enumerate(requests)) { + if (std::optional<Result> answer = AskPlatform(request, search_paths)) { + slots[i] = std::move(*answer); + progress.Increment(1, request.description); + } else { + remaining.push_back(i); + } + } + + auto locate = [&](size_t i) { + slots[i] = LocateWithPlugins(requests[i], search_paths); + progress.Increment(1, requests[i].description); + }; + + // One search has nothing to overlap with, and handing it to the pool would + // only move it off this thread. + if (parallel && rem... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/215393 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
