Add tests and dev-doc

Add test to run things in parallel
Add documentation for developers

Author: Vipul-Cariappa

docs/DevelopersDocumentation.rst

@@ -423,12 +423,54 @@ library files and run pytest:
    python -m pip install pytest
    python -m pytest -sv
 
-***********************************
+###################################
  CppInterOp Internal Documentation
-***********************************
+###################################
 
 CppInterOp maintains an internal Doxygen documentation of its components.
 Internal documentation aims to capture intrinsic details and overall usage of
 code components. The goal of internal documentation is to make the codebase
 easier to understand for the new developers. Internal documentation can be
 visited : `here <build/html/index.html>`_
+
+**************************************
+ Multiple Interpreter & Thread-Safety
+**************************************
+
+CppInterOp allows the user to create multiple interpreters at a time and
+use those interpreters. The interpreters that are created are stored in a
+stack and a map. The stack is used to enable the model where the user
+wants to create a temporary interpreter and destroy it after performing a
+few operations. In such a use case, the top of the stack is the only
+interpreter in use at any given point in time.
+
+The map is used to store the mapping from :code:`clang::ASTContext` to
+:code:`Cpp::InterpreterInfo`. This is required to figure out which
+interpreter an object belongs to. Say the library user performs the
+following operations:
+
+1. Create an Interpreter
+2. Compile some code with variable :code:`a`
+3. Create another Interpreter
+4. Performs :code:`Cpp::GetVariableOffset(a)`
+
+In step 4, the top of the stack is an interpreter without the definition of
+:code:`a`. And we cannot use it to figure out the address of :code:`a`.
+The :code:`clang::Decl` passed to :code:`Cpp::GetVariableOffset` is used to
+retrieve the :code:`clang::ASTContext`, using
+:code:`clang::Decl::getASTContext`. We then use the map to figure out the
+exact Interpreter Instance this :code:`clang::Decl` belongs to and perform
+the operation.
+
+A shortcoming of this is that if the CppInterOp accepts a
+:code:`clang::QualType` instead of :code:`clang::Decl`, then it is not
+possible to get the :code:`clang::ASTContext` from the :code:`clang::QualType`.
+In such cases, we iterate over the Allocator of all the Interpreters in our
+stack and figure out which :code:`clang::ASTContext` allocated this
+:code:`clang::QualType`. This is a very expensive operation. But there is no
+alternative to this.
+
+For **thread-safety**, we introduce a lock for each of the interpreters we
+create. And lock only that one specific interpreter when required. We also
+have 2 global locks, one for LLVM, and another is used to lock operations
+performed on the interpreter stack and the map itself.

lib/CppInterOp/CppInterOp.cpp

@@ -3459,6 +3459,8 @@ static inline auto find_interpreter_in_map(InterpreterInfo* I) {
 bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {
   std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
   assert(sInterpreters->size() == sInterpreterASTMap->size());
+  if (sInterpreters->empty())
+    return false;
 
   if (!I) {
     auto foundAST = find_interpreter_in_map(sInterpreters->back().get());

unittests/CppInterOp/CMakeLists.txt

@@ -128,6 +128,8 @@ set_output_directory(DynamicLibraryManagerTests
 )
 
 add_dependencies(DynamicLibraryManagerTests TestSharedLib)
+add_dependencies(DynamicLibraryManagerTests TestSharedLib2)
 
 #export_executable_symbols_for_plugins(TestSharedLib)
 add_subdirectory(TestSharedLib)
+add_subdirectory(TestSharedLib2)

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

@@ -19,7 +19,7 @@ std::string GetExecutablePath(const char* Argv0) {
   return llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
 }
 
-TEST(DynamicLibraryManagerTest, Sanity) {
+TEST(DynamicLibraryManagerTest, Sanity1) {
 #ifdef EMSCRIPTEN
   GTEST_SKIP() << "Test fails for Emscipten builds";
 #endif
@@ -66,6 +66,55 @@ TEST(DynamicLibraryManagerTest, Sanity) {
   // EXPECT_FALSE(Cpp::GetFunctionAddress("ret_zero"));
 }
 
+TEST(DynamicLibraryManagerTest, Sanity2) {
+#ifdef EMSCRIPTEN
+  GTEST_SKIP() << "Test fails for Emscipten builds";
+#endif
+
+#if CLANG_VERSION_MAJOR == 18 && defined(CPPINTEROP_USE_CLING) &&              \
+    defined(_WIN32)
+  GTEST_SKIP() << "Test fails with Cling on Windows";
+#endif
+
+  auto* I = Cpp::CreateInterpreter();
+  EXPECT_TRUE(I);
+  EXPECT_FALSE(Cpp::GetFunctionAddress("ret_one", I));
+
+  std::string BinaryPath = GetExecutablePath(/*Argv0=*/nullptr);
+  llvm::StringRef Dir = llvm::sys::path::parent_path(BinaryPath);
+  Cpp::AddSearchPath(Dir.str().c_str(), true, false, I);
+
+  // 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_one", /*system_search=*/false, I);
+#else
+  std::string PathToTestSharedLib =
+      Cpp::SearchLibrariesForSymbol("ret_one", /*system_search=*/false, I);
+#endif // __APPLE__
+
+  EXPECT_STRNE("", PathToTestSharedLib.c_str())
+      << "Cannot find: '" << PathToTestSharedLib << "' in '" << Dir.str()
+      << "'";
+
+  EXPECT_TRUE(Cpp::LoadLibrary(PathToTestSharedLib.c_str(), true, I));
+  // Force ExecutionEngine to be created.
+  Cpp::Process("", I);
+  Cpp::Declare("", I);
+  // FIXME: Conda returns false to run this code on osx.
+#ifndef __APPLE__
+  EXPECT_TRUE(Cpp::GetFunctionAddress("ret_one", I));
+#endif //__APPLE__
+
+  Cpp::UnloadLibrary("TestSharedLib2", I);
+  // 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"));
+}
+
 TEST(DynamicLibraryManagerTest, BasicSymbolLookup) {
 #ifndef EMSCRIPTEN
   GTEST_SKIP() << "This test is only intended for Emscripten builds.";

unittests/CppInterOp/FunctionReflectionTest.cpp

@@ -1014,6 +1014,8 @@ TEST(FunctionReflectionTest, BestOverloadFunctionMatch1) {
   GetAllTopLevelDecls(code, Decls);
   std::vector<Cpp::TCppFunction_t> candidates;
 
+  EXPECT_FALSE(Cpp::BestOverloadFunctionMatch({}, {}, {}));
+
   for (auto decl : Decls)
     if (Cpp::IsTemplatedFunction(decl)) candidates.push_back((Cpp::TCppFunction_t)decl);
 

unittests/CppInterOp/InterpreterTest.cpp

@@ -17,13 +17,16 @@
 #include "clang-c/CXCppInterOp.h"
 
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
-#include <llvm/Support/FileSystem.h>
+#include "llvm/Support/raw_ostream.h"
 
 #include <gmock/gmock.h>
 #include "gtest/gtest.h"
 
 #include <algorithm>
+#include <cstdint>
+#include <thread>
 #include <utility>
 
 using ::testing::StartsWith;
@@ -247,7 +250,7 @@ TEST(InterpreterTest, DISABLED_DetectResourceDir) {
 #ifdef _WIN32
   GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
 #endif
-  Cpp::CreateInterpreter();
+  auto* I = Cpp::CreateInterpreter();
   EXPECT_STRNE(Cpp::DetectResourceDir().c_str(), Cpp::GetResourceDir());
   llvm::SmallString<256> Clang(LLVM_BINARY_DIR);
   llvm::sys::path::append(Clang, "bin", "clang");
@@ -256,7 +259,7 @@ TEST(InterpreterTest, DISABLED_DetectResourceDir) {
     GTEST_SKIP() << "Test not run (Clang binary does not exist)";
 
   std::string DetectedPath = Cpp::DetectResourceDir(Clang.str().str().c_str());
-  EXPECT_STREQ(DetectedPath.c_str(), Cpp::GetResourceDir());
+  EXPECT_STREQ(DetectedPath.c_str(), Cpp::GetResourceDir(I));
 }
 
 TEST(InterpreterTest, DetectSystemCompilerIncludePaths) {
@@ -364,26 +367,83 @@ if (llvm::sys::RunningOnValgrind())
 #endif
 }
 
+static int printf_jit(const char* format, ...) {
+  llvm::errs() << "printf_jit called!\n";
+  return 0;
+}
+
 TEST(InterpreterTest, MultipleInterpreter) {
-#if CLANG_VERSION_MAJOR < 20 && defined(EMSCRIPTEN)
-  GTEST_SKIP() << "Test fails for Emscipten LLVM 20 builds";
+#ifdef EMSCRIPTEN
+  GTEST_SKIP() << "Test fails for Emscipten builds";
 #endif
-  auto* I = Cpp::CreateInterpreter();
-  EXPECT_TRUE(I);
-  Cpp::Declare(R"(
-  void f() {}
-  )");
-  Cpp::TCppScope_t f = Cpp::GetNamed("f");
+#ifdef _WIN32
+  GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
+#endif
+  // delete all old interpreters
+  while (Cpp::DeleteInterpreter())
+    ;
+  std::vector<const char*> interpreter_args = {"-include", "new"};
+  auto* I1 = Cpp::CreateInterpreter(interpreter_args);
+  EXPECT_TRUE(I1);
+
+  auto F = [](Cpp::TInterp_t I) {
+    bool hasError = true;
+    EXPECT_TRUE(Cpp::Evaluate("__cplusplus", &hasError, I) == 201402);
+    EXPECT_FALSE(hasError);
+
+    Cpp::Declare(R"(
+    #include <new>
+    extern "C" int printf(const char*,...);
+    class MyKlass {};
+    )",
+                 false, I);
+    Cpp::TCppScope_t f = Cpp::GetNamed("printf", Cpp::GetGlobalScope(I));
+    EXPECT_TRUE(f);
+    EXPECT_TRUE(Cpp::GetComplexType(Cpp::GetType("int", I)));
+    Cpp::TCppType_t MyKlass = Cpp::GetType("MyKlass", I);
+    EXPECT_EQ(Cpp::GetTypeAsString(MyKlass), "MyKlass");
+    EXPECT_EQ(Cpp::GetNumBases(MyKlass), 0);
+    EXPECT_FALSE(Cpp::GetBaseClass(MyKlass, 3));
+    std::vector<Cpp::TCppScope_t> members;
+    Cpp::GetEnumConstantDatamembers(Cpp::GetScopeFromType(MyKlass), members);
+    EXPECT_EQ(members.size(), 0);
+
+    EXPECT_FALSE(
+        Cpp::InsertOrReplaceJitSymbol("printf", (uint64_t)&printf_jit, I));
+
+    auto f_callable = Cpp::MakeFunctionCallable(f);
+    EXPECT_EQ(f_callable.getKind(), Cpp::JitCall::Kind::kGenericCall);
+
+    EXPECT_FALSE(
+        Cpp::TakeInterpreter((TInterp_t)1)); // try to take ownership of an
+                                             // interpreter that does not exist
+
+    std::vector<std::string> includes;
+    Cpp::AddIncludePath("/non/existent/", I);
+    Cpp::GetIncludePaths(includes, false, false, I);
+    EXPECT_NE(std::find(includes.begin(), includes.end(), "/non/existent/"),
+              std::end(includes));
+
+    EXPECT_TRUE(Cpp::InsertOrReplaceJitSymbol("non_existent", (uint64_t)&f, I));
+  };
+  F(I1);
 
-  auto* I2 = Cpp::CreateInterpreter();
+  auto* I2 = Cpp::CreateInterpreter(interpreter_args);
+  auto* I3 = Cpp::CreateInterpreter(interpreter_args);
+  auto* I4 = Cpp::CreateInterpreter(interpreter_args);
   EXPECT_TRUE(I2);
-  Cpp::Declare(R"(
-  void ff() {}
-  )");
-  Cpp::TCppScope_t ff = Cpp::GetNamed("ff");
-
-  auto f_callable = Cpp::MakeFunctionCallable(f);
-  EXPECT_EQ(f_callable.getKind(), Cpp::JitCall::Kind::kGenericCall);
-  auto ff_callable = Cpp::MakeFunctionCallable(ff);
-  EXPECT_EQ(ff_callable.getKind(), Cpp::JitCall::Kind::kGenericCall);
+  EXPECT_TRUE(I3);
+  EXPECT_TRUE(I4);
+
+  std::thread t2(F, I2);
+  std::thread t3(F, I3);
+  std::thread t4(F, I4);
+  t2.join();
+  t3.join();
+  t4.join();
+
+  testing::internal::CaptureStderr();
+  Cpp::Process("printf(\"Blah\");", I2);
+  std::string cerrs = testing::internal::GetCapturedStderr();
+  EXPECT_STREQ(cerrs.c_str(), "printf_jit called!\n");
 }

unittests/CppInterOp/TestSharedLib2/CMakeLists.txt

@@ -0,0 +1,23 @@
+add_llvm_library(TestSharedLib2
+  SHARED
+  DISABLE_LLVM_LINK_LLVM_DYLIB
+  BUILDTREE_ONLY
+  TestSharedLib.cpp)
+# Put TestSharedLib2 next to the unit test executable.
+set_output_directory(TestSharedLib2
+  BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/../TestSharedLib/unittests/bin/$<CONFIG>/
+  LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/../TestSharedLib/unittests/bin/$<CONFIG>/
+  )
+
+
+if (EMSCRIPTEN)
+  set_target_properties(TestSharedLib2
+    PROPERTIES NO_SONAME 1
+  )
+  target_link_options(TestSharedLib2
+    PRIVATE "SHELL: -s WASM_BIGINT"
+    PRIVATE "SHELL: -s SIDE_MODULE=1"
+  )
+endif()
+
+set_target_properties(TestSharedLib2 PROPERTIES FOLDER "Tests")

unittests/CppInterOp/TestSharedLib2/TestSharedLib.cpp

@@ -0,0 +1,3 @@
+#include "TestSharedLib.h"
+
+int ret_one() { return 1; }

unittests/CppInterOp/TestSharedLib2/TestSharedLib.h

@@ -0,0 +1,11 @@
+#ifndef UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H
+#define UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H
+
+// Avoid having to mangle/demangle the symbol name in tests
+#ifdef _WIN32
+extern "C" __declspec(dllexport) int ret_one();
+#else
+extern "C" int __attribute__((visibility("default"))) ret_one();
+#endif
+
+#endif // UNITTESTS_CPPINTEROP_TESTSHAREDLIB_TESTSHAREDLIB2_H

unittests/CppInterOp/VariableReflectionTest.cpp

@@ -287,6 +287,8 @@ TEST(VariableReflectionTest, GetVariableOffset) {
   std::vector<Cpp::TCppScope_t> datamembers;
   Cpp::GetDatamembers(Decls[4], datamembers);
 
+  EXPECT_FALSE((bool)Cpp::GetVariableOffset(nullptr));
+
   EXPECT_TRUE((bool) Cpp::GetVariableOffset(Decls[0])); // a
   EXPECT_TRUE((bool) Cpp::GetVariableOffset(Decls[1])); // N
   EXPECT_TRUE((bool)Cpp::GetVariableOffset(Decls[2]));  // S