Allow libraries to be loaded with RTLD_LOCAL on Unix.

[v.g.vassilev]

This patch doesn't apply for me on top of D33529.

[marsupial]

Works fine here against master, though D33658 did have a conflict.

[joerg]

The bigger question for me is: what depends on the RTLD_GLOBAL behavior? I generally consider using it a design bug. I.e. what breaks if we just change to RTLD_LOCAL?

[marsupial]

From what I can tell, in LLVM the clients of this are solely clang and lldb.
Nothing should break for them as they explicitly search single libraries for symbols.

D33529 attempts to mitigate any problems that might occur for external usage by giving greater control of how symbols are searched.

But moving to RTLD_LOCAL by default though, may cause breakage for clients of the JIT as a normal pattern would be:

1. DynamicLibrary::getPermanentLibrary(nullptr); // synonym for Process = dlopen(nullptr, RTLD_LAZY  | RTLD_GLOBAL);
2. // Other code...
3. DynamicLibrary::getAddressOfSymbol("SomeSymbol"); // synonym for dlsym(Process, "SomeSymbol");

A. If any other libraries were loaded RTLD_LOCAL by default in step 2, step 3 would no longer find the symbols. (though again, D33529 should make it easy to update their implementation).
B. On Windows this behaviour would differ as there is no concept of RTLD_LOCAL and that would have to be tracked by hand if parity was wanted.

I kind of think the cost against A is worth it as the injection of symbols globally is generally bad, but the disparity of behaviour from B gives me pause.
The easiest solution for B would rely on Module Handles being the same across calls which I don't think is documented but perhaps worth the risk?

[v.g.vassilev]

In other words D33529 broke the cases where we dlopen with RTLD_LOCAL (like cling and ROOT) and this should fix it.

@marsupial I've tested these patches and it looks like they do not fix the regression. I'd be interested in solving this issue for the upcoming release. If nothing works I will have to revert D33529 :(

[marsupial]

This has nothing to do with fixing your issue and is solely to allow clients to load via RTLD_LOCAL.
I don't see how D33529 could have broken anything as it is not committed yet. D33529 attempts to address what I understand is your issue.
If it's not working perhaps you could explain or provide a bit more info what doesn't work...(on that thread not here).
Perhaps @pcanal can help with that?

[v.g.vassilev]

I meant to reference D30107 instead of D33529, sorry.

[marsupial]

@joerg Any further thoughts/desire to review this?
Finishing this off would be nice and at least opens the door to using RTLD_LOCAL.

[marsupial]

Ping,

[v.g.vassilev]

This LGTM but I'd like to hear from @lhames.

[marsupial]

@lhames, any thoughts?

include/llvm/Support/DynamicLibrary.h

public:
     /// This function permanently loads the dynamic library at the given path.
     /// The library will only be unloaded when llvm_shutdown() is called.
     /// This returns a valid DynamicLibrary instance on success and an invalid
     /// instance on failure (see isValid()). \p *errMsg will only be modified
     /// if the library fails to load.
     ///
     /// It is safe to call this function multiple times for the same library.
     /// @brief Open a dynamic library permanently.
-    static DynamicLibrary getPermanentLibrary(const char *filename,
-                                              std::string *errMsg = nullptr);
+    /// @param Filename The path to the library.
+    /// @param ErrMsg Any error encountered will be stored here.
+    /// @param RTLocal Open with RTLD_LOCAL flag on platforms supporting dlopen,
+    /// otherwise ignored.
+    static DynamicLibrary getPermanentLibrary(const char *Filename,
+                                              std::string *ErrMsg = nullptr,
+                                              bool RTLocal = false);
+
+    /// @brief Convenience function for using RTLocal without error reporting.
+    static DynamicLibrary getPermanentLibrary(const char *Filename,
+                                              bool RTLocal) {
+      return getPermanentLibrary(Filename, nullptr, RTLocal);
+    }
 
     /// Registers an externally loaded library. The library will be unloaded
     /// when the program terminates.
     ///
     /// It is safe to call this function multiple times for the same library,
     /// though ownership is only taken if there was no error.
     ///
     /// \returns An empty \p DynamicLibrary if the library was already loaded.

lib/Support/DynamicLibrary.cpp

 
 // All methods for HandleSet should be used holding SymbolsMutex.
 class DynamicLibrary::HandleSet {
   typedef std::vector<void *> HandleList;
   HandleList Handles;
   void *Process;
 
 public:
-  static void *DLOpen(const char *Filename, std::string *Err);
+  static void *DLOpen(const char *Filename, std::string *Err, bool Local);
   static void DLClose(void *Handle);
   static void *DLSym(void *Handle, const char *Symbol);
 
   HandleSet() : Process(nullptr) {}
   ~HandleSet();
 
   HandleList::iterator Find(void *Handle) {
     return std::find(Handles.begin(), Handles.end(), Handle);

 }
 
 void DynamicLibrary::AddSymbol(StringRef SymbolName, void *SymbolValue) {
   SmartScopedLock<true> Lock(*SymbolsMutex);
   (*ExplicitSymbols)[SymbolName] = SymbolValue;
 }
 
 DynamicLibrary DynamicLibrary::getPermanentLibrary(const char *FileName,
-                                                   std::string *Err) {
+                                                   std::string *Err, bool Lcl) {
   // Force OpenedHandles to be added into the ManagedStatic list before any
   // ManagedStatic can be added from static constructors in HandleSet::DLOpen.
   HandleSet& HS = *OpenedHandles;
 
-  void *Handle = HandleSet::DLOpen(FileName, Err);
+  void *Handle = HandleSet::DLOpen(FileName, Err, Lcl);
   if (Handle != &Invalid) {
     SmartScopedLock<true> Lock(*SymbolsMutex);
     HS.AddLibrary(Handle, /*IsProcess*/ FileName == nullptr);
   }
 
   return DynamicLibrary(Handle);
 }
 

lib/Support/Unix/DynamicLibrary.inc

for (void *Handle : llvm::reverse(Handles))
     ::dlclose(Handle);
   if (Process)
     ::dlclose(Process);
 
   // llvm_shutdown called, Return to default
   DynamicLibrary::SearchOrder = DynamicLibrary::SO_Linker;
 }
 
-void *DynamicLibrary::HandleSet::DLOpen(const char *File, std::string *Err) {
-  void *Handle = ::dlopen(File, RTLD_LAZY|RTLD_GLOBAL);
+void *DynamicLibrary::HandleSet::DLOpen(const char *File, std::string *Err,
+                                        bool Local) {
+  void *Handle = ::dlopen(File, RTLD_LAZY | (Local ? RTLD_LOCAL : RTLD_GLOBAL));
   if (!Handle) {
-    if (Err) *Err = ::dlerror();
+    if (Err)
+      *Err = ::dlerror();
     return &DynamicLibrary::Invalid;
   }
 
 #ifdef __CYGWIN__
   // Cygwin searches symbols only in the main
   // with the handle of dlopen(NULL, RTLD_GLOBAL).
   if (!File)
     Handle = RTLD_DEFAULT;

lib/Support/Windows/DynamicLibrary.inc

for (void *Handle : llvm::reverse(Handles))
     FreeLibrary(HMODULE(Handle));
 
   // 'Process' should not be released on Windows.
   assert((!Process || Process==this) && "Bad Handle");
   // llvm_shutdown called, Return to default
   DynamicLibrary::SearchOrder = DynamicLibrary::SO_Linker;
 }
 
-void *DynamicLibrary::HandleSet::DLOpen(const char *File, std::string *Err) {
+void *DynamicLibrary::HandleSet::DLOpen(const char *File, std::string *Err,
+                                        bool /*Local*/) {
   // Create the instance and return it to be the *Process* handle
   // simillar to dlopen(NULL, RTLD_LAZY|RTLD_GLOBAL)
   if (!File)
     return &(*OpenedHandles);
 
   SmallVector<wchar_t, MAX_PATH> FileUnicode;
   if (std::error_code ec = windows::UTF8ToUTF16(File, FileUnicode)) {
     SetLastError(ec.value());

unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp

template <class T> static void* PtrFunc(T *Func) {
   Tmp.F = Func;
   return Tmp.P;
 }
 
 static const char *OverloadTestA() { return "OverloadCall"; }
 
 std::string StdString(const char *Ptr) { return Ptr ? Ptr : ""; }
 
-TEST(DynamicLibrary, Overload) {
+static void TestOverload(bool RTLocal = false) {
   {
     std::string Err;
     llvm_shutdown_obj Shutdown;
     DynamicLibrary DL =
-        DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err);
+        DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err, RTLocal);
     EXPECT_TRUE(DL.isValid());
     EXPECT_TRUE(Err.empty());
 
     GetString GS = FuncPtr<GetString>(DL.getAddressOfSymbol("TestA"));
     EXPECT_TRUE(GS != nullptr && GS != &TestA);
     EXPECT_EQ(StdString(GS()), "LibCall");
 
     GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
     EXPECT_TRUE(GS != nullptr && GS != &TestA);
     EXPECT_EQ(StdString(GS()), "LibCall");
 
     DL = DynamicLibrary::getPermanentLibrary(nullptr, &Err);
     EXPECT_TRUE(DL.isValid());
     EXPECT_TRUE(Err.empty());
 
     // Test overloading local symbols does not occur by default
     GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
     EXPECT_TRUE(GS != nullptr && GS == &TestA);
     EXPECT_EQ(StdString(GS()), "ProcessCall");
 
     GS = FuncPtr<GetString>(DL.getAddressOfSymbol("TestA"));
     EXPECT_TRUE(GS != nullptr && GS == &TestA);
     EXPECT_EQ(StdString(GS()), "ProcessCall");
 
-    // Test overloading by forcing library priority when searching for a symbol
+    // Test overloading by forcing a priority when searching for a symbol
+    if (RTLocal) {
+      // SetStrings un-resolved because RTLocal
+      SetStrings SS = FuncPtr<SetStrings>(
+          DynamicLibrary::SearchForAddressOfSymbol("SetStrings"));
+      EXPECT_TRUE(SS == nullptr);
+
+      DynamicLibrary::SearchOrder = DynamicLibrary::SO_LoadedLast;
+
+      // SetStrings now resolved because SO_LoadedLast
+      SS = FuncPtr<SetStrings>(
+          DynamicLibrary::SearchForAddressOfSymbol("SetStrings"));
+      EXPECT_TRUE(SS != nullptr);
+
+      // TestA resolved from Process first
+      GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
+      EXPECT_TRUE(GS != nullptr && GS == &TestA);
+      EXPECT_EQ(StdString(GS()), "ProcessCall");
+    } else {
+      // SetStrings resolved because DynamicLibrary::getPermanentLibrary(nullptr)
+      SetStrings SS = FuncPtr<SetStrings>(
+          DynamicLibrary::SearchForAddressOfSymbol("SetStrings"));
+      EXPECT_TRUE(SS != nullptr);
+
       DynamicLibrary::SearchOrder = DynamicLibrary::SO_LoadedFirst;
+
+      // TestA resolved from Library first
       GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
       EXPECT_TRUE(GS != nullptr && GS != &TestA);
       EXPECT_EQ(StdString(GS()), "LibCall");
+    }
 
     DynamicLibrary::AddSymbol("TestA", PtrFunc(&OverloadTestA));
     GS = FuncPtr<GetString>(DL.getAddressOfSymbol("TestA"));
     EXPECT_TRUE(GS != nullptr && GS != &OverloadTestA);
 
     GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
     EXPECT_TRUE(GS != nullptr && GS == &OverloadTestA);
     EXPECT_EQ(StdString(GS()), "OverloadCall");
   }
   EXPECT_TRUE(FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol(
                   "TestA")) == nullptr);
 
   // Check serach ordering is reset to default after call to llvm_shutdown
   EXPECT_TRUE(DynamicLibrary::SearchOrder == DynamicLibrary::SO_Linker);
 }
 
+TEST(DynamicLibrary, Overload) {
+  TestOverload();
+}
+
+#if defined(HAVE_DLFCN_H) && defined(HAVE_DLOPEN)
+TEST(DynamicLibrary, RTLocal) {
+  TestOverload(true);
+}
+#endif
+
 TEST(DynamicLibrary, Shutdown) {
   std::string A, B;
   {
     std::string Err;
     llvm_shutdown_obj Shutdown;
     DynamicLibrary DL =
         DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err);
     EXPECT_TRUE(DL.isValid());