[Offload] Stop using global ctor/dtors in Offload unit tests

[callumfare]

Use of a static global to manage the lifetime of liboffload could lead to problems with teardown order. Change the Offload unit test environment to explicitly init and teardown liboffload in main instead.

• This requires opting out of using llvm_gtest_main, so we can't use add_unittest, but manually create the test executables instead
• We can't use GTest's ::testing::AddGlobalTestEnvironment() to do this as test discovery happens before global environment setup, and we depend on liboffload calls to discover devices for parameterized tests.

[llvmbot]

@llvm/pr-subscribers-offload

Author: Callum Fare (callumfare)

Changes

Use of a static global to manage the lifetime of liboffload could lead to problems with teardown order. Change the Offload unit test environment to explicitly init and teardown liboffload in main instead.

• This requires opting out of using llvm_gtest_main, so we can't use add_unittest, but manually create the test executables instead
• We can't use GTest's ::testing::AddGlobalTestEnvironment() to do this as test discovery happens before global environment setup, and we depend on liboffload calls to discover devices for parameterized tests.

---

Full diff: https://github.com/llvm/llvm-project/pull/149299.diff

3 Files Affected:

• (modified) offload/unittests/CMakeLists.txt (+15-4)
• (modified) offload/unittests/OffloadAPI/CMakeLists.txt (+1-1)
• (modified) offload/unittests/OffloadAPI/common/Environment.cpp (+22-10)

diff --git a/offload/unittests/CMakeLists.txt b/offload/unittests/CMakeLists.txt
index 388d15f834b1d..ada2706bb0320 100644
--- a/offload/unittests/CMakeLists.txt
+++ b/offload/unittests/CMakeLists.txt
@@ -85,13 +85,24 @@ function(add_offload_unittest test_dirname)
   set(target_name "${test_dirname}.unittests")
 
   list(TRANSFORM ARGN PREPEND "${CMAKE_CURRENT_SOURCE_DIR}/" OUTPUT_VARIABLE files)
+  list(APPEND files ${CMAKE_CURRENT_SOURCE_DIR}/common/Environment.cpp)
 
-  add_unittest(OffloadUnitTests "${target_name}"
-    ${CMAKE_CURRENT_SOURCE_DIR}/common/Environment.cpp
-    ${files})
+  if( NOT LLVM_BUILD_TESTS )
+    set(EXCLUDE_FROM_ALL ON)
+  endif()
+  add_llvm_executable(${target_name} IGNORE_EXTERNALIZE_DEBUGINFO NO_INSTALL_RPATH ${files})
+  get_subproject_title(subproject_title)
+  set_target_properties(${target_name} PROPERTIES FOLDER "${subproject_title}/Tests/Unit")
+
+  target_link_options(${target_name} PRIVATE "${LLVM_UNITTEST_LINK_FLAGS}")
+
+  set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
+  set_output_directory(${target_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
+  target_link_libraries(${target_name} PRIVATE llvm_gtest ${LLVM_PTHREAD_LIB} ${PLUGINS_TEST_COMMON})
+
+  add_dependencies(OffloadUnitTests ${target_name})
   add_dependencies(${target_name} ${PLUGINS_TEST_COMMON} offload_device_binaries)
   target_compile_definitions(${target_name} PRIVATE DEVICE_CODE_PATH="${OFFLOAD_TEST_DEVICE_CODE_PATH}")
-  target_link_libraries(${target_name} PRIVATE ${PLUGINS_TEST_COMMON})
   target_include_directories(${target_name} PRIVATE ${PLUGINS_TEST_INCLUDE})
 endfunction()
 
diff --git a/offload/unittests/OffloadAPI/CMakeLists.txt b/offload/unittests/OffloadAPI/CMakeLists.txt
index d76338612210d..a393bf5b39bb6 100644
--- a/offload/unittests/OffloadAPI/CMakeLists.txt
+++ b/offload/unittests/OffloadAPI/CMakeLists.txt
@@ -16,7 +16,7 @@ add_offload_unittest("event"
 
 add_offload_unittest("init"
     init/olInit.cpp)
-target_compile_definitions("init.unittests" PRIVATE DISABLE_WRAPPER)
+target_compile_definitions("init.unittests" PRIVATE DISABLE_OL_INIT)
 
 add_offload_unittest("kernel"
     kernel/olLaunchKernel.cpp)
diff --git a/offload/unittests/OffloadAPI/common/Environment.cpp b/offload/unittests/OffloadAPI/common/Environment.cpp
index ef092cd4187d3..d04add55ac54d 100644
--- a/offload/unittests/OffloadAPI/common/Environment.cpp
+++ b/offload/unittests/OffloadAPI/common/Environment.cpp
@@ -11,20 +11,11 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <OffloadAPI.h>
+#include <OffloadPrint.hpp>
 #include <fstream>
 
 using namespace llvm;
 
-// Wrapper so we don't have to constantly init and shutdown Offload in every
-// test, while having sensible lifetime for the platform environment
-#ifndef DISABLE_WRAPPER
-struct OffloadInitWrapper {
-  OffloadInitWrapper() { olInit(); }
-  ~OffloadInitWrapper() { olShutDown(); }
-};
-static OffloadInitWrapper Wrapper{};
-#endif
-
 static cl::opt<std::string>
     SelectedPlatform("platform", cl::desc("Only test the specified platform"),
                      cl::value_desc("platform"));
@@ -193,3 +184,24 @@ bool TestEnvironment::loadDeviceBinary(
   BinaryOut = std::move(SourceFile.get());
   return true;
 }
+
+int main(int argc, char **argv) {
+#ifndef DISABLE_OL_INIT
+  if (auto Err = olInit()) {
+    errs() << "Failed to initialize liboffload: " << Err->Code << "\n";
+    return -1;
+  }
+#endif
+
+  ::testing::InitGoogleTest(&argc, argv);
+  int Result = RUN_ALL_TESTS();
+
+#ifndef DISABLE_OL_INIT
+  if (auto Err = olShutDown()) {
+    errs() << "Failed to shut down liboffload: " << Err->Code << "\n";
+    return -1;
+  }
+#endif
+
+  return Result;
+}

[jhuber6]

What teardown issues are you seeing? I thought I fixed most of these.

[callumfare]

I didn't fully understand why the fix in #148642 worked, because the plugins were showing up as uninitialized before deinit() was ever called on them. My assumption is that's because plugin objects were being clobbered during teardown, but I don't actually know if that's the case. This seemed like a sensible change either way.

[jhuber6]

I forget if the proper reference counting patch landed for init / deinit. The important thing however is that deinit is done via atexit rather than a global destructor (Which C++ does). So, I'm unsure why it fails here if we have reference counting. The executable's list should be handled after all the dynamically loaded libraries.

[callumfare]

We do have reference counting now. This might not be strictly necessary then, I'll have a closer look at the original problem and see what's actually happening