From 72cb9d0d06654b14e88d5d33ca53c7e88a3ca95c Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Wed, 14 Aug 2024 19:50:08 +0200 Subject: [PATCH] Refactor testing exe path and benchmark main handling. (#4216) Consolidates both main libraries into `//testing/base`, and factors out the exe path handling for benchmarks and unit tests into a common library to remove duplication. Refactors how that logic is managed to be cleaner and avoid a confusing bool that came up in code review. Updates all the tests and benchmarks that use these. I still need to update other benchmarks to use the same main, but I wanted to keep this PR somewhat minimal. This also fixes a bug noticed in passing that the compilation benchmark didn't have the required dependency on the benchmark library itself, just the benchmark main library. --- common/BUILD | 16 +------ testing/base/BUILD | 48 +++++++++++++++---- {common => testing/base}/benchmark_main.cpp | 27 +---------- testing/base/global_exe_path.cpp | 27 +++++++++++ .../base/{gtest_main.h => global_exe_path.h} | 14 ++++-- ...main_test.cpp => global_exe_path_test.cpp} | 4 +- testing/base/gtest_main.cpp | 27 ++--------- testing/base/source_gen_test.cpp | 4 +- toolchain/driver/BUILD | 6 ++- toolchain/driver/clang_runner_test.cpp | 6 +-- toolchain/driver/compile_benchmark.cpp | 5 +- toolchain/driver/driver_test.cpp | 4 +- toolchain/install/BUILD | 1 + toolchain/install/install_paths_test.cpp | 7 ++- toolchain/lex/BUILD | 6 +-- toolchain/sem_ir/BUILD | 1 + toolchain/sem_ir/yaml_test.cpp | 4 +- 17 files changed, 109 insertions(+), 98 deletions(-) rename {common => testing/base}/benchmark_main.cpp (63%) create mode 100644 testing/base/global_exe_path.cpp rename testing/base/{gtest_main.h => global_exe_path.h} (52%) rename testing/base/{gtest_main_test.cpp => global_exe_path_test.cpp} (86%) diff --git a/common/BUILD b/common/BUILD index 0ff30c9f51b1..cb2aa2dfcb83 100644 --- a/common/BUILD +++ b/common/BUILD @@ -35,20 +35,6 @@ cc_library( ], ) -cc_library( - name = "benchmark_main", - srcs = ["benchmark_main.cpp"], - hdrs = ["benchmark_main.h"], - deps = [ - ":check", - ":exe_path", - ":init_llvm", - "@abseil-cpp//absl/flags:parse", - "@google_benchmark//:benchmark", - "@llvm-project//llvm:Support", - ], -) - cc_library( name = "command_line", srcs = ["command_line.cpp"], @@ -196,9 +182,9 @@ cc_binary( testonly = 1, srcs = ["hashing_benchmark.cpp"], deps = [ - ":benchmark_main", ":check", ":hashing", + "//testing/base:benchmark_main", "@abseil-cpp//absl/hash", "@abseil-cpp//absl/random", "@google_benchmark//:benchmark", diff --git a/testing/base/BUILD b/testing/base/BUILD index 4ed85d7881e4..69845c012043 100644 --- a/testing/base/BUILD +++ b/testing/base/BUILD @@ -19,23 +19,28 @@ cc_library( name = "gtest_main", testonly = 1, srcs = ["gtest_main.cpp"], - hdrs = ["gtest_main.h"], deps = [ - "//common:check", - "//common:exe_path", + ":global_exe_path", "//common:init_llvm", "@googletest//:gtest", "@llvm-project//llvm:Support", ], ) -cc_test( - name = "gtest_main_test", - size = "small", - srcs = ["gtest_main_test.cpp"], +# This does extra initialization on top of Google benchmark's main in order to +# provide stack traces and setup LLVM. +# +# This replaces `@google_benchmark//:benchmark_main`; +# `@google_benchmark//:benchmark` should still be used directly. +cc_library( + name = "benchmark_main", + testonly = 1, + srcs = ["benchmark_main.cpp"], deps = [ - ":gtest_main", - "@googletest//:gtest", + ":global_exe_path", + "//common:init_llvm", + "@abseil-cpp//absl/flags:parse", + "@google_benchmark//:benchmark", "@llvm-project//llvm:Support", ], ) @@ -60,6 +65,7 @@ cc_test( size = "small", srcs = ["source_gen_test.cpp"], deps = [ + ":global_exe_path", ":gtest_main", ":source_gen_lib", "//common:set", @@ -104,3 +110,27 @@ cc_test( "@googletest//:gtest", ], ) + +cc_library( + name = "global_exe_path", + testonly = 1, + srcs = ["global_exe_path.cpp"], + hdrs = ["global_exe_path.h"], + deps = [ + "//common:check", + "//common:exe_path", + "@llvm-project//llvm:Support", + ], +) + +cc_test( + name = "global_exe_path_test", + size = "small", + srcs = ["global_exe_path_test.cpp"], + deps = [ + ":global_exe_path", + ":gtest_main", + "@googletest//:gtest", + "@llvm-project//llvm:Support", + ], +) diff --git a/common/benchmark_main.cpp b/testing/base/benchmark_main.cpp similarity index 63% rename from common/benchmark_main.cpp rename to testing/base/benchmark_main.cpp index 8d7bc02df52c..195f3a2d3108 100644 --- a/common/benchmark_main.cpp +++ b/testing/base/benchmark_main.cpp @@ -2,40 +2,17 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -#include "common/benchmark_main.h" - #include -#include - #include "absl/flags/parse.h" -#include "common/check.h" -#include "common/exe_path.h" #include "common/init_llvm.h" -#include "llvm/ADT/ArrayRef.h" -#include "llvm/ADT/StringRef.h" +#include "testing/base/global_exe_path.h" -static bool after_main = false; -static llvm::StringRef exe_path; - -namespace Carbon::Testing { - -auto GetBenchmarkExePath() -> llvm::StringRef { - CARBON_CHECK(after_main) - << "Must not query the executable path until after `main` is entered!"; - return exe_path; -} - -} // namespace Carbon::Testing - -// TODO: Refactor this to share code with `gtest_main.cpp`. auto main(int orig_argc, char** orig_argv) -> int { // Do LLVM's initialization first, this will also transform UTF-16 to UTF-8. Carbon::InitLLVM init_llvm(orig_argc, orig_argv); - std::string exe_path_storage = Carbon::FindExecutablePath(orig_argv[0]); - exe_path = exe_path_storage; - after_main = true; + Carbon::Testing::SetExePath(orig_argv[0]); // Inject a flag to override the defaults for benchmarks. This can still be // disabled by user arguments. diff --git a/testing/base/global_exe_path.cpp b/testing/base/global_exe_path.cpp new file mode 100644 index 000000000000..ebeb0d99c5c2 --- /dev/null +++ b/testing/base/global_exe_path.cpp @@ -0,0 +1,27 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "testing/base/global_exe_path.h" + +#include + +#include "common/check.h" +#include "common/exe_path.h" + +static constinit std::optional exe_path = {}; + +namespace Carbon::Testing { + +auto GetExePath() -> llvm::StringRef { + CARBON_CHECK(exe_path) + << "Must not query the executable path until after it has been set!"; + return *exe_path; +} + +auto SetExePath(const char* argv_zero) -> void { + CARBON_CHECK(!exe_path) << "Must not call `SetExePath` more than once!"; + exe_path.emplace(Carbon::FindExecutablePath(argv_zero)); +} + +} // namespace Carbon::Testing diff --git a/testing/base/gtest_main.h b/testing/base/global_exe_path.h similarity index 52% rename from testing/base/gtest_main.h rename to testing/base/global_exe_path.h index 5aa3b72b6f68..f32c7d78ffe7 100644 --- a/testing/base/gtest_main.h +++ b/testing/base/global_exe_path.h @@ -2,8 +2,8 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -#ifndef CARBON_TESTING_BASE_GTEST_MAIN_H_ -#define CARBON_TESTING_BASE_GTEST_MAIN_H_ +#ifndef CARBON_TESTING_BASE_GLOBAL_EXE_PATH_H_ +#define CARBON_TESTING_BASE_GLOBAL_EXE_PATH_H_ #include "llvm/ADT/StringRef.h" @@ -13,8 +13,14 @@ namespace Carbon::Testing { // The executable path of the test binary. -auto GetTestExePath() -> llvm::StringRef; +auto GetExePath() -> llvm::StringRef; + +// Sets the executable path of a test binary from its `argv[0]`. +// +// This function must only be called once for an execution, and before any +// callers to `GetExePath`. Typically, it is called from within `main`. +auto SetExePath(const char* argv_zero) -> void; } // namespace Carbon::Testing -#endif // CARBON_TESTING_BASE_GTEST_MAIN_H_ +#endif // CARBON_TESTING_BASE_GLOBAL_EXE_PATH_H_ diff --git a/testing/base/gtest_main_test.cpp b/testing/base/global_exe_path_test.cpp similarity index 86% rename from testing/base/gtest_main_test.cpp rename to testing/base/global_exe_path_test.cpp index 0a72b6706aa8..f54701e79e97 100644 --- a/testing/base/gtest_main_test.cpp +++ b/testing/base/global_exe_path_test.cpp @@ -2,7 +2,7 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -#include "testing/base/gtest_main.h" +#include "testing/base/global_exe_path.h" #include #include @@ -15,7 +15,7 @@ namespace { using ::testing::StrNe; TEST(TestExePathTest, Test) { - llvm::StringRef exe_path = GetTestExePath(); + llvm::StringRef exe_path = GetExePath(); EXPECT_THAT(exe_path, StrNe("")); EXPECT_TRUE(llvm::sys::fs::exists(exe_path)); EXPECT_TRUE(llvm::sys::fs::can_execute(exe_path)); diff --git a/testing/base/gtest_main.cpp b/testing/base/gtest_main.cpp index e56b62976002..f0881e813525 100644 --- a/testing/base/gtest_main.cpp +++ b/testing/base/gtest_main.cpp @@ -2,35 +2,16 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -#include "testing/base/gtest_main.h" - #include -#include - -#include "common/check.h" -#include "common/exe_path.h" #include "common/init_llvm.h" - -static bool after_main = false; -static llvm::StringRef exe_path; - -namespace Carbon::Testing { - -auto GetTestExePath() -> llvm::StringRef { - CARBON_CHECK(after_main) - << "Must not query the executable path until after `main` is entered!"; - return exe_path; -} - -} // namespace Carbon::Testing +#include "testing/base/global_exe_path.h" auto main(int argc, char** argv) -> int { - std::string exe_path_storage = Carbon::FindExecutablePath(argv[0]); - exe_path = exe_path_storage; - after_main = true; - + // Initialize LLVM first, as that will also handle ensuring UTF-8 encoding. Carbon::InitLLVM init_llvm(argc, argv); + + Carbon::Testing::SetExePath(argv[0]); testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); } diff --git a/testing/base/source_gen_test.cpp b/testing/base/source_gen_test.cpp index 9686bd7be14f..284bf24219c9 100644 --- a/testing/base/source_gen_test.cpp +++ b/testing/base/source_gen_test.cpp @@ -8,7 +8,7 @@ #include #include "common/set.h" -#include "testing/base/gtest_main.h" +#include "testing/base/global_exe_path.h" #include "toolchain/driver/driver.h" namespace Carbon::Testing { @@ -144,7 +144,7 @@ TEST(SourceGenTest, UniqueIdentifiers) { auto TestCompile(llvm::StringRef source) -> bool { llvm::vfs::InMemoryFileSystem fs; InstallPaths installation( - InstallPaths::MakeForBazelRunfiles(Testing::GetTestExePath())); + InstallPaths::MakeForBazelRunfiles(Testing::GetExePath())); Driver driver(fs, &installation, llvm::outs(), llvm::errs()); // Load the prelude into our VFS. diff --git a/toolchain/driver/BUILD b/toolchain/driver/BUILD index ab85bd70a2c1..c7d600e066eb 100644 --- a/toolchain/driver/BUILD +++ b/toolchain/driver/BUILD @@ -43,6 +43,7 @@ cc_test( "//common:all_llvm_targets", "//common:check", "//common:ostream", + "//testing/base:global_exe_path", "//testing/base:gtest_main", "//testing/base:test_raw_ostream", "@googletest//:gtest", @@ -58,8 +59,10 @@ cc_binary( srcs = ["compile_benchmark.cpp"], deps = [ ":driver", - "//common:benchmark_main", + "//testing/base:benchmark_main", + "//testing/base:global_exe_path", "//testing/base:source_gen_lib", + "@google_benchmark//:benchmark", "@llvm-project//llvm:Support", ], ) @@ -110,6 +113,7 @@ cc_test( deps = [ ":driver", "//common:all_llvm_targets", + "//testing/base:global_exe_path", "//testing/base:gtest_main", "//testing/base:test_raw_ostream", "//toolchain/diagnostics:diagnostic_emitter", diff --git a/toolchain/driver/clang_runner_test.cpp b/toolchain/driver/clang_runner_test.cpp index 5a413f95663b..09d6a4b42c14 100644 --- a/toolchain/driver/clang_runner_test.cpp +++ b/toolchain/driver/clang_runner_test.cpp @@ -18,7 +18,7 @@ #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Program.h" #include "llvm/TargetParser/Host.h" -#include "testing/base/gtest_main.h" +#include "testing/base/global_exe_path.h" #include "testing/base/test_raw_ostream.h" namespace Carbon { @@ -56,7 +56,7 @@ static auto RunWithCapturedOutput(std::string& out, std::string& err, TEST(ClangRunnerTest, Version) { TestRawOstream test_os; const auto install_paths = - InstallPaths::MakeForBazelRunfiles(Testing::GetTestExePath()); + InstallPaths::MakeForBazelRunfiles(Testing::GetExePath()); std::string target = llvm::sys::getDefaultTargetTriple(); ClangRunner runner(&install_paths, target, &test_os); @@ -125,7 +125,7 @@ TEST(ClangRunnerTest, LinkCommandEcho) { std::filesystem::path bar_file = WriteTestFile("bar.o", ""); const auto install_paths = - InstallPaths::MakeForBazelRunfiles(Testing::GetTestExePath()); + InstallPaths::MakeForBazelRunfiles(Testing::GetExePath()); std::string verbose_out; llvm::raw_string_ostream verbose_os(verbose_out); std::string target = llvm::sys::getDefaultTargetTriple(); diff --git a/toolchain/driver/compile_benchmark.cpp b/toolchain/driver/compile_benchmark.cpp index b68a2a1fd1d8..1701b5fc82ed 100644 --- a/toolchain/driver/compile_benchmark.cpp +++ b/toolchain/driver/compile_benchmark.cpp @@ -6,7 +6,7 @@ #include -#include "common/benchmark_main.h" +#include "testing/base/global_exe_path.h" #include "testing/base/source_gen.h" #include "toolchain/driver/driver.h" @@ -20,8 +20,7 @@ namespace { class CompileBenchmark { public: CompileBenchmark() - : installation_( - InstallPaths::MakeForBazelRunfiles(GetBenchmarkExePath())), + : installation_(InstallPaths::MakeForBazelRunfiles(GetExePath())), driver_(fs_, &installation_, llvm::outs(), llvm::errs()) { // Load the prelude into our VFS. // diff --git a/toolchain/driver/driver_test.cpp b/toolchain/driver/driver_test.cpp index d751a2f749ad..009835081fce 100644 --- a/toolchain/driver/driver_test.cpp +++ b/toolchain/driver/driver_test.cpp @@ -14,7 +14,7 @@ #include "llvm/ADT/ScopeExit.h" #include "llvm/Object/Binary.h" #include "llvm/Support/FormatVariadic.h" -#include "testing/base/gtest_main.h" +#include "testing/base/global_exe_path.h" #include "testing/base/test_raw_ostream.h" #include "toolchain/testing/yaml_test_helpers.h" @@ -43,7 +43,7 @@ class DriverTest : public testing::Test { protected: DriverTest() : installation_( - InstallPaths::MakeForBazelRunfiles(Testing::GetTestExePath())), + InstallPaths::MakeForBazelRunfiles(Testing::GetExePath())), driver_(fs_, &installation_, test_output_stream_, test_error_stream_) { char* tmpdir_env = getenv("TEST_TMPDIR"); CARBON_CHECK(tmpdir_env != nullptr); diff --git a/toolchain/install/BUILD b/toolchain/install/BUILD index 996ec8a9385f..f2e249dcea82 100644 --- a/toolchain/install/BUILD +++ b/toolchain/install/BUILD @@ -144,6 +144,7 @@ cc_test( ":install_paths", "//common:check", "//common:ostream", + "//testing/base:global_exe_path", "//testing/base:gtest_main", "@bazel_tools//tools/cpp/runfiles", "@googletest//:gtest", diff --git a/toolchain/install/install_paths_test.cpp b/toolchain/install/install_paths_test.cpp index 85784f0e810e..b3c980417bc5 100644 --- a/toolchain/install/install_paths_test.cpp +++ b/toolchain/install/install_paths_test.cpp @@ -12,7 +12,7 @@ #include "llvm/Support/FileSystem.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" -#include "testing/base/gtest_main.h" +#include "testing/base/global_exe_path.h" #include "tools/cpp/runfiles/runfiles.h" namespace Carbon { @@ -28,8 +28,7 @@ class InstallPathsTest : public ::testing::Test { protected: InstallPathsTest() { std::string error; - test_runfiles_.reset( - Runfiles::Create(Testing::GetTestExePath().str(), &error)); + test_runfiles_.reset(Runfiles::Create(Testing::GetExePath().str(), &error)); CARBON_CHECK(test_runfiles_ != nullptr) << error; } @@ -102,7 +101,7 @@ TEST_F(InstallPathsTest, PrefixRootExplicit) { } TEST_F(InstallPathsTest, TestRunfiles) { - auto paths = InstallPaths::MakeForBazelRunfiles(Testing::GetTestExePath()); + auto paths = InstallPaths::MakeForBazelRunfiles(Testing::GetExePath()); ASSERT_THAT(paths.error(), Eq(std::nullopt)) << *paths.error(); TestInstallPaths(paths); } diff --git a/toolchain/lex/BUILD b/toolchain/lex/BUILD index 004468a86705..4753ce75bfba 100644 --- a/toolchain/lex/BUILD +++ b/toolchain/lex/BUILD @@ -84,8 +84,8 @@ cc_binary( srcs = ["numeric_literal_benchmark.cpp"], deps = [ ":numeric_literal", - "//common:benchmark_main", "//common:check", + "//testing/base:benchmark_main", "//toolchain/diagnostics:null_diagnostics", "@google_benchmark//:benchmark", ], @@ -140,7 +140,7 @@ cc_binary( srcs = ["string_literal_benchmark.cpp"], deps = [ ":string_literal", - "//common:benchmark_main", + "//testing/base:benchmark_main", "//toolchain/diagnostics:null_diagnostics", "@google_benchmark//:benchmark", ], @@ -287,8 +287,8 @@ cc_binary( ":lex", ":token_kind", ":tokenized_buffer", - "//common:benchmark_main", "//common:check", + "//testing/base:benchmark_main", "//testing/base:source_gen_lib", "//toolchain/base:value_store", "//toolchain/diagnostics:diagnostic_emitter", diff --git a/toolchain/sem_ir/BUILD b/toolchain/sem_ir/BUILD index 7d6294368d35..5ea9dea9c347 100644 --- a/toolchain/sem_ir/BUILD +++ b/toolchain/sem_ir/BUILD @@ -196,6 +196,7 @@ cc_test( srcs = ["yaml_test.cpp"], deps = [ "//common:ostream", + "//testing/base:global_exe_path", "//testing/base:gtest_main", "//testing/base:test_raw_ostream", "//toolchain/driver", diff --git a/toolchain/sem_ir/yaml_test.cpp b/toolchain/sem_ir/yaml_test.cpp index ed91257231fd..34456d1c4eb7 100644 --- a/toolchain/sem_ir/yaml_test.cpp +++ b/toolchain/sem_ir/yaml_test.cpp @@ -8,7 +8,7 @@ #include "common/ostream.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/VirtualFileSystem.h" -#include "testing/base/gtest_main.h" +#include "testing/base/global_exe_path.h" #include "testing/base/test_raw_ostream.h" #include "toolchain/driver/driver.h" #include "toolchain/testing/yaml_test_helpers.h" @@ -36,7 +36,7 @@ TEST(SemIRTest, YAML) { "test.carbon", /*ModificationTime=*/0, llvm::MemoryBuffer::getMemBuffer("fn F() { var x: () = (); return; }"))); const auto install_paths = - InstallPaths::MakeForBazelRunfiles(Testing::GetTestExePath()); + InstallPaths::MakeForBazelRunfiles(Testing::GetExePath()); TestRawOstream print_stream; Driver d(fs, &install_paths, print_stream, llvm::errs()); auto run_result =