Expand testing coverage to cover the dynamic library manager

Author: vgvassilev

include/clang/Interpreter/CppInterOp.h

@@ -340,7 +340,10 @@ namespace Cpp {
   /// Checks if the provided parameter is a 'Static' method.
   bool IsStaticMethod(TCppConstFunction_t method);
 
-  /// Gets the address of the function to be able to call it.
+  ///\returns the address of the function given its potentially mangled name.
+  TCppFuncAddr_t GetFunctionAddress(const char* mangled_name);
+
+  ///\returns the address of the function given its function declaration.
   TCppFuncAddr_t GetFunctionAddress(TCppFunction_t method);
 
   /// Checks if the provided parameter is a 'Virtual' method.
@@ -461,9 +464,21 @@ namespace Cpp {
   ///\returns the path to the library.
   const std::string LookupLibrary(const char *lib_name);
 
-  /// Loads the library based on the path returned by the LookupLibrary()
+  /// Finds \c lib_stem considering the list of search paths and loads it by
+  /// calling dlopen.
+  /// \returns true on success.
+  bool LoadLibrary(const char* lib_stem, bool lookup = true);
+
+  /// Finds \c lib_stem considering the list of search paths and unloads it by
+  /// calling dlclose.
   /// function.
-  bool LoadLibrary(const char *lib_path, bool lookup = true);
+  void UnloadLibrary(const char* lib_stem);
+
+  /// Scans all libraries on the library search path for a given potentially
+  /// mangled symbol name.
+  ///\returns the path to the first library that contains the symbol definition.
+  std::string SearchLibrariesForSymbol(const char* mangled_name,
+                                       bool search_system /*true*/);
 
   /// Inserts or replaces a symbol in the JIT with the one provided. This is
   /// useful for providing our own implementations of facilities such as printf.

lib/Interpreter/Compatibility.h

@@ -46,7 +46,11 @@ inline llvm::orc::LLJIT* getExecutionEngine(const cling::Interpreter& I) {
   // to directly.
 
   // sizeof (m_Opts) + sizeof(m_LLVMContext)
+#ifdef __APPLE__
+  const unsigned m_ExecutorOffset = 62;
+#else
   const unsigned m_ExecutorOffset = 72;
+#endif // __APPLE__
   int* IncrementalExecutor =
       ((int*)(const_cast<cling::Interpreter*>(&I))) + m_ExecutorOffset;
   int* IncrementalJit = *(int**)IncrementalExecutor + 0;

lib/Interpreter/CppInterOp.cpp

@@ -980,6 +980,17 @@ namespace Cpp {
     return false;
   }
 
+  TCppFuncAddr_t GetFunctionAddress(const char* mangled_name) {
+    auto& I = getInterp();
+    auto FDAorErr = compat::getSymbolAddress(I, mangled_name);
+    if (llvm::Error Err = FDAorErr.takeError())
+      llvm::consumeError(std::move(Err)); // nullptr if missing
+    else
+      return llvm::jitTargetAddressToPointer<void*>(*FDAorErr);
+
+    return nullptr;
+  }
+
   TCppFuncAddr_t GetFunctionAddress(TCppFunction_t method)
   {
     auto *D = (Decl *) method;
@@ -1002,14 +1013,8 @@ namespace Cpp {
       return mangled_name;
     };
 
-    auto &I = getInterp();
-    if (auto *FD = llvm::dyn_cast_or_null<FunctionDecl>(D)) {
-      auto FDAorErr = compat::getSymbolAddress(I, StringRef(get_mangled_name(FD)));
-      if (llvm::Error Err = FDAorErr.takeError())
-        llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "Failed to GetFunctionAdress:");
-      else
-        return llvm::jitTargetAddressToPointer<void*>(*FDAorErr);
-    }
+    if (auto* FD = llvm::dyn_cast_or_null<FunctionDecl>(D))
+      return GetFunctionAddress(get_mangled_name(FD).c_str());
 
     return 0;
   }
@@ -2588,13 +2593,23 @@ namespace Cpp {
     return getInterp().getDynamicLibraryManager()->lookupLibrary(lib_name);
   }
 
-  bool LoadLibrary(const char *lib_name, bool lookup) {
+  bool LoadLibrary(const char* lib_stem, bool lookup) {
     compat::Interpreter::CompilationResult res =
-      getInterp().loadLibrary(lib_name, lookup);
+        getInterp().loadLibrary(lib_stem, lookup);
 
     return res == compat::Interpreter::kSuccess;
   }
 
+  void UnloadLibrary(const char* lib_stem) {
+    getInterp().getDynamicLibraryManager()->unloadLibrary(lib_stem);
+  }
+
+  std::string SearchLibrariesForSymbol(const char* mangled_name,
+                                       bool search_system /*true*/) {
+    auto* DLM = getInterp().getDynamicLibraryManager();
+    return DLM->searchLibrariesForSymbol(mangled_name, search_system);
+  }
+
   bool InsertOrReplaceJitSymbol(const char* linker_mangled_name,
                                 uint64_t address) {
     // FIXME: This approach is problematic since we could replace a symbol

lib/Interpreter/CppInterOpInterpreter.h

@@ -330,8 +330,12 @@ class Interpreter {
 
   const DynamicLibraryManager* getDynamicLibraryManager() const {
     assert(compat::getExecutionEngine(*inner) && "We must have an executor");
-    static const DynamicLibraryManager* DLM = new DynamicLibraryManager();
-    return DLM;
+    static std::unique_ptr<DynamicLibraryManager> DLM = nullptr;
+    if (!DLM) {
+      DLM.reset(new DynamicLibraryManager());
+      DLM->initializeDyld([](llvm::StringRef) { /*ignore*/ return false; });
+    }
+    return DLM.get();
     // TODO: Add DLM to InternalExecutor and use executor->getDML()
     //      return inner->getExecutionEngine()->getDynamicLibraryManager();
   }

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

@@ -2,7 +2,49 @@
 
 #include "clang/Interpreter/CppInterOp.h"
 
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+
+// This function isn't referenced outside its translation unit, but it
+// can't use the "static" keyword because its address is used for
+// GetMainExecutable (since some platforms don't support taking the
+// address of main, and some platforms can't implement GetMainExecutable
+// without being given the address of a function in the main executable).
+std::string GetExecutablePath(const char* Argv0) {
+  // This just needs to be some symbol in the binary; C++ doesn't
+  // allow taking the address of ::main however.
+  void* MainAddr = (void*)intptr_t(GetExecutablePath);
+  return llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
+}
+
 TEST(DynamicLibraryManagerTest, Sanity) {
   EXPECT_TRUE(Cpp::CreateInterpreter());
-  EXPECT_TRUE(Cpp::LoadLibrary("TestSharedLib"));
+  EXPECT_FALSE(Cpp::GetFunctionAddress("ret_zero"));
+
+  std::string BinaryPath = GetExecutablePath(/*Argv0=*/nullptr);
+  llvm::StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
+  Cpp::AddSearchPath(Dir.str().c_str());
+
+  // FIXME: dlsym on mach-o takes the C-level name, however, the macho-o format
+  // adds an additional underscore (_) prefix to the lowered names. Figure out
+  // how to harmonize that API.
+#ifdef __APPLE__
+  std::string PathToTestSharedLib =
+      Cpp::SearchLibrariesForSymbol("_ret_zero", /*system_search=*/false);
+#else
+  std::string PathToTestSharedLib =
+      Cpp::SearchLibrariesForSymbol("ret_zero", /*system_search=*/false);
+#endif // __APPLE__
+
+  EXPECT_STRNE("", PathToTestSharedLib.c_str())
+      << "Cannot find: '" << PathToTestSharedLib << "' in '" << Dir.str()
+      << "'";
+
+  EXPECT_TRUE(Cpp::LoadLibrary(PathToTestSharedLib.c_str()));
+  EXPECT_TRUE(Cpp::GetFunctionAddress("ret_zero"));
+  Cpp::UnloadLibrary("TestSharedLib");
+  // We have no reliable way to check if it was unloaded because posix does not
+  // require the library to be actually unloaded but just the handle to be
+  // invalidated...
+  // EXPECT_FALSE(Cpp::GetFunctionAddress("ret_zero"));
 }

unittests/CppInterOp/TestSharedLib/TestSharedLib.h

@@ -1,6 +1,7 @@
 #ifndef UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB_H
 #define UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB_H
 
-int ret_zero();
+// Avoid having to mangle/demangle the symbol name in tests.
+extern "C" int ret_zero();
 
 #endif // UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB_H

unittests/CppInterOp/Utils.h

@@ -1,7 +1,7 @@
 #ifndef CPPINTEROP_UNITTESTS_LIBCPPINTEROP_UTILS_H
 #define CPPINTEROP_UNITTESTS_LIBCPPINTEROP_UTILS_H
 
-#include "../../../lib/Interpreter/Compatibility.h"
+#include "../../lib/Interpreter/Compatibility.h"
 
 #include <memory>
 #include <vector>