Fix

Author: alexander-penev

include/clang/Interpreter/CppInterOp.h

@@ -667,7 +667,7 @@ namespace Cpp {
   ///\param[in] autoload - true = libraries autoload is on.
   CPPINTEROP_API void SetLibrariesAutoload(bool autoload = true);
 
-  /// Get libraries autoload.
+  /// Get libraries autoload status.
   ///\returns LibraryAutoLoad state (true = libraries autoload is on).
   CPPINTEROP_API bool GetLibrariesAutoload();
 

lib/Interpreter/CppInterOp.cpp

@@ -58,6 +58,10 @@ namespace Cpp {
   using namespace std;
 
   static std::unique_ptr<compat::Interpreter> sInterpreter;
+  // Last assigned Autoload SearchGenerator
+  // TODO: Test for thread safe.
+  class AutoLoadLibrarySearchGenerator;
+  static AutoLoadLibrarySearchGenerator *sAutoSG = nullptr;
   // Valgrind complains about __cxa_pure_virtual called when deleting
   // llvm::SectionMemoryManager::~SectionMemoryManager as part of the dtor chain
   // of the Interpreter.
@@ -1073,9 +1077,9 @@ namespace Cpp {
     auto FDAorErr = compat::getSymbolAddress(I, mangled_name);
     if (llvm::Error Err = FDAorErr.takeError())
       llvm::consumeError(std::move(Err)); // nullptr if missing
-    else
+    else {
       return llvm::jitTargetAddressToPointer<void*>(*FDAorErr);
-
+    }
     return nullptr;
   }
 
@@ -2534,7 +2538,7 @@ namespace Cpp {
     std::string BinaryPath = GetExecutablePath(/*Argv0=*/nullptr, MainAddr);
 
     // build/tools/clang/unittests/Interpreter/Executable -> build/
-    StringRef Dir = sys::path::parent_path(BinaryPath);
+    Dir = sys::path::parent_path(BinaryPath);
 
     Dir = sys::path::parent_path(Dir);
     Dir = sys::path::parent_path(Dir);
@@ -2609,6 +2613,7 @@ namespace Cpp {
     // calls to CreateInterpreter.
     //assert(!sInterpreter && "Interpreter already set.");
     sInterpreter.reset(I);
+    sAutoSG = nullptr;
     return I;
   }
 
@@ -3286,12 +3291,6 @@ namespace Cpp {
     return nameForDlsym;
   }
 
-  class AutoLoadLibrarySearchGenerator;
-
-  // Last assigned Autoload SearchGenerator
-  // TODO: Test for thread safe.
-  static AutoLoadLibrarySearchGenerator *AutoSG = nullptr;
-
   class AutoLoadLibrarySearchGenerator : public llvm::orc::DefinitionGenerator {
     bool Enabled = false;
   public:
@@ -3308,15 +3307,15 @@ namespace Cpp {
       std::string lib;
       llvm::orc::SymbolNameVector syms;
     public:
-      AutoloadLibraryMU(const std::string& Library, const llvm::orc::SymbolNameVector &Symbols)
-        : MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), lib(Library), syms(Symbols) {}
+      AutoloadLibraryMU(std::string Library, const llvm::orc::SymbolNameVector &Symbols)
+        : MaterializationUnit({getSymbolFlagsMap(Symbols), nullptr}), lib(std::move(Library)), syms(Symbols) {}
 
-      [[nodiscard]] StringRef getName() const override {
+      StringRef getName() const override {
         return "<Symbols from Autoloaded Library>";
       }
 
       void materialize(std::unique_ptr<llvm::orc::MaterializationResponsibility> R) override {
-        if (!AutoSG || !AutoSG->isEnabled()) {
+        if (!sAutoSG || !sAutoSG->isEnabled()) {
           R->failMaterialization();
           return;
         }
@@ -3390,37 +3389,37 @@ namespace Cpp {
 
     llvm::Error tryToGenerate(llvm::orc::LookupState &LS, llvm::orc::LookupKind K, llvm::orc::JITDylib &JD,
                               llvm::orc::JITDylibLookupFlags JDLookupFlags, const llvm::orc::SymbolLookupSet &Symbols) override {
-      if (isEnabled()) {
-        LLVM_DEBUG(dbgs() << "tryToGenerate");
-
-        auto& I = getInterp();
-        auto *DLM = I.getDynamicLibraryManager();
-
-        std::unordered_map<std::string, llvm::orc::SymbolNameVector> found;
-        llvm::orc::SymbolMap NewSymbols;
-        for (const auto &KV : Symbols) {
-          const auto &Name = KV.first;
-          if ((*Name).empty())
-            continue;
-
-          auto lib = DLM->searchLibrariesForSymbol(*Name, /*searchSystem=*/true); // false?
-          if (lib.empty())
-            continue;
-
-          found[lib].push_back(Name);
-
-          // Workaround: This getAddressOfGlobal call make first symbol search
-          // to work, immediatelly after library auto load. This approach do not
-          // use MU
-          //DLM->loadLibrary(lib, true);
-          //I.getAddressOfGlobal(*Name);
-        }
+      if (!isEnabled())
+	return llvm::Error::success();
+      LLVM_DEBUG(dbgs() << "tryToGenerate");
+
+      auto& I = getInterp();
+      auto *DLM = I.getDynamicLibraryManager();
+
+      std::unordered_map<std::string, llvm::orc::SymbolNameVector> found;
+      llvm::orc::SymbolMap NewSymbols;
+      for (const auto &KV : Symbols) {
+	const auto &Name = KV.first;
+	if ((*Name).empty())
+	  continue;
+
+	auto lib = DLM->searchLibrariesForSymbol(*Name, /*searchSystem=*/true); // false?
+	if (lib.empty())
+	  continue;
+
+	found[lib].push_back(Name);
+
+	// Workaround: This getAddressOfGlobal call make first symbol search
+	// to work, immediatelly after library auto load. This approach do not
+	// use MU
+	//DLM->loadLibrary(lib, true);
+	//I.getAddressOfGlobal(*Name);
+      }
 
-        for (auto &&KV : found) {
-          auto MU = std::make_unique<AutoloadLibraryMU>(KV.first, std::move(KV.second));
-          if (auto Err = JD.define(MU))
-            return Err;
-        }
+      for (auto &&KV : found) {
+	auto MU = std::make_unique<AutoloadLibraryMU>(KV.first, std::move(KV.second));
+	if (auto Err = JD.define(MU))
+	  return Err;
       }
 
       return llvm::Error::success();
@@ -3437,16 +3436,17 @@ namespace Cpp {
     llvm::orc::JITDylib& DyLib = *EE.getProcessSymbolsJITDylib().get();
 #endif // CLANG_VERSION_MAJOR
 
-    if (!AutoSG)
-      AutoSG = &DyLib.addGenerator(std::make_unique<AutoLoadLibrarySearchGenerator>());
-    AutoSG->setEnabled(autoload);
+    if (!sAutoSG) {
+      sAutoSG = &DyLib.addGenerator(std::make_unique<AutoLoadLibrarySearchGenerator>());
+    }
+    sAutoSG->setEnabled(autoload);
 
-    LLVM_DEBUG(dbgs() << "Autoload=" << (AutoSG->isEnabled() ? "ON" : "OFF"));
+    LLVM_DEBUG(dbgs() << "Autoload=" << (sAutoSG->isEnabled() ? "ON" : "OFF"));
   }
 
   bool GetLibrariesAutoload() {
-    LLVM_DEBUG(dbgs() << "Autoload is " << (AutoSG && AutoSG->isEnabled() ? "ON" : "OFF"));
-    return AutoSG && AutoSG->isEnabled();
+    LLVM_DEBUG(dbgs() << "Autoload is " << (sAutoSG && sAutoSG->isEnabled() ? "ON" : "OFF"));
+    return sAutoSG && sAutoSG->isEnabled();
   }
 
 #undef DEBUG_TYPE

unittests/CppInterOp/CMakeLists.txt

@@ -30,13 +30,24 @@ target_link_libraries(DynamicLibraryManagerTests
   PRIVATE
   clangCppInterOp
   )
+set_source_files_properties(DynamicLibraryManagerTest.cpp PROPERTIES COMPILE_DEFINITIONS
+  "LLVM_BINARY_DIR=\"${LLVM_BINARY_DIR}\""
+  )
+set_source_files_properties(DynamicLibraryManagerTest.cpp PROPERTIES COMPILE_DEFINITIONS
+  "CPPINTEROP_VERSION=\"${CPPINTEROP_VERSION}\""
+  )
 
 set_output_directory(DynamicLibraryManagerTests BINARY_DIR ${CMAKE_BINARY_DIR}/unittests/bin/$<CONFIG>/)
 
 add_dependencies(DynamicLibraryManagerTests TestSharedLib)
 #export_executable_symbols_for_plugins(TestSharedLib)
 add_subdirectory(TestSharedLib)
-# TODO: Remove when libraryunload work correctly
 add_dependencies(DynamicLibraryManagerTests TestSharedLib1)
 #export_executable_symbols_for_plugins(TestSharedLib1)
 add_subdirectory(TestSharedLib1)
+add_dependencies(DynamicLibraryManagerTests TestSharedLib2)
+#export_executable_symbols_for_plugins(TestSharedLib2)
+add_subdirectory(TestSharedLib2)
+add_dependencies(DynamicLibraryManagerTests TestSharedLib3)
+#export_executable_symbols_for_plugins(TestSharedLib3)
+add_subdirectory(TestSharedLib3)

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

@@ -2,9 +2,12 @@
 
 #include "clang/Interpreter/CppInterOp.h"
 
+
+#include "llvm/ExecutionEngine/Orc/Core.h"
+#include "llvm/ExecutionEngine/Orc/DebugUtils.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
-
+#include "llvm/Support/raw_ostream.h"
 
 // Helper functions
 
@@ -13,13 +16,14 @@
 // 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) {
+std::string GetExecutablePath(const char* Argv0, void* mainAddr = nullptr) {
   // 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);
+  void* MainAddr = mainAddr ? mainAddr : (void*)intptr_t(GetExecutablePath);
   return llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
 }
 
+// Mangle symbol name
 static inline std::string MangleNameForDlsym(const std::string& name) {
   std::string nameForDlsym = name;
 #if defined(R__MACOSX) || defined(R__WIN32)
@@ -29,30 +33,33 @@ static inline std::string MangleNameForDlsym(const std::string& name) {
   return nameForDlsym;
 }
 
+#include "../../lib/Interpreter/CppInterOp.cpp"
+
+
 // Tests
 
 TEST(DynamicLibraryManagerTest, Sanity) {
   EXPECT_TRUE(Cpp::CreateInterpreter());
   EXPECT_FALSE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_zero").c_str()));
 
-  std::string BinaryPath = GetExecutablePath(/*Argv0=*/nullptr);
+  std::string BinaryPath = GetExecutablePath(nullptr, nullptr);
   llvm::StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
   Cpp::AddSearchPath(Dir.str().c_str());
 
+  // Find and load library with "ret_zero" symbol defined and exported
+  //
   // 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.
-
   std::string PathToTestSharedLib =
       Cpp::SearchLibrariesForSymbol(MangleNameForDlsym("ret_zero").c_str(), /*system_search=*/false);
-
   EXPECT_STRNE("", PathToTestSharedLib.c_str())
-      << "Cannot find: '" << PathToTestSharedLib << "' in '" << Dir.str()
-      << "'";
-
+      << "Cannot find: '" << PathToTestSharedLib << "' in '" << Dir.str() << "'";
   EXPECT_TRUE(Cpp::LoadLibrary(PathToTestSharedLib.c_str()));
+
   // Force ExecutionEngine to be created.
   Cpp::Process("");
+
   // FIXME: Conda returns false to run this code on osx.
   EXPECT_TRUE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_zero").c_str()));
 
@@ -70,19 +77,18 @@ TEST(DynamicLibraryManagerTest, LibrariesAutoload) {
   EXPECT_FALSE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_one").c_str()));
 
   // Libraries Search path by default is set to main executable directory.
-  std::string BinaryPath = GetExecutablePath(/*Argv0=*/nullptr);
+  std::string BinaryPath = GetExecutablePath(nullptr, nullptr);
   llvm::StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
   Cpp::AddSearchPath(Dir.str().c_str());
 
-  // Find library with "rec_one" symbol defined and exported
+  // Find library with "ret_one" symbol defined and exported
   //
   // 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. For now we use out minimal implementation of
   // helper function.
   std::string PathToTestSharedLib1 =
       Cpp::SearchLibrariesForSymbol(MangleNameForDlsym("ret_one").c_str(), /*system_search=*/false);
-
   // If result is "" then we cannot find this library.
   EXPECT_STRNE("", PathToTestSharedLib1.c_str())
       << "Cannot find: '" << PathToTestSharedLib1 << "' in '" << Dir.str() << "'";
@@ -117,13 +123,95 @@ TEST(DynamicLibraryManagerTest, LibrariesAutoload) {
   Cpp::SetLibrariesAutoload(false);
   // Check autorum status (must be turned OFF)
   EXPECT_FALSE(Cpp::GetLibrariesAutoload());
-  //EXPECT_FALSE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_one").c_str())); // if unload works
-  //EXPECT_TRUE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_one").c_str()));
+  EXPECT_TRUE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_one").c_str()));  // false if unload works
 
   // Autoload turn ON again
   Cpp::SetLibrariesAutoload(true);
   // Check autorum status (must be turned ON)
   EXPECT_TRUE(Cpp::GetLibrariesAutoload());
-  //EXPECT_FALSE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_one").c_str())); // if unload works
-  //EXPECT_TRUE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_one").c_str()));
+  EXPECT_TRUE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_one").c_str()));
+
+  // Find library with "ret_1" symbol defined and exported
+  std::string PathToTestSharedLib2 =
+      Cpp::SearchLibrariesForSymbol(MangleNameForDlsym("ret_1").c_str(), /*system_search=*/false);
+  // If result is "" then we cannot find this library.
+  EXPECT_STRNE("", PathToTestSharedLib2.c_str())
+      << "Cannot find: '" << PathToTestSharedLib2 << "' in '" << Dir.str() << "'";
+  // Find symbol must success (when auotload=on)
+  EXPECT_TRUE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_1").c_str()));
+  // Find symbol must success (when auotload=on)
+  EXPECT_TRUE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_2").c_str()));
+}
+
+TEST(DynamicLibraryManagerTest, LibrariesAutoloadExtraCoverage) {
+  EXPECT_TRUE(Cpp::CreateInterpreter());
+
+  // Libraries Search path by default is set to main executable directory.
+  std::string BinaryPath = GetExecutablePath(nullptr, nullptr);
+  llvm::StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
+  Cpp::AddSearchPath(Dir.str().c_str());
+
+  // Force ExecutionEngine to be created.
+  Cpp::Process("");
+
+  // Autoload turn ON
+  Cpp::SetLibrariesAutoload(true);
+  // Check autorum status (must be turned ON)
+  EXPECT_TRUE(Cpp::GetLibrariesAutoload());
+
+  // Cover: MU getName()
+  std::string res;
+  llvm::raw_string_ostream rss(res);
+  const llvm::orc::SymbolNameVector syms;
+  Cpp::AutoLoadLibrarySearchGenerator::AutoloadLibraryMU MU("test", syms);
+  rss << MU;
+  EXPECT_STRNE("", rss.str().c_str()) << "MU problem!";
+
+  // Cover: LoadLibrary error
+  // if (DLM->loadLibrary(lib, false) != DynamicLibraryManager::LoadLibResult::kLoadLibSuccess) {
+  //   LLVM_DEBUG(dbgs() << "MU: Failed to load library " << lib);
+  //   string err = "MU: Failed to load library! " + lib;
+  //   perror(err.c_str());
+  // } else {
+  // Find library with "ret_value" symbol defined and exported
+  std::string PathToTestSharedLib3 =
+      Cpp::SearchLibrariesForSymbol(MangleNameForDlsym("ret_val").c_str(), /*system_search=*/false);
+  // If result is "" then we cannot find this library.
+  EXPECT_STRNE("", PathToTestSharedLib3.c_str())
+      << "Cannot find: '" << PathToTestSharedLib3 << "' in '" << Dir.str() << "'";
+  // Remove library for simulate load error
+  llvm::sys::fs::remove(PathToTestSharedLib3, true);
+  EXPECT_TRUE(Cpp::GetLibrariesAutoload());
+  // FIXME: Conda returns false to run this code on osx.
+  EXPECT_FALSE(Cpp::GetFunctionAddress(MangleNameForDlsym("ret_val").c_str()));
+
+  // Cover
+  // } else {
+  //   // Collect all failing symbols, delegate their responsibility and then
+  //   // fail their materialization. R->defineNonExistent() sounds like it
+  //   // should do that, but it's not implemented?!
+  //   failedSymbols.insert(symbol);
+  // TODO: implement test this case
+
+  // Cover
+  // if (!failedSymbols.empty()) {
+  //   auto failingMR = R->delegate(failedSymbols);
+  //   if (failingMR) {
+  //     (*failingMR)->failMaterialization();
+  // TODO: implement test this case
+
+  // Cover
+  // void discard(const llvm::orc::JITDylib &JD, const llvm::orc::SymbolStringPtr &Name) override {}
+  // TODO: implement test this case
+
+  // Cover
+  // if (Path.empty()) {
+  //   LLVM_DEBUG(dbgs() << "DynamicLibraryManager::lookupLibMaybeAddExt(): "
+  // TODO: implement test this case
+
+  // Cover
+  // platform::DLClose(dyLibHandle, &errMsg);
+  // if (!errMsg.empty()) {
+  //   LLVM_DEBUG(dbgs() << "DynamicLibraryManager::unloadLibrary(): "
+  // TODO: implement test this case
 }