Allow clients to specify search order of DynamicLibraries.

[marsupial]

Summary

Different JITs and other clients of LLVM may have different needs in how symbol resolution should occur.

[marsupial]

I'm not actually sure I'm in favor of this but wanted another opinion.
To me DynamicLibrary::addPermanentLibrary doesn't really seem all that useful and actually complicates things a bit.

What is the reasoning of it existence?
If adding libraries with RTLD_LOCAL is important, could that not be accomplished with an additional argument to DynamicLibrary::getPermanentLibrary?

[v.g.vassilev]

@marsupial, the reasoning for this is that we do not want to expose the symbols to our experiments software stacks because they have other uses of llvm. dlopening cling as libcling allows to hide symbols avoiding potential versioning and other issues.

[v.g.vassilev]

In D33529#764012, @marsupial wrote:

What is the reasoning of it existence?
If adding libraries with RTLD_LOCAL is important, could that not be accomplished with an additional argument to DynamicLibrary::getPermanentLibrary?

We had this discussion in another review. The dlopen has to be outside, because DynamicLibrary::getPermanentLibrary symbol is not available until you call dlopen :)

[v.g.vassilev]

lib/Support/DynamicLibrary.cpp

85 ↗

(On Diff #100191)

I am a little confused by this section here. This seems well optimized for binaries which are statically linked.

IIUC we disallow searching symbols from the dlopened from the process libraries? Why?

[marsupial]

lib/Support/DynamicLibrary.cpp

85 ↗

(On Diff #100191)

Yes it's a bit uglier than needed...I can fix that.

The point is ROOT seems to be the only user of addPermanentLibrary which is the only way to inject a handle of RTLD_LOCAL.

As such searching the Process handle will in cases other than ROOT will have already searched all libs that are in Handles. Which means you've slowed things down for nonexistent symbols 2x for all users to accommodate your special needs.

I can understand that you have a desire outside normal usage in LLVM but think it's better to have an argument to explicitly search any RTLD_LOCAL in Handles rather than require it for everyone.

[v.g.vassilev]

lib/Support/DynamicLibrary.cpp

85 ↗

(On Diff #100191)

As such searching the Process handle will in cases other than ROOT will have already searched all libs that are in Handles. Which means you've slowed things down for nonexistent symbols 2x for all users to accommodate your special needs.

Hm... It looks like you are arguing both ways here: you sold the initial version of the patch as 'improving' the situation for cling and its use-cases. Now you are essentially saying that you don't care if you broke it as this would slowdown 'hypothetical' users who resolve symbols from the process and have dlopened libraries. Oddly, I am not aware of any such complaints from those users as the previous version of the code was doing exactly this.

Over the years ROOT has been one of the (if not the only) few use cases of this code path. We have extended this several times and your refactoring is breaking our use-case. To give you context: ROOT is used in many domains outside high-energy physics estimating over 10K users, excluding the CERN LHC program.

[marsupial]

lib/Support/DynamicLibrary.cpp

85 ↗

(On Diff #100191)

Can we keep it civil?
I 'sold' nothing as improving cling nor said I don't care if it breaks anything.

I'm offering a solution that will fix the problem for you, not at the cost for every other LLVM user and have merely asked for someone outside of ROOT to comment on the current situation.

Other users of the JIT does symbol resolution this way and they also have users with tangible numbers.

[marsupial]

Formatting.

[marsupial]

Add tests for symbol resolution order.

[pcanal]

The new version looks promising. We are going to give a try. Thanks.

[v.g.vassilev]

That looks very good. Can we add a test case with dlopening a library with RTLD_LOCAL.

I plan to test whether this patch is enough for us later this week.

[marsupial]

In D33529#767045, @v.g.vassilev wrote:

That looks very good. Can we add a test case with dlopening a library with RTLD_LOCAL.

I'd like to avoid adding the test at this moment.
I have subsequent patches to make DynamicLibrary a more usable class without adding them permanently.
In that is the ability to open with RTLD_LOCAL as well as tests for it.

[marsupial]

Restore default ordering after llvm_shutdown called.

[marsupial]

Spelling.

[pcanal]

include/llvm/Support/DynamicLibrary.h

96 ↗

(On Diff #100637)

Did you mean "Search all loaded libraries as the SO_Linker would" ?

[marsupial]

include/llvm/Support/DynamicLibrary.h

96 ↗

(On Diff #100637)

No, its what I think you are for.
The process does not have priority.

BTW, In case it wasn't clear: using this on Windows can cause real problems as a lot of C++ runtime can be exported via the binary (though I understand you want to preserve behavior ROOT has on Unix).

[pcanal]

include/llvm/Support/DynamicLibrary.h

96 ↗

(On Diff #100637)

No, its what I think you are for.

My bad. I mis-parsed the sentence, maybe adding a comma would have helped me: "Search all loaded libraries, then as the SO_Linker would".

Another point you might consider clarifying in the doc is whether the search of the libraries in SO_LoadedFirst and SO_LoadedLast is done in load order ('oldest first') or reverse load order ('newest first') [which is the closest to 'dependency order')

[marsupial]

Allow forward and backward search of loaded modules.

[marsupial]

Ping.

[v.g.vassilev]

Currently I am busy with other things and I would be able to test this beginning on July when I plan to upgrade LLVM again. Sorry...

[marsupial]

Typo & reword documentation.

[marsupial]

@v.g.vassilev @pcanal :
Can you provide a concrete example of a test in ROOT that this patch does not solve?

[v.g.vassilev]

@marsupial, thanks for this. With a little tweaking I can say it fixes our use case.

@lhames, that seems to address what we discussed on irc. This covers our use case.

llvm/trunk/include/llvm/Support/DynamicLibrary.h

public:
     /// symbols from the library itself.
     ///
     /// It is safe to call this function multiple times for the same library.
     static bool LoadLibraryPermanently(const char *Filename,
                                        std::string *ErrMsg = nullptr) {
       return !getPermanentLibrary(Filename, ErrMsg).isValid();
     }
 
+    enum SearchOrdering {
+      /// SO_Linker - Search as a call to dlsym(dlopen(NULL)) would when
+      /// DynamicLibrary::getPermanentLibrary(NULL) has been called or
+      /// search the list of explcitly loaded symbols if not.
+      SO_Linker,
+      /// SO_LoadedFirst - Search all loaded libraries, then as SO_Linker would.
+      SO_LoadedFirst,
+      /// SO_LoadedLast - Search as SO_Linker would, then loaded libraries.
+      /// Only useful to search if libraries with RTLD_LOCAL have been added.
+      SO_LoadedLast,
+      /// SO_LoadOrder - Or this in to search libraries in the ordered loaded.
+      /// The default bahaviour is to search loaded libraries in reverse.
+      SO_LoadOrder = 4
+    };
+    static SearchOrdering SearchOrder; // = SO_Linker
+
     /// This function will search through all previously loaded dynamic
     /// libraries for the symbol \p symbolName. If it is found, the address of
     /// that symbol is returned. If not, null is returned. Note that this will
     /// search permanently loaded libraries (getPermanentLibrary()) as well
     /// as explicitly registered symbols (AddSymbol()).
     /// @throws std::string on error.
     /// @brief Search through libraries for address of a symbol
     static void *SearchForAddressOfSymbol(const char *symbolName);

llvm/trunk/lib/Support/DynamicLibrary.cpp

 //===-- DynamicLibrary.cpp - Runtime link/load libraries --------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the operating system DynamicLibrary concept.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm-c/Support.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Mutex.h"
 #include <cstdio>
 #include <cstring>
 #include <vector>
 
#ifndef LLVM_ON_WIN32
           return false;
       }
 #endif
       Process = Handle;
     }
     return true;
   }
 
-  void *Lookup(const char *Symbol) {
-    // Process handle gets first try.
+  void *LibLookup(const char *Symbol, DynamicLibrary::SearchOrdering Order) {
+    if (Order & SO_LoadOrder) {
+      for (void *Handle : Handles) {
+        if (void *Ptr = DLSym(Handle, Symbol))
+          return Ptr;
+      }
+    } else {
+      for (void *Handle : llvm::reverse(Handles)) {
+        if (void *Ptr = DLSym(Handle, Symbol))
+          return Ptr;
+      }
+    }
+    return nullptr;
+  }
+
+  void *Lookup(const char *Symbol, DynamicLibrary::SearchOrdering Order) {
+    assert(!((Order & SO_LoadedFirst) && (Order & SO_LoadedLast)) &&
+           "Invalid Ordering");
+
+    if (!Process || (Order & SO_LoadedFirst)) {
+      if (void *Ptr = LibLookup(Symbol, Order))
+        return Ptr;
+    }
     if (Process) {
+      // Use OS facilities to search the current binary and all loaded libs.
       if (void *Ptr = DLSym(Process, Symbol))
         return Ptr;
-#ifndef NDEBUG
-      for (void *Handle : Handles)
-        assert(!DLSym(Handle, Symbol) && "Symbol exists in non process handle");
-#endif
-    } else {
-      // Iterate in reverse, so newer libraries/symbols override older.
-      for (auto &&I = Handles.rbegin(), E = Handles.rend(); I != E; ++I) {
-        if (void *Ptr = DLSym(*I, Symbol))
+
+      // Search any libs that might have been skipped because of RTLD_LOCAL.
+      if (Order & SO_LoadedLast) {
+        if (void *Ptr = LibLookup(Symbol, Order))
           return Ptr;
       }
     }
     return nullptr;
   }
 };
 
 namespace {

 
 #else
 
 #include "Unix/DynamicLibrary.inc"
 
 #endif
 
 char DynamicLibrary::Invalid;
+DynamicLibrary::SearchOrdering DynamicLibrary::SearchOrder =
+    DynamicLibrary::SO_Linker;
 
 namespace llvm {
 void *SearchForAddressOfSpecialSymbol(const char *SymbolName) {
   return DoSearch(SymbolName); // DynamicLibrary.inc
 }
 }
 
 void DynamicLibrary::AddSymbol(StringRef SymbolName, void *SymbolValue) {
if (ExplicitSymbols.isConstructed()) {
       StringMap<void *>::iterator i = ExplicitSymbols->find(SymbolName);
 
       if (i != ExplicitSymbols->end())
         return i->second;
     }
 
     // Now search the libraries.
     if (OpenedHandles.isConstructed()) {
-      if (void *Ptr = OpenedHandles->Lookup(SymbolName))
+      if (void *Ptr = OpenedHandles->Lookup(SymbolName, SearchOrder))
         return Ptr;
     }
   }
 
   return llvm::SearchForAddressOfSpecialSymbol(SymbolName);
 }
 
 //===----------------------------------------------------------------------===//

llvm/trunk/lib/Support/Unix/DynamicLibrary.inc

 #include <dlfcn.h>
 
 DynamicLibrary::HandleSet::~HandleSet() {
   // Close the libraries in reverse order.
   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);
   if (!Handle) {
     if (Err) *Err = ::dlerror();
     return &DynamicLibrary::Invalid;
   }

llvm/trunk/lib/Support/Windows/DynamicLibrary.inc

 
 
 DynamicLibrary::HandleSet::~HandleSet() {
   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) {
   // Create the instance and return it to be the *Process* handle
   // simillar to dlopen(NULL, RTLD_LAZY|RTLD_GLOBAL)
   if (!File)
     return &(*OpenedHandles);
 

llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp

TEST(DynamicLibrary, Overload) {
     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
+    DynamicLibrary::SearchOrder = DynamicLibrary::SO_LoadedFirst;
+    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, Shutdown) {
   std::string A("PipSqueak"), B, C("SecondLib");
   std::vector<std::string> Order;
   {
     std::string Err;
     llvm_shutdown_obj Shutdown;