From f964739ead050d9c3ecdedb5ae717b0af2a2886f Mon Sep 17 00:00:00 2001
From: Marc Chevrier <marc.chevrier@gmail.com>
Date: Tue, 10 Mar 2020 16:04:51 +0100
Subject: cmCTestRunTest: modernize memory management
---
Source/CTest/cmCTestMultiProcessHandler.cxx | 44 ++++++++++-----------
Source/CTest/cmCTestMultiProcessHandler.h | 3 +-
Source/CTest/cmCTestRunTest.cxx | 61 ++++++++++++++++++++---------
Source/CTest/cmCTestRunTest.h | 13 ++++--
Source/CTest/cmProcess.cxx | 28 ++++++-------
Source/CTest/cmProcess.h | 11 +++++-
6 files changed, 100 insertions(+), 60 deletions(-)
diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx
index 2192843..50c963d 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.cxx
+++ b/Source/CTest/cmCTestMultiProcessHandler.cxx
@@ -12,13 +12,13 @@
#include <iomanip>
#include <iostream>
#include <list>
-#include <memory>
#include <sstream>
#include <stack>
#include <unordered_map>
#include <utility>
#include <vector>
+#include <cm/memory>
#include <cmext/algorithm>
#include "cmsys/FStream.hxx"
@@ -172,7 +172,8 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
this->EraseTest(test);
this->RunningCount += GetProcessorsUsed(test);
- cmCTestRunTest* testRun = new cmCTestRunTest(*this);
+ auto testRun = cm::make_unique<cmCTestRunTest>(*this);
+
if (this->RepeatMode != cmCTest::Repeat::Never) {
testRun->SetRepeatMode(this->RepeatMode);
testRun->SetNumberOfRuns(this->RepeatCount);
@@ -229,28 +230,25 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
e << "\n";
}
e << "Resource spec file:\n\n " << this->TestHandler->ResourceSpecFile;
- testRun->StartFailure(e.str(), "Insufficient resources");
- this->FinishTestProcess(testRun, false);
+ cmCTestRunTest::StartFailure(std::move(testRun), e.str(),
+ "Insufficient resources");
return false;
}
cmWorkingDirectory workdir(this->Properties[test]->Directory);
if (workdir.Failed()) {
- testRun->StartFailure("Failed to change working directory to " +
- this->Properties[test]->Directory + " : " +
- std::strerror(workdir.GetLastResult()),
- "Failed to change working directory");
- } else {
- if (testRun->StartTest(this->Completed, this->Total)) {
- // Ownership of 'testRun' has moved to another structure.
- // When the test finishes, FinishTestProcess will be called.
- return true;
- }
+ cmCTestRunTest::StartFailure(std::move(testRun),
+ "Failed to change working directory to " +
+ this->Properties[test]->Directory + " : " +
+ std::strerror(workdir.GetLastResult()),
+ "Failed to change working directory");
+ return false;
}
- // Pass ownership of 'testRun'.
- this->FinishTestProcess(testRun, false);
- return false;
+ // Ownership of 'testRun' has moved to another structure.
+ // When the test finishes, FinishTestProcess will be called.
+ return cmCTestRunTest::StartTest(std::move(testRun), this->Completed,
+ this->Total);
}
bool cmCTestMultiProcessHandler::AllocateResources(int index)
@@ -540,7 +538,8 @@ void cmCTestMultiProcessHandler::StartNextTests()
if (this->SerialTestRunning) {
break;
}
- // We can only start a RUN_SERIAL test if no other tests are also running.
+ // We can only start a RUN_SERIAL test if no other tests are also
+ // running.
if (this->Properties[test]->RunSerial && this->RunningCount > 0) {
continue;
}
@@ -618,8 +617,8 @@ void cmCTestMultiProcessHandler::OnTestLoadRetryCB(uv_timer_t* timer)
self->StartNextTests();
}
-void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
- bool started)
+void cmCTestMultiProcessHandler::FinishTestProcess(
+ std::unique_ptr<cmCTestRunTest> runner, bool started)
{
this->Completed++;
@@ -631,7 +630,8 @@ void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
this->SetStopTimePassed();
}
if (started) {
- if (!this->StopTimePassed && runner->StartAgain(this->Completed)) {
+ if (!this->StopTimePassed &&
+ cmCTestRunTest::StartAgain(std::move(runner), this->Completed)) {
this->Completed--; // remove the completed test because run again
return;
}
@@ -659,7 +659,7 @@ void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
}
properties->Affinity.clear();
- delete runner;
+ runner.reset();
if (started) {
this->StartNextTests();
}
diff --git a/Source/CTest/cmCTestMultiProcessHandler.h b/Source/CTest/cmCTestMultiProcessHandler.h
index 5b429d4..c3686bc 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.h
+++ b/Source/CTest/cmCTestMultiProcessHandler.h
@@ -6,6 +6,7 @@
#include "cmConfigure.h" // IWYU pragma: keep
#include <map>
+#include <memory>
#include <set>
#include <string>
#include <vector>
@@ -124,7 +125,7 @@ protected:
// Removes the checkpoint file
void MarkFinished();
void EraseTest(int index);
- void FinishTestProcess(cmCTestRunTest* runner, bool started);
+ void FinishTestProcess(std::unique_ptr<cmCTestRunTest> runner, bool started);
static void OnTestLoadRetryCB(uv_timer_t* timer);
diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx
index ec54960..7d0f69b 100644
--- a/Source/CTest/cmCTestRunTest.cxx
+++ b/Source/CTest/cmCTestRunTest.cxx
@@ -314,23 +314,27 @@ bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started)
return passed || skipped;
}
-bool cmCTestRunTest::StartAgain(size_t completed)
+bool cmCTestRunTest::StartAgain(std::unique_ptr<cmCTestRunTest> runner,
+ size_t completed)
{
- if (!this->RunAgain) {
+ auto* testRun = runner.get();
+
+ if (!testRun->RunAgain) {
return false;
}
- this->RunAgain = false; // reset
+ testRun->RunAgain = false; // reset
+ testRun->TestProcess = cm::make_unique<cmProcess>(std::move(runner));
// change to tests directory
- cmWorkingDirectory workdir(this->TestProperties->Directory);
+ cmWorkingDirectory workdir(testRun->TestProperties->Directory);
if (workdir.Failed()) {
- this->StartFailure("Failed to change working directory to " +
- this->TestProperties->Directory + " : " +
- std::strerror(workdir.GetLastResult()),
- "Failed to change working directory");
+ testRun->StartFailure("Failed to change working directory to " +
+ testRun->TestProperties->Directory + " : " +
+ std::strerror(workdir.GetLastResult()),
+ "Failed to change working directory");
return true;
}
- this->StartTest(completed, this->TotalNumberOfTests);
+ testRun->StartTest(completed, testRun->TotalNumberOfTests);
return true;
}
@@ -382,6 +386,18 @@ void cmCTestRunTest::MemCheckPostProcess()
handler->PostProcessTest(this->TestResult, this->Index);
}
+void cmCTestRunTest::StartFailure(std::unique_ptr<cmCTestRunTest> runner,
+ std::string const& output,
+ std::string const& detail)
+{
+ auto* testRun = runner.get();
+
+ testRun->TestProcess = cm::make_unique<cmProcess>(std::move(runner));
+ testRun->StartFailure(output, detail);
+
+ testRun->FinalizeTest(false);
+}
+
void cmCTestRunTest::StartFailure(std::string const& output,
std::string const& detail)
{
@@ -413,7 +429,6 @@ void cmCTestRunTest::StartFailure(std::string const& output,
this->TestResult.Path = this->TestProperties->Directory;
this->TestResult.Output = output;
this->TestResult.FullCommandLine.clear();
- this->TestProcess = cm::make_unique<cmProcess>(*this);
}
std::string cmCTestRunTest::GetTestPrefix(size_t completed, size_t total) const
@@ -437,6 +452,21 @@ std::string cmCTestRunTest::GetTestPrefix(size_t completed, size_t total) const
return outputStream.str();
}
+bool cmCTestRunTest::StartTest(std::unique_ptr<cmCTestRunTest> runner,
+ size_t completed, size_t total)
+{
+ auto* testRun = runner.get();
+
+ testRun->TestProcess = cm::make_unique<cmProcess>(std::move(runner));
+
+ if (!testRun->StartTest(completed, total)) {
+ testRun->FinalizeTest(false);
+ return false;
+ }
+
+ return true;
+}
+
// Starts the execution of a test. Returns once it has started
bool cmCTestRunTest::StartTest(size_t completed, size_t total)
{
@@ -468,7 +498,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
if (this->TestProperties->Disabled) {
this->TestResult.CompletionStatus = "Disabled";
this->TestResult.Status = cmCTestTestHandler::NOT_RUN;
- this->TestProcess = cm::make_unique<cmProcess>(*this);
this->TestResult.Output = "Disabled";
this->TestResult.FullCommandLine.clear();
return false;
@@ -482,7 +511,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
// its arguments are irrelevant. This matters for the case where a fixture
// dependency might be creating the executable we want to run.
if (!this->FailedDependencies.empty()) {
- this->TestProcess = cm::make_unique<cmProcess>(*this);
std::string msg = "Failed test dependencies:";
for (std::string const& failedDep : this->FailedDependencies) {
msg += " " + failedDep;
@@ -499,7 +527,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
this->ComputeArguments();
std::vector<std::string>& args = this->TestProperties->Args;
if (args.size() >= 2 && args[1] == "NOT_AVAILABLE") {
- this->TestProcess = cm::make_unique<cmProcess>(*this);
std::string msg;
if (this->CTest->GetConfigType().empty()) {
msg = "Test not available without configuration. (Missing \"-C "
@@ -521,7 +548,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
for (std::string const& file : this->TestProperties->RequiredFiles) {
if (!cmSystemTools::FileExists(file)) {
// Required file was not found
- this->TestProcess = cm::make_unique<cmProcess>(*this);
*this->TestHandler->LogFile << "Unable to find required file: " << file
<< std::endl;
cmCTestLog(this->CTest, ERROR_MESSAGE,
@@ -537,7 +563,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
if (this->ActualCommand.empty()) {
// if the command was not found create a TestResult object
// that has that information
- this->TestProcess = cm::make_unique<cmProcess>(*this);
*this->TestHandler->LogFile << "Unable to find executable: " << args[1]
<< std::endl;
cmCTestLog(this->CTest, ERROR_MESSAGE,
@@ -649,7 +674,6 @@ bool cmCTestRunTest::ForkProcess(cmDuration testTimeOut, bool explicitTimeout,
std::vector<std::string>* environment,
std::vector<size_t>* affinity)
{
- this->TestProcess = cm::make_unique<cmProcess>(*this);
this->TestProcess->SetId(this->Index);
this->TestProcess->SetWorkingDirectory(this->TestProperties->Directory);
this->TestProcess->SetCommand(this->ActualCommand);
@@ -816,7 +840,8 @@ void cmCTestRunTest::WriteLogOutputTop(size_t completed, size_t total)
"Testing " << this->TestProperties->Name << " ... ");
}
-void cmCTestRunTest::FinalizeTest()
+void cmCTestRunTest::FinalizeTest(bool started)
{
- this->MultiTestHandler.FinishTestProcess(this, true);
+ this->MultiTestHandler.FinishTestProcess(this->TestProcess->GetRunner(),
+ started);
}
diff --git a/Source/CTest/cmCTestRunTest.h b/Source/CTest/cmCTestRunTest.h
index 4988839..b1d188a 100644
--- a/Source/CTest/cmCTestRunTest.h
+++ b/Source/CTest/cmCTestRunTest.h
@@ -65,6 +65,15 @@ public:
// Read and store output. Returns true if it must be called again.
void CheckOutput(std::string const& line);
+ static bool StartTest(std::unique_ptr<cmCTestRunTest> runner,
+ size_t completed, size_t total);
+ static bool StartAgain(std::unique_ptr<cmCTestRunTest> runner,
+ size_t completed);
+
+ static void StartFailure(std::unique_ptr<cmCTestRunTest> runner,
+ std::string const& output,
+ std::string const& detail);
+
// launch the test process, return whether it started correctly
bool StartTest(size_t completed, size_t total);
// capture and report the test results
@@ -74,8 +83,6 @@ public:
void ComputeWeightedCost();
- bool StartAgain(size_t completed);
-
void StartFailure(std::string const& output, std::string const& detail);
cmCTest* GetCTest() const { return this->CTest; }
@@ -84,7 +91,7 @@ public:
const std::vector<std::string>& GetArguments() { return this->Arguments; }
- void FinalizeTest();
+ void FinalizeTest(bool started = true);
bool TimedOutForStopTime() const { return this->TimeoutIsForStopTime; }
diff --git a/Source/CTest/cmProcess.cxx b/Source/CTest/cmProcess.cxx
index cdf899c..76ffb20 100644
--- a/Source/CTest/cmProcess.cxx
+++ b/Source/CTest/cmProcess.cxx
@@ -5,6 +5,7 @@
#include <csignal>
#include <iostream>
#include <string>
+#include <utility>
#include <cmext/algorithm>
@@ -18,12 +19,11 @@
#if defined(_WIN32)
# include "cm_kwiml.h"
#endif
-#include <utility>
#define CM_PROCESS_BUF_SIZE 65536
-cmProcess::cmProcess(cmCTestRunTest& runner)
- : Runner(runner)
+cmProcess::cmProcess(std::unique_ptr<cmCTestRunTest> runner)
+ : Runner(std::move(runner))
, Conv(cmProcessOutput::UTF8, CM_PROCESS_BUF_SIZE)
{
this->Timeout = cmDuration::zero();
@@ -69,7 +69,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
cm::uv_timer_ptr timer;
int status = timer.init(loop, this);
if (status != 0) {
- cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+ cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
"Error initializing timer: " << uv_strerror(status)
<< std::endl);
return false;
@@ -84,7 +84,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
int fds[2] = { -1, -1 };
status = cmGetPipes(fds);
if (status != 0) {
- cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+ cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
"Error initializing pipe: " << uv_strerror(status)
<< std::endl);
return false;
@@ -127,7 +127,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
uv_read_start(pipe_reader, &cmProcess::OnAllocateCB, &cmProcess::OnReadCB);
if (status != 0) {
- cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+ cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
"Error starting read events: " << uv_strerror(status)
<< std::endl);
return false;
@@ -135,7 +135,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
status = this->Process.spawn(loop, options, this);
if (status != 0) {
- cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+ cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
"Process not started\n " << this->Command << "\n["
<< uv_strerror(status) << "]\n");
return false;
@@ -152,7 +152,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
void cmProcess::StartTimer()
{
- auto properties = this->Runner.GetTestProperties();
+ auto properties = this->Runner->GetTestProperties();
auto msec =
std::chrono::duration_cast<std::chrono::milliseconds>(this->Timeout);
@@ -222,7 +222,7 @@ void cmProcess::OnRead(ssize_t nread, const uv_buf_t* buf)
cm::append(this->Output, strdata);
while (this->Output.GetLine(line)) {
- this->Runner.CheckOutput(line);
+ this->Runner->CheckOutput(line);
line.clear();
}
@@ -236,20 +236,20 @@ void cmProcess::OnRead(ssize_t nread, const uv_buf_t* buf)
// The process will provide no more data.
if (nread != UV_EOF) {
auto error = static_cast<int>(nread);
- cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+ cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
"Error reading stream: " << uv_strerror(error) << std::endl);
}
// Look for partial last lines.
if (this->Output.GetLast(line)) {
- this->Runner.CheckOutput(line);
+ this->Runner->CheckOutput(line);
}
this->ReadHandleClosed = true;
this->PipeReader.reset();
if (this->ProcessHandleClosed) {
uv_timer_stop(this->Timer);
- this->Runner.FinalizeTest();
+ this->Runner->FinalizeTest();
}
}
@@ -291,7 +291,7 @@ void cmProcess::OnTimeout()
// Our on-exit handler already ran but did not finish the test
// because we were still reading output. We've just dropped
// our read handler, so we need to finish the test now.
- this->Runner.FinalizeTest();
+ this->Runner->FinalizeTest();
}
}
@@ -333,7 +333,7 @@ void cmProcess::OnExit(int64_t exit_status, int term_signal)
this->ProcessHandleClosed = true;
if (this->ReadHandleClosed) {
uv_timer_stop(this->Timer);
- this->Runner.FinalizeTest();
+ this->Runner->FinalizeTest();
}
}
diff --git a/Source/CTest/cmProcess.h b/Source/CTest/cmProcess.h
index 2c24f2d..0f69f68 100644
--- a/Source/CTest/cmProcess.h
+++ b/Source/CTest/cmProcess.h
@@ -6,7 +6,9 @@
#include "cmConfigure.h" // IWYU pragma: keep
#include <chrono>
+#include <memory>
#include <string>
+#include <utility>
#include <vector>
#include <stddef.h>
@@ -28,7 +30,7 @@ class cmCTestRunTest;
class cmProcess
{
public:
- explicit cmProcess(cmCTestRunTest& runner);
+ explicit cmProcess(std::unique_ptr<cmCTestRunTest> runner);
~cmProcess();
void SetCommand(std::string const& command);
void SetCommandArguments(std::vector<std::string> const& arg);
@@ -70,6 +72,11 @@ public:
Exception GetExitException();
std::string GetExitExceptionString();
+ std::unique_ptr<cmCTestRunTest> GetRunner()
+ {
+ return std::move(this->Runner);
+ }
+
private:
cmDuration Timeout;
std::chrono::steady_clock::time_point StartTime;
@@ -82,7 +89,7 @@ private:
cm::uv_timer_ptr Timer;
std::vector<char> Buf;
- cmCTestRunTest& Runner;
+ std::unique_ptr<cmCTestRunTest> Runner;
cmProcessOutput Conv;
int Signal = 0;
cmProcess::State ProcessState = cmProcess::State::Starting;
--
cgit v0.12