Fix hw detection more
authorMark Abraham <mark.j.abraham@gmail.com>
Thu, 26 Oct 2017 08:36:13 +0000 (10:36 +0200)
committerMark Abraham <mark.j.abraham@gmail.com>
Fri, 27 Oct 2017 12:20:46 +0000 (14:20 +0200)
gmx_hardware_detect was called in response to GoogleTest environment
SetUp function, so the cleanup for its global should occur in response
to the corresponding TearDown function. Both those should be virtual.
Thus the hardwareInfo should not be in a smart pointer called by a
destructor that might be called at a different point from TearDown.

The new getter function and the callback that handles making the first
call to it conform better to GoogleTest's recommendation to arrange to
call AddGlobalTestEnvironment from main() rather than rely on static
initialization.

Made hardwareInit a non-member function because that improves
encapsulation.

Change-Id: I2f8e14ecc1707bf31d023a4eb4fea0a20543910b

src/gromacs/ewald/tests/CMakeLists.txt
src/gromacs/ewald/tests/pmegathertest.cpp
src/gromacs/ewald/tests/pmesolvetest.cpp
src/gromacs/ewald/tests/pmesplinespreadtest.cpp
src/gromacs/ewald/tests/testhardwarecontexts.cpp
src/gromacs/ewald/tests/testhardwarecontexts.h
src/testutils/TestMacros.cmake
src/testutils/testinit.cpp
src/testutils/testinit.h
src/testutils/unittest_main.cpp

index 3155182ae7e8f4b9fc265af193e32351b2dadbbc..26756429a237008975a770481a48047cf47bde0c 100644 (file)
@@ -37,5 +37,5 @@ if (GMX_USE_CUDA)
     file(GLOB EWALD_CUDA_SOURCES ../*.cu)
 endif()
 
-gmx_add_unit_test(EwaldUnitTests ewald-test
+gmx_add_unit_test(EwaldUnitTests ewald-test HARDWARE_DETECTION
                   ${EWALD_TEST_SOURCES} ${EWALD_CUDA_SOURCES})
index 00eada0855d20b452e2c48e5657dfb47152668b9..ef29c954755d70c9c14e2864973096a21f71aec9 100644 (file)
@@ -403,7 +403,7 @@ class PmeGatherTest : public ::testing::TestWithParam<GatherInputParameters>
                     continue;
                 }
 
-                const auto contextsToTest = pmeEnv->getHardwareContexts(mode.first);
+                const auto contextsToTest = getPmeTestEnv()->getHardwareContexts(mode.first);
                 for (const auto &context : contextsToTest)
                 {
                     /* Describing the test uniquely */
index da0d84923f943232fd1965a0003c8287f34de28e..3957c7cb00566a9489d474f84592fe3654d9ab67 100644 (file)
@@ -127,7 +127,7 @@ class PmeSolveTest : public ::testing::TestWithParam<SolveInputParameters>
                 {
                     gridOrderingsToTest[GridOrdering::XYZ] = "XYZ";
                 }
-                const auto contextsToTest = pmeEnv->getHardwareContexts(mode.first);
+                const auto contextsToTest = getPmeTestEnv()->getHardwareContexts(mode.first);
                 for (const auto &gridOrdering : gridOrderingsToTest)
                 {
                     for (const auto &context : contextsToTest)
index 67ea564d330fb3a3eb5cf0f809cd8949596bbea2..3bb5917f35fd6413d40619ab97647220c309a20b 100644 (file)
@@ -134,7 +134,7 @@ class PmeSplineAndSpreadTest : public ::testing::TestWithParam<SplineAndSpreadIn
                     continue;
                 }
 
-                const auto contextsToTest = pmeEnv->getHardwareContexts(mode.first);
+                const auto contextsToTest = getPmeTestEnv()->getHardwareContexts(mode.first);
                 for (const auto &context : contextsToTest)
                 {
                     for (const auto &option : optionsToTest)
index 762e4d482a8a2fbfac18d8bf384927b4bd5265b0..08d6fa2bea6a76ec9fcda5ce1d4ce016785a9837 100644 (file)
 #include "gromacs/taskassignment/hardwareassign.h"
 #include "gromacs/utility/exceptions.h"
 #include "gromacs/utility/loggerbuilder.h"
+#include "gromacs/utility/unique_cptr.h"
 
 namespace gmx
 {
 namespace test
 {
 
-//! This constructs the test environment
-PmeTestEnvironment * const pmeEnv = (PmeTestEnvironment *)::testing::AddGlobalTestEnvironment(new PmeTestEnvironment);
+/* Implements the "construct on first use" idiom to avoid any static
+ * initialization order fiasco.
+ *
+ * Note that thread-safety of the initialization is guaranteed by the
+ * C++11 language standard.
+ *
+ * The pointer itself (not the memory it points to) has no destructor,
+ * so there is no deinitialization issue.  See
+ * https://isocpp.org/wiki/faq/ctors for discussion of alternatives
+ * and trade-offs. */
+const PmeTestEnvironment *getPmeTestEnv()
+{
+    static PmeTestEnvironment *pmeTestEnvironment = nullptr;
+    if (pmeTestEnvironment == nullptr)
+    {
+        // Ownership of the TestEnvironment is taken by GoogleTest, so nothing can leak
+        pmeTestEnvironment = static_cast<PmeTestEnvironment *>(::testing::AddGlobalTestEnvironment(new PmeTestEnvironment));
+    }
+    return pmeTestEnvironment;
+}
+
+void callAddGlobalTestEnvironment()
+{
+    getPmeTestEnv();
+}
 
 //! Simple hardware initialization
-void PmeTestEnvironment::hardwareInit()
+static gmx_hw_info_t *hardwareInit()
 {
     unique_cptr<t_commrec, done_commrec> commrec(init_commrec());
     gmx_init_intranode_counters(commrec.get());
     LoggerBuilder builder;
     LoggerOwner   logOwner(builder.build());
     MDLogger      log(logOwner.logger());
-    hardwareInfo_.reset(gmx_detect_hardware(log, commrec.get()));
+    return gmx_detect_hardware(log, commrec.get());
 }
 
 void PmeTestEnvironment::SetUp()
@@ -77,7 +101,7 @@ void PmeTestEnvironment::SetUp()
     TestHardwareContext emptyContext("", nullptr);
     hardwareContextsByMode_[CodePath::CPU].push_back(emptyContext);
 
-    hardwareInit();
+    hardwareInfo_ = hardwareInit();
 
     // Constructing contexts for all compatible GPUs - will be empty on non-GPU builds
     TestHardwareContexts gpuContexts;
@@ -87,14 +111,17 @@ void PmeTestEnvironment::SetUp()
         char        stmp[200] = {};
         get_gpu_device_info_string(stmp, hardwareInfo_->gpu_info, gpuIndex);
         std::string description = "(GPU " + std::string(stmp) + ") ";
-        auto       *gpuInfo     = reinterpret_cast<gmx_device_info_t *>(reinterpret_cast<char *>(hardwareInfo_->gpu_info.gpu_dev) + gpuIndex * sizeof_gpu_dev_info());
-        //TODO move previous line to gpu_utils and reuse in hardwareassign.cpp
-        gpuContexts.emplace_back(TestHardwareContext(description.c_str(), gpuInfo));
+        gpuContexts.emplace_back(TestHardwareContext(description.c_str(), getDeviceInfo(hardwareInfo_->gpu_info, gpuIndex)));
     }
 #if GMX_GPU == GMX_GPU_CUDA
     hardwareContextsByMode_[CodePath::CUDA] = gpuContexts;
 #endif
 }
 
+void PmeTestEnvironment::TearDown()
+{
+    gmx_hardware_info_free(hardwareInfo_);
+}
+
 }
 }
index 9fc2b51336debffa03032af12695c445803afcf9..82128b75df1f18b229cb3a4167daa3867c06415f 100644 (file)
@@ -50,7 +50,6 @@
 
 #include "gromacs/hardware/detecthardware.h"
 #include "gromacs/hardware/gpu_hw_info.h"
-#include "gromacs/utility/unique_cptr.h"
 
 namespace gmx
 {
@@ -91,27 +90,31 @@ typedef std::list<TestHardwareContext> TestHardwareContexts;
 /*! \internal \brief
  * This class performs one-time test initialization (enumerating the hardware)
  */
+// cppcheck-suppress noConstructor
 class PmeTestEnvironment : public ::testing::Environment
 {
     private:
         //! General hardware info
-        unique_cptr<gmx_hw_info_t, gmx_hardware_info_free> hardwareInfo_;
+        gmx_hw_info_t *hardwareInfo_;
         //! Storage of hardware contexts
-        std::map<CodePath, TestHardwareContexts>           hardwareContextsByMode_;
-        //! Simple GPU initialization, allowing for PME to work on GPU
-        void hardwareInit();
+        std::map<CodePath, TestHardwareContexts> hardwareContextsByMode_;
 
     public:
-        //! Default
-        ~PmeTestEnvironment() = default;
         //! This is called by GTest framework once to query the hardware
-        void SetUp();
+        virtual void SetUp();
+        //! This is called by GTest framework once to clean up
+        virtual void TearDown();
         //! Get available hardware contexts for given code path
-        const TestHardwareContexts &getHardwareContexts(CodePath mode){return hardwareContextsByMode_.at(mode); }
+        const TestHardwareContexts &getHardwareContexts(CodePath mode) const {return hardwareContextsByMode_.at(mode); }
 };
 
-//! The test environment
-extern PmeTestEnvironment * const pmeEnv;
+//! Get the test environment
+const PmeTestEnvironment *getPmeTestEnv();
+
+/*! \brief This constructs the test environment during setup of the
+ * unit test so that they can use the hardware context. */
+void callAddGlobalTestEnvironment();
+
 }
 }
 #endif
index 5b71c5972d47aa80c72139a2bf986bce099e0cff..539b40d5d10f97b0c9f2e676e1d2985bd88c7fbb 100644 (file)
@@ -1,7 +1,7 @@
 #
 # This file is part of the GROMACS molecular simulation package.
 #
-# Copyright (c) 2011,2012,2013,2014,2015,2016, by the GROMACS development team, led by
+# Copyright (c) 2011,2012,2013,2014,2015,2016,2017, by the GROMACS development team, led by
 # Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
 # and including many others, as listed in the AUTHORS file in the
 # top-level source directory and at http://www.gromacs.org.
@@ -45,7 +45,7 @@ endfunction ()
 
 function (gmx_add_gtest_executable EXENAME)
     if (GMX_BUILD_UNITTESTS AND BUILD_TESTING)
-        set(_options MPI)
+        set(_options MPI HARDWARE_DETECTION)
         cmake_parse_arguments(ARG "${_options}" "" "" ${ARGN})
         set(_source_files ${ARG_UNPARSED_ARGUMENTS})
 
@@ -66,6 +66,10 @@ function (gmx_add_gtest_executable EXENAME)
             list(APPEND EXTRA_COMPILE_DEFINITIONS
                  TEST_USES_MPI=true)
         endif()
+        if (ARG_HARDWARE_DETECTION)
+            list(APPEND EXTRA_COMPILE_DEFINITIONS
+                 TEST_USES_HARDWARE_DETECTION=true)
+        endif()
 
         include_directories(BEFORE SYSTEM ${GMOCK_INCLUDE_DIRS})
         add_executable(${EXENAME} ${UNITTEST_TARGET_OPTIONS}
index 751e3e5d18d078a98478f353700f77d7cf1a2b22..e7a2e90ba2d08e5ad06ee2d02947585b7feb024c 100644 (file)
@@ -158,7 +158,7 @@ std::unique_ptr<TestProgramContext> g_testContext;
 
 //! \cond internal
 void initTestUtils(const char *dataPath, const char *tempPath, bool usesMpi,
-                   int *argc, char ***argv)
+                   bool usesHardwareDetection, int *argc, char ***argv)
 {
 #ifndef NDEBUG
     gmx_feenableexcept();
@@ -181,6 +181,10 @@ void initTestUtils(const char *dataPath, const char *tempPath, bool usesMpi,
             finalizeForCommandLine();
             std::exit(1);
         }
+        if (usesHardwareDetection)
+        {
+            callAddGlobalTestEnvironment();
+        }
         g_testContext.reset(new TestProgramContext(context));
         setProgramContext(g_testContext.get());
         // Use the default finder that does not respect GMXLIB, since the tests
index 1ce1cbbb28e0dda5d97be101547cd75f75d3c051..3842434f4979d8ec16ae74f72c6ede7988b9f111 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2012,2013,2014,2015,2016, by the GROMACS development team, led by
+ * Copyright (c) 2012,2013,2014,2015,2016,2017, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -61,7 +61,7 @@ namespace test
  * \ingroup module_testutils
  */
 void initTestUtils(const char *dataPath, const char *tempPath, bool usesMpi,
-                   int *argc, char ***argv);
+                   bool usesHardwareDetection, int *argc, char ***argv);
 
 /*! \internal
  * \brief
@@ -77,6 +77,19 @@ void initTestUtils(const char *dataPath, const char *tempPath, bool usesMpi,
 void finalizeTestUtils();
 //! \endcond
 
+/*! \brief Declare a function that all unit test implementations can use
+ * to set up any environment that they need.
+ *
+ * When registering the unit test in CMake, the HARDWARE_DETECTION
+ * flag requires that the code for that unit test implements this
+ * function.  Otherwise, a default stub implementation is provided.
+ *
+ * This approach conforms to the recommendation by GoogleTest to
+ * arrange for the code that sets up the global test environment to be
+ * called from main, rather than potentially rely on brittle static
+ * initialization order. */
+void callAddGlobalTestEnvironment();
+
 } // namespace test
 } // namespace gmx
 
index 562f1422c1dd3d6ab0741ea344fe2d8fddad415d..706ba13e58139c130ac97f79224fa4b20e1b3502 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2010,2011,2012,2013,2014,2015,2016, by the GROMACS development team, led by
+ * Copyright (c) 2010,2011,2012,2013,2014,2015,2016,2017, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
 #define TEST_USES_MPI false
 #endif
 
+#ifndef TEST_USES_HARDWARE_DETECTION
+//! Whether the test expects/supports running with knowledge of the hardware.
+#define TEST_USES_HARDWARE_DETECTION false
+namespace gmx
+{
+namespace test
+{
+//! Implement a stub definition for tests that don't ask for a real one.
+void callAddGlobalTestEnvironment() {};
+}
+}
+#endif
+
 /*! \brief
  * Initializes unit testing for \ref module_testutils.
  */
 int main(int argc, char *argv[])
 {
     // Calls ::testing::InitGoogleMock()
-    ::gmx::test::initTestUtils(TEST_DATA_PATH, TEST_TEMP_PATH, TEST_USES_MPI,
+    ::gmx::test::initTestUtils(TEST_DATA_PATH, TEST_TEMP_PATH,
+                               TEST_USES_MPI, TEST_USES_HARDWARE_DETECTION,
                                &argc, &argv);
     int errcode = RUN_ALL_TESTS();
     ::gmx::test::finalizeTestUtils();