From e770b1b86e7157db4191096f226a8b0175819cff Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 8 Dec 2017 07:04:36 -0500 Subject: CTest: Fix regression in build-and-test timeout compuatation Refactoring in commit 66419bc046 (CTest: convert timeouts to std::chrono::duration, 2017-11-20) accidentally changed the logic used to compute the timeout for a test when it starts. It incorrectly limits the maximum possible timeout to 2 minutes rather than 2 minutes less than the total allowed test time remaining. Update the new logic to restore the original behavior. Avoid subtracting 2 minutes from our "infinite" timeout value to avoid creating very large timeouts that are not "infinite" and may exceed integer type ranges. --- Source/cmCTest.cxx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index a4ca301..2cd60e5 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -1089,9 +1089,10 @@ int cmCTest::RunTest(std::vector argv, std::string* output, bool modifyEnv = (environment && !environment->empty()); // determine how much time we have - std::chrono::duration timeout = - std::min>(this->GetRemainingTimeAllowed(), - std::chrono::minutes(2)); + std::chrono::duration timeout = this->GetRemainingTimeAllowed(); + if (timeout != std::chrono::duration::max()) { + timeout -= std::chrono::minutes(2); + } if (this->TimeOut > std::chrono::duration::zero() && this->TimeOut < timeout) { timeout = this->TimeOut; -- cgit v0.12 From 687a26b7023748cc98317eae53ee2ac3cc520bda Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 8 Dec 2017 07:33:39 -0500 Subject: CTest: Fix regression in build-and-test timeout forwarding Refactoring in commit 66419bc046 (CTest: convert timeouts to std::chrono::duration, 2017-11-20) changed out "infinite" timeout to a value not representable by a 64-bit integer. Update the `--build-and-test` forwarding of `--test-timeout` to not forward an "infinite" timeout. --- Source/cmCTest.cxx | 1 + 1 file changed, 1 insertion(+) diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index 2cd60e5..ed45644 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -1131,6 +1131,7 @@ int cmCTest::RunTest(std::vector argv, std::string* output, // invocations. Since --build-generator is required this is a // good place to check for it, and to add the arguments in if (strcmp(i, "--build-generator") == 0 && + timeout != std::chrono::duration::max() && timeout > std::chrono::duration::zero()) { args.push_back("--test-timeout"); std::ostringstream msg; -- cgit v0.12 From de0035fdcccb4c8acc92bf1db0a76b3c1f18003f Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 8 Dec 2017 08:07:26 -0500 Subject: cmCTestBuildAndTestHandler: Convert timeout to std::chrono::duration --- Source/CTest/cmCTestBuildAndTestHandler.cxx | 19 +++++++++---------- Source/CTest/cmCTestBuildAndTestHandler.h | 3 ++- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Source/CTest/cmCTestBuildAndTestHandler.cxx b/Source/CTest/cmCTestBuildAndTestHandler.cxx index 672087d..85d98d0 100644 --- a/Source/CTest/cmCTestBuildAndTestHandler.cxx +++ b/Source/CTest/cmCTestBuildAndTestHandler.cxx @@ -11,6 +11,7 @@ #include "cmsys/Process.h" #include +#include #include cmCTestBuildAndTestHandler::cmCTestBuildAndTestHandler() @@ -18,7 +19,7 @@ cmCTestBuildAndTestHandler::cmCTestBuildAndTestHandler() this->BuildTwoConfig = false; this->BuildNoClean = false; this->BuildNoCMake = false; - this->Timeout = 0; + this->Timeout = std::chrono::duration::zero(); } void cmCTestBuildAndTestHandler::Initialize() @@ -224,10 +225,9 @@ int cmCTestBuildAndTestHandler::RunCMakeAndTest(std::string* outstring) } for (std::string const& tar : this->BuildTargets) { std::chrono::duration remainingTime = std::chrono::seconds(0); - if (this->Timeout > 0) { - remainingTime = std::chrono::duration(this->Timeout) - - std::chrono::duration_cast( - std::chrono::steady_clock::now() - clock_start); + if (this->Timeout > std::chrono::duration::zero()) { + remainingTime = + this->Timeout - (std::chrono::steady_clock::now() - clock_start); if (remainingTime <= std::chrono::seconds(0)) { if (outstring) { *outstring = "--build-and-test timeout exceeded. "; @@ -324,10 +324,9 @@ int cmCTestBuildAndTestHandler::RunCMakeAndTest(std::string* outstring) // how much time is remaining std::chrono::duration remainingTime = std::chrono::seconds(0); - if (this->Timeout > 0) { - remainingTime = std::chrono::duration(this->Timeout) - - std::chrono::duration_cast( - std::chrono::steady_clock::now() - clock_start); + if (this->Timeout > std::chrono::duration::zero()) { + remainingTime = + this->Timeout - (std::chrono::steady_clock::now() - clock_start); if (remainingTime <= std::chrono::seconds(0)) { if (outstring) { *outstring = "--build-and-test timeout exceeded. "; @@ -396,7 +395,7 @@ int cmCTestBuildAndTestHandler::ProcessCommandLineArguments( } if (currentArg.find("--test-timeout", 0) == 0 && idx < allArgs.size() - 1) { idx++; - this->Timeout = atof(allArgs[idx].c_str()); + this->Timeout = std::chrono::duration(atof(allArgs[idx].c_str())); } if (currentArg == "--build-generator" && idx < allArgs.size() - 1) { idx++; diff --git a/Source/CTest/cmCTestBuildAndTestHandler.h b/Source/CTest/cmCTestBuildAndTestHandler.h index f19cb67..f8a9ed7 100644 --- a/Source/CTest/cmCTestBuildAndTestHandler.h +++ b/Source/CTest/cmCTestBuildAndTestHandler.h @@ -7,6 +7,7 @@ #include "cmCTestGenericHandler.h" +#include #include #include #include @@ -67,7 +68,7 @@ protected: std::vector TestCommandArgs; std::vector BuildTargets; bool BuildNoCMake; - double Timeout; + std::chrono::duration Timeout; }; #endif -- cgit v0.12 From 548e8f6ffedb57fcdee56633ba454ae0ddf38983 Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 8 Dec 2017 08:22:15 -0500 Subject: CTest: Simplify std::chrono::duration conversion to double The ratio of ticks to seconds for this type is 1, so we can just use its `count()` directly. This also avoids converting through the integer representation of `std::chrono::milliseconds`, which has a much smaller allowed range. Drop our `cmsysProcess_SetTimeout` wrapper as it is now very thin. --- Source/CTest/cmCTestCoverageHandler.cxx | 3 +-- Source/CTest/cmCTestRunTest.cxx | 12 ++---------- Source/CTest/cmCTestTestHandler.cxx | 17 ++++------------- Source/CTest/cmProcess.cxx | 19 +++---------------- Source/CTest/cmProcess.h | 7 ------- Source/cmCTest.cxx | 7 +++---- 6 files changed, 13 insertions(+), 52 deletions(-) diff --git a/Source/CTest/cmCTestCoverageHandler.cxx b/Source/CTest/cmCTestCoverageHandler.cxx index 39b90d8..2a9fd72 100644 --- a/Source/CTest/cmCTestCoverageHandler.cxx +++ b/Source/CTest/cmCTestCoverageHandler.cxx @@ -11,7 +11,6 @@ #include "cmParseGTMCoverage.h" #include "cmParseJacocoCoverage.h" #include "cmParsePHPCoverage.h" -#include "cmProcess.h" #include "cmSystemTools.h" #include "cmWorkingDirectory.h" #include "cmXMLWriter.h" @@ -81,7 +80,7 @@ public: cmsysProcess_SetOption(this->Process, cmsysProcess_Option_HideWindow, 1); if (this->TimeOut >= std::chrono::duration::zero()) { - cmsysProcess_SetTimeout(this->Process, this->TimeOut); + cmsysProcess_SetTimeout(this->Process, this->TimeOut.count()); } cmsysProcess_Execute(this->Process); this->PipeState = cmsysProcess_GetState(this->Process); diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index a075649..c76eb4e 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/Source/CTest/cmCTestRunTest.cxx @@ -265,11 +265,7 @@ bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started) passed = this->TestResult.Status == cmCTestTestHandler::COMPLETED; char buf[1024]; - sprintf(buf, "%6.2f sec", - double(std::chrono::duration_cast( - this->TestProcess->GetTotalTime()) - .count()) / - 1000.0); + sprintf(buf, "%6.2f sec", this->TestProcess->GetTotalTime().count()); cmCTestLog(this->CTest, HANDLER_OUTPUT, buf << "\n"); if (outputTestErrorsToConsole) { @@ -394,11 +390,7 @@ void cmCTestRunTest::ComputeWeightedCost() { double prev = static_cast(this->TestProperties->PreviousRuns); double avgcost = static_cast(this->TestProperties->Cost); - double current = - double(std::chrono::duration_cast( - this->TestResult.ExecutionTime) - .count()) / - 1000.0; + double current = this->TestResult.ExecutionTime.count(); if (this->TestResult.Status == cmCTestTestHandler::COMPLETED) { this->TestProperties->Cost = diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx index 1e15cc5..e5a0503 100644 --- a/Source/CTest/cmCTestTestHandler.cxx +++ b/Source/CTest/cmCTestTestHandler.cxx @@ -540,10 +540,8 @@ int cmCTestTestHandler::ProcessHandler() this->PrintLabelOrSubprojectSummary(false); } char realBuf[1024]; - auto durationInMs = std::chrono::duration_cast( - clock_finish - clock_start) - .count(); - sprintf(realBuf, "%6.2f sec", static_cast(durationInMs) / 1000.0); + std::chrono::duration durationInSecs = clock_finish - clock_start; + sprintf(realBuf, "%6.2f sec", durationInSecs.count()); cmCTestOptionalLog(this->CTest, HANDLER_OUTPUT, "\nTotal Test time (real) = " << realBuf << "\n", this->Quiet); @@ -654,10 +652,7 @@ void cmCTestTestHandler::PrintLabelOrSubprojectSummary(bool doSubProject) // only use labels found in labels if (labels.find(l) != labels.end()) { labelTimes[l] += - double(std::chrono::duration_cast( - result.ExecutionTime) - .count()) / - 1000.0 * result.Properties->Processors; + result.ExecutionTime.count() * result.Properties->Processors; ++labelCounts[l]; } } @@ -1327,11 +1322,7 @@ void cmCTestTestHandler::GenerateDartOutput(cmXMLWriter& xml) xml.StartElement("NamedMeasurement"); xml.Attribute("type", "numeric/double"); xml.Attribute("name", "Execution Time"); - xml.Element("Value", - double(std::chrono::duration_cast( - result.ExecutionTime) - .count()) / - 1000.0); + xml.Element("Value", result.ExecutionTime.count()); xml.EndElement(); // NamedMeasurement if (!result.Reason.empty()) { const char* reasonType = "Pass Reason"; diff --git a/Source/CTest/cmProcess.cxx b/Source/CTest/cmProcess.cxx index 69ffb33..0db66c3 100644 --- a/Source/CTest/cmProcess.cxx +++ b/Source/CTest/cmProcess.cxx @@ -5,16 +5,6 @@ #include "cmProcessOutput.h" #include -void cmsysProcess_SetTimeout(cmsysProcess* process, - std::chrono::duration timeout) -{ - cmsysProcess_SetTimeout( - process, - double( - std::chrono::duration_cast(timeout).count()) / - 1000.0); -}; - cmProcess::cmProcess() { this->Process = nullptr; @@ -59,7 +49,7 @@ bool cmProcess::StartProcess() cmsysProcess_SetWorkingDirectory(this->Process, this->WorkingDirectory.c_str()); } - cmsysProcess_SetTimeout(this->Process, this->Timeout); + cmsysProcess_SetTimeout(this->Process, this->Timeout.count()); cmsysProcess_SetOption(this->Process, cmsysProcess_Option_MergeOutput, 1); cmsysProcess_Execute(this->Process); return (cmsysProcess_GetState(this->Process) == @@ -115,10 +105,7 @@ int cmProcess::GetNextOutputLine(std::string& line, { cmProcessOutput processOutput(cmProcessOutput::UTF8); std::string strdata; - double waitTimeout = - double( - std::chrono::duration_cast(timeout).count()) / - 1000.0; + double waitTimeout = timeout.count(); for (;;) { // Look for lines already buffered. if (this->Output.GetLine(line)) { @@ -240,7 +227,7 @@ int cmProcess::ReportStatus() void cmProcess::ChangeTimeout(std::chrono::duration t) { this->Timeout = t; - cmsysProcess_SetTimeout(this->Process, this->Timeout); + cmsysProcess_SetTimeout(this->Process, this->Timeout.count()); } void cmProcess::ResetStartTime() diff --git a/Source/CTest/cmProcess.h b/Source/CTest/cmProcess.h index cbb611d..f3b0bd7 100644 --- a/Source/CTest/cmProcess.h +++ b/Source/CTest/cmProcess.h @@ -10,13 +10,6 @@ #include #include -/* - * A wrapper function for cmsysProcess_SetTimeout that takes an - * std::chrono::duration. For convenience only. - */ -void cmsysProcess_SetTimeout(cmsysProcess* process, - std::chrono::duration timeout); - /** \class cmProcess * \brief run a process with c++ * diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index ed45644..902f1ae 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -50,7 +50,6 @@ #include "cmGeneratedFileStream.h" #include "cmGlobalGenerator.h" #include "cmMakefile.h" -#include "cmProcess.h" #include "cmProcessOutput.h" #include "cmState.h" #include "cmStateSnapshot.h" @@ -998,7 +997,7 @@ int cmCTest::RunMakeCommand(const char* command, std::string& output, cmsysProcess_SetCommand(cp, &*argv.begin()); cmsysProcess_SetWorkingDirectory(cp, dir); cmsysProcess_SetOption(cp, cmsysProcess_Option_HideWindow, 1); - cmsysProcess_SetTimeout(cp, timeout); + cmsysProcess_SetTimeout(cp, timeout.count()); cmsysProcess_Execute(cp); // Initialize tick's @@ -1186,7 +1185,7 @@ int cmCTest::RunTest(std::vector argv, std::string* output, cmsysProcess_SetOption(cp, cmsysProcess_Option_HideWindow, 1); } - cmsysProcess_SetTimeout(cp, timeout); + cmsysProcess_SetTimeout(cp, timeout.count()); cmsysProcess_Execute(cp); char* data; @@ -2610,7 +2609,7 @@ bool cmCTest::RunCommand(std::vector const& args, if (cmSystemTools::GetRunCommandHideConsole()) { cmsysProcess_SetOption(cp, cmsysProcess_Option_HideWindow, 1); } - cmsysProcess_SetTimeout(cp, timeout); + cmsysProcess_SetTimeout(cp, timeout.count()); cmsysProcess_Execute(cp); std::vector tempOutput; -- cgit v0.12 From ba6caafa42afb7fa2c7d8d0d49c73d79efee1e9c Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 11 Dec 2017 10:40:29 -0500 Subject: CTest: Use integer-representable value for "infinite" timeout Refactoring in commit 66419bc046 (CTest: convert timeouts to std::chrono::duration, 2017-11-20) changed out "infinite" timeout to a value not representable by a 64-bit integer. This causes undefined behavior when e.g. KWSys Process converts the duration to a `long` to interact with system APIs. Use the old `1.0e7` maximum value. --- Source/CTest/cmCTestRunTest.cxx | 8 ++------ Source/CTest/cmCTestScriptHandler.cxx | 11 ++--------- Source/cmCTest.cxx | 19 +++++++++---------- Source/cmCTest.h | 2 ++ 4 files changed, 15 insertions(+), 25 deletions(-) diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index c76eb4e..dcb17d0 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/Source/CTest/cmCTestRunTest.cxx @@ -1,9 +1,5 @@ /* Distributed under the OSI-approved BSD 3-Clause License. See accompanying file Copyright.txt or https://cmake.org/licensing for details. */ -#ifdef _WIN32 -/* windows.h defines min() and max() macros that interfere. */ -#define NOMINMAX -#endif #include "cmCTestRunTest.h" #include "cmCTest.h" @@ -678,7 +674,7 @@ bool cmCTestRunTest::ForkProcess(std::chrono::duration testTimeOut, // determine how much time we have std::chrono::duration timeout = this->CTest->GetRemainingTimeAllowed(); - if (timeout != std::chrono::duration::max()) { + if (timeout != cmCTest::MaxDuration()) { timeout -= std::chrono::minutes(2); } if (this->CTest->GetTimeOut() > std::chrono::duration::zero() && @@ -702,7 +698,7 @@ bool cmCTestRunTest::ForkProcess(std::chrono::duration testTimeOut, this->CTest, HANDLER_VERBOSE_OUTPUT, this->Index << ": " << "Test timeout computed to be: " - << (timeout == std::chrono::duration::max() + << (timeout == cmCTest::MaxDuration() ? std::string("infinite") : std::to_string( std::chrono::duration_cast(timeout) diff --git a/Source/CTest/cmCTestScriptHandler.cxx b/Source/CTest/cmCTestScriptHandler.cxx index 922f5c7..716ea10 100644 --- a/Source/CTest/cmCTestScriptHandler.cxx +++ b/Source/CTest/cmCTestScriptHandler.cxx @@ -1,12 +1,5 @@ /* Distributed under the OSI-approved BSD 3-Clause License. See accompanying file Copyright.txt or https://cmake.org/licensing for details. */ - -#ifdef _WIN32 -/* windows.h defines min() and max() macros, unless told to otherwise. This - * interferes with std::min() and std::max() at the very least. */ -#define NOMINMAX -#endif - #include "cmCTestScriptHandler.h" #include "cmsys/Directory.hxx" @@ -970,13 +963,13 @@ bool cmCTestScriptHandler::TryToRemoveBinaryDirectoryOnce( std::chrono::duration cmCTestScriptHandler::GetRemainingTimeAllowed() { if (!this->Makefile) { - return std::chrono::duration::max(); + return cmCTest::MaxDuration(); } const char* timelimitS = this->Makefile->GetDefinition("CTEST_TIME_LIMIT"); if (!timelimitS) { - return std::chrono::duration::max(); + return cmCTest::MaxDuration(); } auto timelimit = std::chrono::duration(atof(timelimitS)); diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index 902f1ae..e48f64a 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -1,11 +1,5 @@ /* Distributed under the OSI-approved BSD 3-Clause License. See accompanying file Copyright.txt or https://cmake.org/licensing for details. */ -#ifdef _WIN32 -/* windows.h defines min() and max() macros by default. This interferes with - * C++ functions names. - */ -#define NOMINMAX -#endif #include "cmCTest.h" #include "cm_curl.h" @@ -1089,7 +1083,7 @@ int cmCTest::RunTest(std::vector argv, std::string* output, // determine how much time we have std::chrono::duration timeout = this->GetRemainingTimeAllowed(); - if (timeout != std::chrono::duration::max()) { + if (timeout != cmCTest::MaxDuration()) { timeout -= std::chrono::minutes(2); } if (this->TimeOut > std::chrono::duration::zero() && @@ -1107,7 +1101,7 @@ int cmCTest::RunTest(std::vector argv, std::string* output, } cmCTestLog( this, HANDLER_VERBOSE_OUTPUT, "Test timeout computed to be: " - << (timeout == std::chrono::duration::max() + << (timeout == cmCTest::MaxDuration() ? std::string("infinite") : std::to_string( std::chrono::duration_cast(timeout) @@ -1130,7 +1124,7 @@ int cmCTest::RunTest(std::vector argv, std::string* output, // invocations. Since --build-generator is required this is a // good place to check for it, and to add the arguments in if (strcmp(i, "--build-generator") == 0 && - timeout != std::chrono::duration::max() && + timeout != cmCTest::MaxDuration() && timeout > std::chrono::duration::zero()) { args.push_back("--test-timeout"); std::ostringstream msg; @@ -2805,7 +2799,7 @@ void cmCTest::Log(int logType, const char* file, int line, const char* msg, std::chrono::duration cmCTest::GetRemainingTimeAllowed() { if (!this->GetHandler("script")) { - return std::chrono::duration::max(); + return cmCTest::MaxDuration(); } cmCTestScriptHandler* ch = @@ -2814,6 +2808,11 @@ std::chrono::duration cmCTest::GetRemainingTimeAllowed() return ch->GetRemainingTimeAllowed(); } +std::chrono::duration cmCTest::MaxDuration() +{ + return std::chrono::duration(1.0e7); +} + void cmCTest::OutputTestErrors(std::vector const& process_output) { std::string test_outputs("\n*** Test Failed:\n"); diff --git a/Source/cmCTest.h b/Source/cmCTest.h index ba94866..418d576 100644 --- a/Source/cmCTest.h +++ b/Source/cmCTest.h @@ -208,6 +208,8 @@ public: */ std::chrono::duration GetRemainingTimeAllowed(); + static std::chrono::duration MaxDuration(); + /** * Open file in the output directory and set the stream */ -- cgit v0.12