[ThreadPool] Fix Windows failures in the AffinityMask unit test

[nga888]

These thread pool tests are typed and one was failing due to the setting of environment variable LLVM_THREADPOOL_AFFINITYMASK leaking out of a previous run of the test. I suspect that this has mainly succeeded because the tests have run in separate shards.

Also the re-invocation of the unit test with the process affinity mask set was not actually running the test because --gtest_filter= was not set correctly. This has been fixed.

The use of the test type (TypeParam) in the AsyncBarrier test has been restored. Without this the typing of these tests would be completely redundant.

I suspect that some of these unit tests do not work if LLVM_ENABLE_THREADS == 0 but that is beyond the scope of this fix.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Andrew Ng (nga888)

Changes

These thread pool tests are typed and one was failing due to the setting of environment variable LLVM_THREADPOOL_AFFINITYMASK leaking out of a previous run of the test. I suspect that this has mainly succeeded because the tests have run in separate shards.

Also the re-invocation of the unit test with the process affinity mask set was not actually running the test because --gtest_filter= was not set correctly. This has been fixed.

The use of the test type (TypeParam) in the AsyncBarrier test has been restored. Without this the typing of these tests would be completely redundant.

I suspect that some of these unit tests do not work if LLVM_ENABLE_THREADS == 0 but that is beyond the scope of this fix.

---

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

1 Files Affected:

• (modified) llvm/unittests/Support/ThreadPool.cpp (+10-4)

diff --git a/llvm/unittests/Support/ThreadPool.cpp b/llvm/unittests/Support/ThreadPool.cpp
index 8dca182882ef3..aa7f8744e1417 100644
--- a/llvm/unittests/Support/ThreadPool.cpp
+++ b/llvm/unittests/Support/ThreadPool.cpp
@@ -140,7 +140,7 @@ TYPED_TEST(ThreadPoolTest, AsyncBarrier) {
 
   std::atomic_int checked_in{0};
 
-  DefaultThreadPool Pool;
+  TypeParam Pool;
   for (size_t i = 0; i < 5; ++i) {
     Pool.async([this, &checked_in] {
       this->waitForMainThread();
@@ -453,15 +453,19 @@ TYPED_TEST(ThreadPoolTest, AffinityMask) {
                         [](auto &T) { return T.getData().front() < 16UL; }) &&
            "Threads ran on more CPUs than expected! The affinity mask does not "
            "seem to work.");
-    GTEST_SKIP();
+    return;
   }
   std::string Executable =
       sys::fs::getMainExecutable(TestMainArgv0, &ThreadPoolTestStringArg1);
-  StringRef argv[] = {Executable, "--gtest_filter=ThreadPoolTest.AffinityMask"};
+  const auto *TestInfo = testing::UnitTest::GetInstance()->current_test_info();
+  std::string Arg = (Twine("--gtest_filter=") + TestInfo->test_suite_name() +
+                     "." + TestInfo->name())
+                        .str();
+  StringRef argv[] = {Executable, Arg};
 
   // Add environment variable to the environment of the child process.
   int Res = setenv("LLVM_THREADPOOL_AFFINITYMASK", "1", false);
-  ASSERT_EQ(Res, 0);
+  ASSERT_EQ(0, Res);
 
   std::string Error;
   bool ExecutionFailed;
@@ -470,6 +474,8 @@ TYPED_TEST(ThreadPoolTest, AffinityMask) {
   Affinity.set(0, 4); // Use CPUs 0,1,2,3.
   int Ret = sys::ExecuteAndWait(Executable, argv, {}, {}, 0, 0, &Error,
                                 &ExecutionFailed, nullptr, &Affinity);
+  Res = setenv("LLVM_THREADPOOL_AFFINITYMASK", "", false);
+  ASSERT_EQ(0, Res);
   ASSERT_EQ(0, Ret);
 }
 

[joker-eph]

I'm surprised we're doing this kind of thing, that seems quite hacky to me.

That said your fix is just improving the status quo.

[joker-eph]

I suspect that some of these unit tests do not work if LLVM_ENABLE_THREADS == 0 but that is beyond the scope of this fix.

Should be acknowledged in the test, I see:

  // Skip this test if less than 4 threads are available.
  if (llvm::hardware_concurrency().compute_thread_count() < 4)
    GTEST_SKIP();

[nga888]

I suspect that some of these unit tests do not work if LLVM_ENABLE_THREADS == 0 but that is beyond the scope of this fix.

Should be acknowledged in the test, I see:

  // Skip this test if less than 4 threads are available.
  if (llvm::hardware_concurrency().compute_thread_count() < 4)
    GTEST_SKIP();

I was referring to the other unit tests in this file, not AffinityMask.