[clang] Register all LLVM targets in AllClangUnitTest main

[rnk]

Addresses feedback in #134196 (comment)

Makes the tests less sensitive to target registration from unrelated test fixtures by registering everything up front.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Reid Kleckner (rnk)

Changes

Addresses feedback in #134196 (comment)

Makes the tests less sensitive to target registration from unrelated test fixtures by registering everything up front.

---

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

2 Files Affected:

• (added) clang/unittests/AllClangUnitTests.cpp (+24)
• (modified) clang/unittests/CMakeLists.txt (+1)

diff --git a/clang/unittests/AllClangUnitTests.cpp b/clang/unittests/AllClangUnitTests.cpp
new file mode 100644
index 0000000000000..1726cabc8107c
--- /dev/null
+++ b/clang/unittests/AllClangUnitTests.cpp
@@ -0,0 +1,24 @@
+//===- clang/unittests/AllClangUnitTests.cpp ------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/TargetSelect.h"
+
+// This custom main entry point for the AllClangUnitTests binary registers all
+// tests on startup, so the tests don't become sensitive to target registration
+// within the test suite.
+int main(int argc, char **argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  llvm::cl::ParseCommandLineOptions(argc, argv);
+
+  llvm::InitializeAllTargets();
+  llvm::InitializeAllTargetMCs();
+
+  return RUN_ALL_TESTS();
+}
diff --git a/clang/unittests/CMakeLists.txt b/clang/unittests/CMakeLists.txt
index aef28f914b640..54c781a35c20c 100644
--- a/clang/unittests/CMakeLists.txt
+++ b/clang/unittests/CMakeLists.txt
@@ -117,6 +117,7 @@ get_property(LINK_LIBS GLOBAL PROPERTY CLANG_UNITTEST_LINK_LIBS)
 get_property(LLVM_COMPONENTS GLOBAL PROPERTY CLANG_UNITTEST_LLVM_COMPONENTS)
 add_distinct_clang_unittest(AllClangUnitTests
   ${SRCS}
+  AllClangUnitTests.cpp
   CLANG_LIBS
   ${CLANG_LIBS}
   LINK_LIBS

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[kadircet]

i think we're still linking against llvm_gtest_main, https://github.com/llvm/llvm-project/blob/main/third-party/unittest/UnitTestMain/TestMain.cpp. we probably need to do some fiddling in the build rules as well to make sure, we only link against llvm_gtest. (not sure if you want to have other msvc related magic in here as well)

[rnk]

I spot checked two other unit tests that have custom mains, and they don't do anything special:

1. llvm-cfi-verify main , cmake
2. LiveIntervalTest main, cmake

I think archive semantics are pretty portable between platforms, so I think this just works. The sources used to build the unit test executable override the main provided by the gtest library.

[kadircet]

i don't feel great about relying on linking order and linker not loading llvm_gtest_main solely because it wasn't referenced, but I guess that's a theoretical concern that won't matter much in practice (since llvm_gtest_main doesn't really define symbols apart from TestMainArgv0, at least yet). so leaving this up to you.

but can we at least leave a comment here, mentioning that we're also linking against llvm_gtest_main to make it easier for people to investigate if they encounter any such issues?

[rnk]

Yeah, on second thought, there's a lot more going on in UnitTestMain than I thought. I rewrote this to use a dynamic initializer to register the targets instead. Those will reliably run before test fixtures. The only risk here is that we encounter intialization-order-fiasco issues, but I spot-checked x86, and it seems to be resilient to that... I will test with ASan locally to build confidence, but PTAL at the new approach.

[kadircet]

thanks a lot, this is really neat! and yeah i think initialization-order-fiasco should be less of a concern here.

[kadircet]

thanks for the quick fix!

[kadircet]

i don't feel great about relying on linking order and linker not loading llvm_gtest_main solely because it wasn't referenced, but I guess that's a theoretical concern that won't matter much in practice (since llvm_gtest_main doesn't really define symbols apart from TestMainArgv0, at least yet). so leaving this up to you.

but can we at least leave a comment here, mentioning that we're also linking against llvm_gtest_main to make it easier for people to investigate if they encounter any such issues?

[kadircet]

thanks a lot, this is really neat! and yeah i think initialization-order-fiasco should be less of a concern here.