From d10d3795f737ca8a0f602e8a442728d301e65378 Mon Sep 17 00:00:00 2001 From: Frederic Riss Date: Fri, 11 May 2018 18:21:11 +0000 Subject: [PATCH] Add a lock to PlatformPOSIX::DoLoadImage Summary: Multiple threads could be calling into DoLoadImage concurrently, only one should be allowed to create the UtilityFunction. Reviewers: jingham Subscribers: emaste, lldb-commits Differential Revision: https://reviews.llvm.org/D46733 llvm-svn: 332115 --- lldb/include/lldb/Target/Process.h | 30 +++++++------------ .../Plugins/Platform/POSIX/PlatformPOSIX.cpp | 30 ++++++++----------- .../Plugins/Platform/POSIX/PlatformPOSIX.h | 8 ++--- lldb/source/Target/Process.cpp | 12 ++++---- 4 files changed, 31 insertions(+), 49 deletions(-) diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index 512cb020ce68..326c771d3763 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -804,7 +804,7 @@ public: } //------------------------------------------------------------------ - // FUTURE WORK: {Set,Get}LoadImageUtilityFunction are the first use we've + // FUTURE WORK: GetLoadImageUtilityFunction are the first use we've // had of having other plugins cache data in the Process. This is handy for // long-living plugins - like the Platform - which manage interactions whose // lifetime is governed by the Process lifetime. If we find we need to do @@ -819,35 +819,24 @@ public: // // We are postponing designing this till we have at least a second use case. //------------------------------------------------------------------ - //------------------------------------------------------------------ - /// Set the cached UtilityFunction that assists in loading binary images - /// into the process. - /// - /// This UtilityFunction is maintained in the Process since the Platforms - /// don't track the lifespan of the Targets/Processes that use them. But - /// it is not intended to be comprehended by the Process, it's up to the - /// Platform that set it to do it right. - /// - /// @param[in] utility_func_up - /// The incoming utility_function. The process will manage the function's - /// lifetime. - /// - //------------------------------------------------------------------ - void SetLoadImageUtilityFunction(std::unique_ptr - utility_func_up); - //------------------------------------------------------------------ /// Get the cached UtilityFunction that assists in loading binary images /// into the process. /// /// @param[in] platform /// The platform fetching the UtilityFunction. - /// + /// @param[in] factory + /// A function that will be called only once per-process in a + /// thread-safe way to create the UtilityFunction if it has not + /// been initialized yet. + /// /// @return /// The cached utility function or null if the platform is not the /// same as the target's platform. //------------------------------------------------------------------ - UtilityFunction *GetLoadImageUtilityFunction(Platform *platform); + UtilityFunction *GetLoadImageUtilityFunction( + Platform *platform, + llvm::function_ref()> factory); //------------------------------------------------------------------ /// Get the dynamic loader plug-in for this process. @@ -3127,6 +3116,7 @@ protected: enum { eCanJITDontKnow = 0, eCanJITYes, eCanJITNo } m_can_jit; std::unique_ptr m_dlopen_utility_func_up; + std::once_flag m_dlopen_utility_func_flag_once; size_t RemoveBreakpointOpcodesFromBuffer(lldb::addr_t addr, size_t size, uint8_t *buf) const; diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp index f7a82651022d..9b2c86a5f686 100644 --- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp +++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp @@ -929,10 +929,9 @@ Status PlatformPOSIX::EvaluateLibdlExpression( return Status(); } -UtilityFunction * -PlatformPOSIX::MakeLoadImageUtilityFunction(ExecutionContext &exe_ctx, - Status &error) -{ +std::unique_ptr +PlatformPOSIX::MakeLoadImageUtilityFunction(ExecutionContext &exe_ctx, + Status &error) { // Remember to prepend this with the prefix from // GetLibdlFunctionDeclarations. The returned values are all in // __lldb_dlopen_result for consistency. The wrapper returns a void * but @@ -982,7 +981,6 @@ PlatformPOSIX::MakeLoadImageUtilityFunction(ExecutionContext &exe_ctx, Value value; ValueList arguments; FunctionCaller *do_dlopen_function = nullptr; - UtilityFunction *dlopen_utility_func = nullptr; // Fetch the clang types we will need: ClangASTContext *ast = process->GetTarget().GetScratchClangASTContext(); @@ -1015,9 +1013,7 @@ PlatformPOSIX::MakeLoadImageUtilityFunction(ExecutionContext &exe_ctx, } // We made a good utility function, so cache it in the process: - dlopen_utility_func = dlopen_utility_func_up.get(); - process->SetLoadImageUtilityFunction(std::move(dlopen_utility_func_up)); - return dlopen_utility_func; + return dlopen_utility_func_up; } uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process, @@ -1038,18 +1034,16 @@ uint32_t PlatformPOSIX::DoLoadImage(lldb_private::Process *process, thread_sp->CalculateExecutionContext(exe_ctx); Status utility_error; - - // The UtilityFunction is held in the Process. Platforms don't track the - // lifespan of the Targets that use them, we can't put this in the Platform. - UtilityFunction *dlopen_utility_func - = process->GetLoadImageUtilityFunction(this); + UtilityFunction *dlopen_utility_func; ValueList arguments; FunctionCaller *do_dlopen_function = nullptr; - - if (!dlopen_utility_func) { - // Make the UtilityFunction: - dlopen_utility_func = MakeLoadImageUtilityFunction(exe_ctx, error); - } + + // The UtilityFunction is held in the Process. Platforms don't track the + // lifespan of the Targets that use them, we can't put this in the Platform. + dlopen_utility_func = process->GetLoadImageUtilityFunction( + this, [&]() -> std::unique_ptr { + return MakeLoadImageUtilityFunction(exe_ctx, error); + }); // If we couldn't make it, the error will be in error, so we can exit here. if (!dlopen_utility_func) return LLDB_INVALID_IMAGE_TOKEN; diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.h b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.h index 1235e21f30df..12d6c6209f0a 100644 --- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.h +++ b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.h @@ -199,10 +199,10 @@ protected: EvaluateLibdlExpression(lldb_private::Process *process, const char *expr_cstr, llvm::StringRef expr_prefix, lldb::ValueObjectSP &result_valobj_sp); - - lldb_private::UtilityFunction * - MakeLoadImageUtilityFunction(lldb_private::ExecutionContext &exe_ctx, - lldb_private::Status &error); + + std::unique_ptr + MakeLoadImageUtilityFunction(lldb_private::ExecutionContext &exe_ctx, + lldb_private::Status &error); virtual llvm::StringRef GetLibdlFunctionDeclarations(lldb_private::Process *process); diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index f957645e0789..996782b53786 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -6164,14 +6164,12 @@ Status Process::UpdateAutomaticSignalFiltering() { return Status(); } -UtilityFunction *Process::GetLoadImageUtilityFunction(Platform *platform) { +UtilityFunction *Process::GetLoadImageUtilityFunction( + Platform *platform, + llvm::function_ref()> factory) { if (platform != GetTarget().GetPlatform().get()) return nullptr; + std::call_once(m_dlopen_utility_func_flag_once, + [&] { m_dlopen_utility_func_up = factory(); }); return m_dlopen_utility_func_up.get(); } - -void Process::SetLoadImageUtilityFunction(std::unique_ptr - utility_func_up) { - m_dlopen_utility_func_up.swap(utility_func_up); -} -