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

86

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

86

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

86

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

86

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

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

[marsupial]

include/llvm/Support/DynamicLibrary.h

96

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

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.

include/llvm/Support/DynamicLibrary.h

static bool LoadLibraryPermanently(const char *Filename,
       return !getPermanentLibrary(Filename, ErrMsg).isValid();
     }
 
     /// 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);
+    /// \param LongSearch On Unix it is possible to use addPermanentLibrary
+    /// to load a library handle that has been opened with RTLD_LOCAL.  In this
+    /// case passing true for LongSearch will allow the symbol to be found.
+    /// This avoids a 2x penalty for symbol lookup in more common cases where
+    /// RTLD_LOCAL has not been used or if DynamicLibrary::getPermanentLibrary
+    /// is solely used to load libraries.
+    static void *SearchForAddressOfSymbol(const char *SymbolName,
+                                          bool LongSearch = false);
 
     /// @brief Convenience function for C++ophiles.
     static void *SearchForAddressOfSymbol(const std::string &symbolName) {
       return SearchForAddressOfSymbol(symbolName.c_str());
     }
 
     /// This functions permanently adds the symbol \p symbolName with the
     /// value \p symbolValue.  These symbols are searched before any

lib/Support/DynamicLibrary.cpp

#ifndef LLVM_ON_WIN32
           return false;
       }
 #endif
       Process = Handle;
     }
     return true;
   }
 
-  void *Lookup(const char *Symbol) {
+  void *Lookup(const char *Symbol, bool LongSearch) {
     // Process handle gets first try.
     if (Process) {
       if (void *Ptr = DLSym(Process, Symbol))
         return Ptr;
+
+#ifdef LLVM_ON_WIN32
+      // On Windows all loaded modules will have been searched at this point.
+      // Double check in Debug builds then exit.
 #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))
-          return Ptr;
-      }
-    }
+      return nullptr;
+#else
+      // On Unix there is a possibility of a Handles containing a library opened
+      // with RTLD_LOCAL and having it's symbols hidden from lookup via dlsym
+      if (!LongSearch)
+        return nullptr;
+#endif
+    }
+
     return nullptr;
   }
 };
 
 namespace {
 // Collection of symbol name/value pairs to be searched prior to any libraries.
 static llvm::ManagedStatic<llvm::StringMap<void *>> ExplicitSymbols;
 // Collection of known library handles.

 }
 
 void *DynamicLibrary::getAddressOfSymbol(const char *SymbolName) {
   if (!isValid())
     return nullptr;
   return HandleSet::DLSym(Data, SymbolName);
 }
 
-void *DynamicLibrary::SearchForAddressOfSymbol(const char *SymbolName) {
+void *DynamicLibrary::SearchForAddressOfSymbol(const char *SymbolName,
+                                               bool LongSearch) {
   {
     SmartScopedLock<true> Lock(*SymbolsMutex);
 
     // First check symbols added via AddSymbol().
     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, LongSearch))
         return Ptr;
     }
   }
 
   return llvm::SearchForAddressOfSpecialSymbol(SymbolName);
 }
 
 //===----------------------------------------------------------------------===//