From 6ba2f6e58c60ba8b15aefaed6d0ba1a3e9fae638 Mon Sep 17 00:00:00 2001 From: Dmitrii Galimzianov Date: Sat, 24 May 2025 23:42:41 +0200 Subject: [PATCH] [LLDB] Deterministic module order in `Target::SetExecutableModule` This change separates module loading from symbol preloading to ensure a deterministic module loading order. Parallel module loading was causing non-deterministic behavior, but we only need to load symbols in parallel as that's the heavy part of the process. --- lldb/include/lldb/Target/Target.h | 44 ++++++++ lldb/source/Target/Target.cpp | 102 +++++++----------- .../deterministic_module_order/Makefile | 26 +++++ .../TestDeterministicModuleOrder.py | 61 +++++++++++ .../deterministic_module_order/lib.c | 1 + .../deterministic_module_order/main.c | 3 + 6 files changed, 172 insertions(+), 65 deletions(-) create mode 100644 lldb/test/API/functionalities/deterministic_module_order/Makefile create mode 100644 lldb/test/API/functionalities/deterministic_module_order/TestDeterministicModuleOrder.py create mode 100644 lldb/test/API/functionalities/deterministic_module_order/lib.c create mode 100644 lldb/test/API/functionalities/deterministic_module_order/main.c diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h index 97eaad79e9da0..1885868d5d680 100644 --- a/lldb/include/lldb/Target/Target.h +++ b/lldb/include/lldb/Target/Target.h @@ -725,6 +725,50 @@ class Target : public std::enable_shared_from_this, lldb::ModuleSP GetOrCreateModule(const ModuleSpec &module_spec, bool notify, Status *error_ptr = nullptr); + /// Find a binary on the system and return its Module, + /// or return an existing Module that is already in the Target. + /// + /// Given a ModuleSpec, find a binary satisifying that specification, + /// or identify a matching Module already present in the Target, + /// and return a shared pointer to it. + /// + /// \param[in] module_spec + /// The criteria that must be matched for the binary being loaded. + /// e.g. UUID, architecture, file path. + /// + /// \param[in] notify + /// If notify is true, and the Module is new to this Target, + /// Target::ModulesDidLoad will be called. + /// If notify is false, it is assumed that the caller is adding + /// multiple Modules and will call ModulesDidLoad with the + /// full list at the end. + /// ModulesDidLoad must be called when a Module/Modules have + /// been added to the target, one way or the other. + /// + /// \param[in] scheduleSymbolsPreload + /// A function that schedules symbol preloading tasks. This allows + /// the caller to customize how symbol preloading is scheduled, + /// for example to run it in parallel. It's called with a callback that + /// will perform the actual symbol loading. The other overload of this + /// method without this parameter will perform symbol preloading + /// immediately in the calling thread. + /// + /// \param[out] error_ptr + /// Optional argument, pointing to a Status object to fill in + /// with any results / messages while attempting to find/load + /// this binary. Many callers will be internal functions that + /// will handle / summarize the failures in a custom way and + /// don't use these messages. + /// + /// \return + /// An empty ModuleSP will be returned if no matching file + /// was found. If error_ptr was non-nullptr, an error message + /// will likely be provided. + lldb::ModuleSP GetOrCreateModule( + const ModuleSpec &module_spec, bool notify, + llvm::function_ref)> scheduleSymbolsPreload, + Status *error_ptr = nullptr); + // Settings accessors static TargetProperties &GetGlobalProperties(); diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 6d4a2ec68ba11..d3dcd73fb1d04 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -1538,73 +1538,35 @@ void Target::SetExecutableModule(ModuleSP &executable_sp, break; } - if (executable_objfile && load_dependents) { - // FileSpecList is not thread safe and needs to be synchronized. - FileSpecList dependent_files; - std::mutex dependent_files_mutex; - - // ModuleList is thread safe. - ModuleList added_modules; - - auto GetDependentModules = [&](FileSpec dependent_file_spec) { - FileSpec platform_dependent_file_spec; - if (m_platform_sp) - m_platform_sp->GetFileWithUUID(dependent_file_spec, nullptr, - platform_dependent_file_spec); - else - platform_dependent_file_spec = dependent_file_spec; - - ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec()); - ModuleSP image_module_sp( - GetOrCreateModule(module_spec, false /* notify */)); - if (image_module_sp) { - added_modules.AppendIfNeeded(image_module_sp, false); - ObjectFile *objfile = image_module_sp->GetObjectFile(); - if (objfile) { - // Create a local copy of the dependent file list so we don't have - // to lock for the whole duration of GetDependentModules. - FileSpecList dependent_files_copy; - { - std::lock_guard guard(dependent_files_mutex); - dependent_files_copy = dependent_files; - } - - // Remember the size of the local copy so we can append only the - // modules that have been added by GetDependentModules. - const size_t previous_dependent_files = - dependent_files_copy.GetSize(); - - objfile->GetDependentModules(dependent_files_copy); - - { - std::lock_guard guard(dependent_files_mutex); - for (size_t i = previous_dependent_files; - i < dependent_files_copy.GetSize(); ++i) - dependent_files.AppendIfUnique( - dependent_files_copy.GetFileSpecAtIndex(i)); - } - } - } - }; - - executable_objfile->GetDependentModules(dependent_files); + if (!executable_objfile || !load_dependents) + return; - llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool()); - for (uint32_t i = 0; i < dependent_files.GetSize(); i++) { - // Process all currently known dependencies in parallel in the innermost - // loop. This may create newly discovered dependencies to be appended to - // dependent_files. We'll deal with these files during the next - // iteration of the outermost loop. - { - std::lock_guard guard(dependent_files_mutex); - for (; i < dependent_files.GetSize(); i++) - task_group.async(GetDependentModules, - dependent_files.GetFileSpecAtIndex(i)); - } - task_group.wait(); + llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool()); + ModuleList added_modules; + FileSpecList dependent_files; + executable_objfile->GetDependentModules(dependent_files); + + for (uint32_t i = 0; i < dependent_files.GetSize(); i++) { + auto dependent_file_spec = dependent_files.GetFileSpecAtIndex(i); + FileSpec platform_dependent_file_spec; + if (m_platform_sp) + m_platform_sp->GetFileWithUUID(dependent_file_spec, nullptr, + platform_dependent_file_spec); + else + platform_dependent_file_spec = dependent_file_spec; + + ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec()); + auto image_module_sp = GetOrCreateModule( + module_spec, false /* notify */, + [&](auto preloadSymbols) { task_group.async(preloadSymbols); }); + if (image_module_sp) { + added_modules.AppendIfNeeded(image_module_sp, false); + if (auto *objfile = image_module_sp->GetObjectFile()) + objfile->GetDependentModules(dependent_files); } - ModulesDidLoad(added_modules); } + task_group.wait(); + ModulesDidLoad(added_modules); } } @@ -2249,6 +2211,15 @@ bool Target::ReadPointerFromMemory(const Address &addr, Status &error, ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify, Status *error_ptr) { + return GetOrCreateModule(module_spec, notify, [](auto preloadSymbols) { + preloadSymbols(); + }, error_ptr); +} + +ModuleSP Target::GetOrCreateModule( + const ModuleSpec &module_spec, bool notify, + llvm::function_ref)> scheduleSymbolsPreload, + Status *error_ptr) { ModuleSP module_sp; Status error; @@ -2408,7 +2379,8 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify, // Preload symbols outside of any lock, so hopefully we can do this for // each library in parallel. if (GetPreloadSymbols()) - module_sp->PreloadSymbols(); + scheduleSymbolsPreload([module_sp] { module_sp->PreloadSymbols(); }); + llvm::SmallVector replaced_modules; for (ModuleSP &old_module_sp : old_modules) { if (m_images.GetIndexForModule(old_module_sp.get()) != diff --git a/lldb/test/API/functionalities/deterministic_module_order/Makefile b/lldb/test/API/functionalities/deterministic_module_order/Makefile new file mode 100644 index 0000000000000..8893ca4b1624a --- /dev/null +++ b/lldb/test/API/functionalities/deterministic_module_order/Makefile @@ -0,0 +1,26 @@ +C_SOURCES := main.c +LD_EXTRAS := -L. -la -lb -lc -ld -le + +a.out: liba libb libc libd libe + +include Makefile.rules + +liba: + "$(MAKE)" -f $(MAKEFILE_RULES) \ + DYLIB_ONLY=YES DYLIB_C_SOURCES=lib.c DYLIB_NAME=a + +libb: + "$(MAKE)" -f $(MAKEFILE_RULES) \ + DYLIB_ONLY=YES DYLIB_C_SOURCES=lib.c DYLIB_NAME=b + +libc: + "$(MAKE)" -f $(MAKEFILE_RULES) \ + DYLIB_ONLY=YES DYLIB_C_SOURCES=lib.c DYLIB_NAME=c + +libd: + "$(MAKE)" -f $(MAKEFILE_RULES) \ + DYLIB_ONLY=YES DYLIB_C_SOURCES=lib.c DYLIB_NAME=d + +libe: + "$(MAKE)" -f $(MAKEFILE_RULES) \ + DYLIB_ONLY=YES DYLIB_C_SOURCES=lib.c DYLIB_NAME=e \ No newline at end of file diff --git a/lldb/test/API/functionalities/deterministic_module_order/TestDeterministicModuleOrder.py b/lldb/test/API/functionalities/deterministic_module_order/TestDeterministicModuleOrder.py new file mode 100644 index 0000000000000..41207acf0bb16 --- /dev/null +++ b/lldb/test/API/functionalities/deterministic_module_order/TestDeterministicModuleOrder.py @@ -0,0 +1,61 @@ +""" +Test that module loading order is deterministic. +""" + +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + + +class DeterministicModuleOrderTestCase(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def setUp(self): + TestBase.setUp(self) + + def test_create_target_module_load_order(self): + """Test that module loading order is deterministic across multiple runs.""" + + self.build() + exe = self.getBuildArtifact("a.out") + + # First run: Create target and save module list + target = self.dbg.CreateTarget(exe) + self.assertTrue(target, VALID_TARGET) + + first_run_modules = [] + for i in range(target.GetNumModules()): + module = target.GetModuleAtIndex(i) + module_name = module.GetFileSpec().GetFilename() + first_run_modules.append(module_name) + + self.dbg.DeleteTarget(target) + + if self.TraceOn(): + print(f"First run module list: {first_run_modules}") + + if not first_run_modules: + self.fail("No modules were found during the first run") + + # Subsequent runs: Create target and compare module list to first run + num_additional_runs = 9 # Total of 10 runs including the first one + for run in range(num_additional_runs): + target = self.dbg.CreateTarget(exe) + self.assertTrue(target, VALID_TARGET) + current_modules = [] + for i in range(target.GetNumModules()): + module = target.GetModuleAtIndex(i) + module_name = module.GetFileSpec().GetFilename() + current_modules.append(module_name) + + if self.TraceOn(): + print(f"Run {run+2} module list: {current_modules}") + + self.assertEqual( + first_run_modules, + current_modules, + f"Module list in run {run+2} differs from the first run" + ) + + self.dbg.DeleteTarget(target) diff --git a/lldb/test/API/functionalities/deterministic_module_order/lib.c b/lldb/test/API/functionalities/deterministic_module_order/lib.c new file mode 100644 index 0000000000000..787afb7bca745 --- /dev/null +++ b/lldb/test/API/functionalities/deterministic_module_order/lib.c @@ -0,0 +1 @@ +void dummy_func(void) {} \ No newline at end of file diff --git a/lldb/test/API/functionalities/deterministic_module_order/main.c b/lldb/test/API/functionalities/deterministic_module_order/main.c new file mode 100644 index 0000000000000..e9cdae1659d1c --- /dev/null +++ b/lldb/test/API/functionalities/deterministic_module_order/main.c @@ -0,0 +1,3 @@ +int main() { + return 0; +} \ No newline at end of file