Add a lock to PlatformPOSIX::DoLoadImage

[friss]

Summary

Multiple threads could be calling into DoLoadImage concurrently,
only one should be allowed to create the UtilityFunction.

[clayborg]

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

1046–1047 ↗

(On Diff #146252)

We should put the mutex, or better yet a std::once_flag, in the process as an instance variable. The utility function belongs to each process.

[friss]

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

1046–1047 ↗

(On Diff #146252)

I don't disagree, but it seemed a little overkill. Would you just put it as a public member? Or add a GetLoadImageUtilityFunctionOnceFlag() accessor?

[clayborg]

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

1047 ↗

(On Diff #146252)

Accessor would be fine. The other reason for putting this in the process is multi-threaded code that attempts to use this after the main thread has exited will crash as the static mutex might destruct itself.

[friss]

I heted the idea of hosting the once_flag in Process but using
it in PlatformPOSIX. I did a bigger refactoring where platforms
pass a factory lambda to the accessor and the thread-safe init
is done in the Process class itself using this lambda.

[jingham]

That looks fine to me.

[clayborg]

Much better.

lldb/trunk/include/lldb/Target/Process.h

virtual Status DoLoadCore() {
     Status error;
     error.SetErrorStringWithFormat(
         "error: %s does not support loading core files.",
         GetPluginName().GetCString());
     return error;
   }
 
   //------------------------------------------------------------------
-  // 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
   // this more often, we should construct a general solution to the problem.
   // The consensus suggestion was that we have a token based registry in the
   // Process. Some undecided questions are  (1) who manages the tokens.  It's
   // probably best that you add the element  and get back a token that
   // represents it.  That will avoid collisions.  But there may be some utility
   // in the registerer controlling the token? (2) whether the thing added
   // should be simply owned by Process, and just go away when it does (3)
   // whether the registree should be notified of the Process' demise.
   //
   // 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<UtilityFunction> 
-                                   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<std::unique_ptr<UtilityFunction>()> factory);
 
   //------------------------------------------------------------------
   /// Get the dynamic loader plug-in for this process.
   ///
   /// The default action is to let the DynamicLoader plug-ins check the main
   /// executable and the DynamicLoader will select itself automatically.
   /// Subclasses can override this if inspecting the executable is not
   /// desired, or if Process subclasses can only use a specific DynamicLoader
protected:
   WarningsCollection m_warnings_issued; // A set of object pointers which have
                                         // already had warnings printed
   std::mutex m_run_thread_plan_lock;
   StructuredDataPluginMap m_structured_data_plugin_map;
 
   enum { eCanJITDontKnow = 0, eCanJITYes, eCanJITNo } m_can_jit;
   
   std::unique_ptr<UtilityFunction> 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;
 
   void SynchronouslyNotifyStateChanged(lldb::StateType state);
 
   void SetPublicState(lldb::StateType new_state, bool restarted);
 

lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.h

std::map<lldb_private::CommandInterpreter *,
       m_options;
   lldb::PlatformSP m_remote_platform_sp; // Allow multiple ways to connect to a
                                          // remote POSIX-compliant OS
 
   lldb_private::Status
   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<lldb_private::UtilityFunction>
+  MakeLoadImageUtilityFunction(lldb_private::ExecutionContext &exe_ctx,
+                               lldb_private::Status &error);
 
   virtual
   llvm::StringRef GetLibdlFunctionDeclarations(lldb_private::Process *process);
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformPOSIX);
 };
 
 #endif // liblldb_PlatformPOSIX_h_

lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

Status PlatformPOSIX::EvaluateLibdlExpression(
   if (result != eExpressionCompleted)
     return expr_error;
 
   if (result_valobj_sp->GetError().Fail())
     return result_valobj_sp->GetError();
   return Status();
 }
 
-UtilityFunction *
-PlatformPOSIX::MakeLoadImageUtilityFunction(ExecutionContext &exe_ctx, 
-                                            Status &error)
-{
+std::unique_ptr<UtilityFunction>
+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
   // doesn't use it because UtilityFunctions don't work with void returns at
   // present.
   static const char *dlopen_wrapper_code = R"(
   struct __lldb_dlopen_result {
     void *image_ptr;
error.SetErrorStringWithFormat("dlopen error: could not install utility"
                                    "function: %s", 
                                    diagnostics.GetString().c_str());
     return nullptr;
   }
 
   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();
 
   CompilerType clang_void_pointer_type
       = ast->GetBasicType(eBasicTypeVoid).GetPointerType();
   CompilerType clang_char_pointer_type
         = ast->GetBasicType(eBasicTypeChar).GetPointerType();
PlatformPOSIX::MakeLoadImageUtilityFunction(ExecutionContext &exe_ctx,
   
   do_dlopen_function = dlopen_utility_func_up->GetFunctionCaller();
   if (!do_dlopen_function) {
     error.SetErrorString("dlopen error: could not get function caller.");
     return nullptr;
   }
   
   // 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,
                                     const lldb_private::FileSpec &remote_file,
                                     lldb_private::Status &error) {
   std::string path;
   path = remote_file.GetPath();
   
   ThreadSP thread_sp = process->GetThreadList().GetExpressionExecutionThread();
   if (!thread_sp) {
     error.SetErrorString("dlopen error: no thread available to call dlopen.");
     return LLDB_INVALID_IMAGE_TOKEN;
   }
   
   DiagnosticManager diagnostics;
   
   ExecutionContext exe_ctx;
   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<UtilityFunction> {
+        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;
     
   do_dlopen_function = dlopen_utility_func->GetFunctionCaller();
   if (!do_dlopen_function) {
     error.SetErrorString("dlopen error: could not get function caller.");
     return LLDB_INVALID_IMAGE_TOKEN;

lldb/trunk/source/Target/Process.cpp

 }
 
 Status Process::UpdateAutomaticSignalFiltering() {
   // Default implementation does nothign.
   // No automatic signal filtering to speak of.
   return Status();
 }
 
-UtilityFunction *Process::GetLoadImageUtilityFunction(Platform *platform) {
+UtilityFunction *Process::GetLoadImageUtilityFunction(
+    Platform *platform,
+    llvm::function_ref<std::unique_ptr<UtilityFunction>()> 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<UtilityFunction> 
-                                          utility_func_up) {
-  m_dlopen_utility_func_up.swap(utility_func_up);
-}
-