[PATCH] D33529: Allow Unix libraries loaded with RTLD_LOCAL to be searched.

[Frederich Munch via Phabricator via llvm-commits]

marsupial updated this revision to Diff 100191.
marsupial added a comment.

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**?


https://reviews.llvm.org/D33529

Files:
  include/llvm/Support/DynamicLibrary.h
  lib/Support/DynamicLibrary.cpp


Index: lib/Support/DynamicLibrary.cpp
===================================================================
--- lib/Support/DynamicLibrary.cpp
+++ lib/Support/DynamicLibrary.cpp
@@ -73,21 +73,26 @@
     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;
-#ifndef NDEBUG
+      // On Windows all loaded modules will have been searched at this point.
+      // On Unix there is a possibility of a Handle having it's symbols hidden
+      // from OS lookup via dlsym.
+#if defined(LLVM_ON_WIN32) && !defined(NDEBUG)
       for (void *Handle : Handles)
         assert(!DLSym(Handle, Symbol) && "Symbol exists in non process handle");
+#else
+      if (!LongSearch)
 #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;
+    }
+    // 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;
   }
@@ -151,7 +156,8 @@
   return HandleSet::DLSym(Data, SymbolName);
 }
 
-void *DynamicLibrary::SearchForAddressOfSymbol(const char *SymbolName) {
+void *DynamicLibrary::SearchForAddressOfSymbol(const char *SymbolName,
+                                               bool LongSearch) {
   {
     SmartScopedLock<true> Lock(*SymbolsMutex);
 
@@ -165,7 +171,7 @@
 
     // Now search the libraries.
     if (OpenedHandles.isConstructed()) {
-      if (void *Ptr = OpenedHandles->Lookup(SymbolName))
+      if (void *Ptr = OpenedHandles->Lookup(SymbolName, LongSearch))
         return Ptr;
     }
   }
Index: include/llvm/Support/DynamicLibrary.h
===================================================================
--- include/llvm/Support/DynamicLibrary.h
+++ include/llvm/Support/DynamicLibrary.h
@@ -93,9 +93,17 @@
     /// 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) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D33529.100191.patch
Type: text/x-patch
Size: 3282 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170525/d09850b3/attachment.bin>