Valgrind: Remove CallgrindController's use of ValgrindProcess

It's simply one-shot command execution, using (only some part of) the
ValgrindProcess machinery that just happens to also to wrap a process
is conceptually different from ValgrindProcess that "is" the
valgrind-with-debuggee entity (and an unneeded dependency)

Change-Id: I57a2c3d1cab6b15e59cb41b8e131948c170297b6
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

src/plugins/valgrind/callgrind/callgrindcontroller.cpp

@@ -24,28 +24,31 @@
 ****************************************************************************/
 
 #include "callgrindcontroller.h"
-#include "../valgrindprocess.h"
+
+#include <ssh/sftpchannel.h>
+#include <ssh/sshconnectionmanager.h>
+
+#include <utils/fileutils.h>
+#include <utils/hostosinfo.h>
+#include <utils/qtcassert.h>
+#include <utils/qtcprocess.h>
+#include <utils/temporaryfile.h>
 
 #include <QDebug>
 #include <QDir>
-
+#include <QEventLoop>
-#include <utils/hostosinfo.h>
-#include <utils/qtcassert.h>
-#include <utils/temporaryfile.h>
-#include <ssh/sftpchannel.h>
 
 #define CALLGRIND_CONTROL_DEBUG 0
 
-const QLatin1String CALLGRIND_CONTROL_BINARY("callgrind_control");
+using namespace ProjectExplorer;
+using namespace Utils;
 
 namespace Valgrind {
 namespace Callgrind {
 
-CallgrindController::CallgrindController(QObject *parent)
+const QLatin1String CALLGRIND_CONTROL_BINARY("callgrind_control");
-    : QObject(parent)
+
-    , m_process(0)
+CallgrindController::CallgrindController()
-    , m_valgrindProc(0)
-    , m_lastOption(Unknown)
 {
 }
 
@@ -54,7 +57,7 @@ CallgrindController::~CallgrindController()
     cleanupTempFile();
 }
 
-QString toOptionString(CallgrindController::Option option)
+static QString toOptionString(CallgrindController::Option option)
 {
     /* callgrind_control help from v3.9.0
 
@@ -86,23 +89,15 @@ QString toOptionString(CallgrindController::Option option)
 
 void CallgrindController::run(Option option)
 {
-    if (m_process) {
+    if (m_controllerProcess) {
         emit statusMessage(tr("Previous command has not yet finished."));
         return;
     }
-    QTC_ASSERT(m_valgrindProc, return);
-
-    m_process = new ValgrindProcess(m_valgrindProc->device(), this);
-
-    connect(m_process, &ValgrindProcess::finished,
-            this, &CallgrindController::processFinished);
-    connect(m_process, &ValgrindProcess::error,
-            this, &CallgrindController::processError);
 
     // save back current running operation
     m_lastOption = option;
 
-    const QString optionString = toOptionString(option);
+    m_controllerProcess = new ApplicationLauncher;
 
     switch (option) {
         case CallgrindController::Dump:
@@ -122,31 +117,49 @@ void CallgrindController::run(Option option)
     }
 
 #if CALLGRIND_CONTROL_DEBUG
-    m_process->setProcessChannelMode(QProcess::ForwardedChannels);
+    m_controllerProcess->setProcessChannelMode(QProcess::ForwardedChannels);
 #endif
-    const int pid = Utils::HostOsInfo::isWindowsHost() ? 0 : m_valgrindProc->pid();
+    connect(m_controllerProcess, &ApplicationLauncher::processExited,
-    m_process->setValgrindExecutable(CALLGRIND_CONTROL_BINARY);
+            this, &CallgrindController::controllerProcessFinished);
-    m_process->setValgrindArguments(QStringList() << optionString << QString::number(pid));
+    connect(m_controllerProcess, &ApplicationLauncher::error,
-    m_process->run(ProjectExplorer::ApplicationLauncher::Gui);
+            this, &CallgrindController::handleControllerProcessError);
+    connect(m_controllerProcess, &ApplicationLauncher::finished,
+            this, &CallgrindController::controllerProcessClosed);
+
+    StandardRunnable controller = m_valgrindRunnable;
+    controller.executable =  CALLGRIND_CONTROL_BINARY;
+    controller.runMode = ApplicationLauncher::Gui;
+    controller.commandLineArguments = QString("%1 %2").arg(toOptionString(option)).arg(m_pid);
+
+    if (!m_valgrindRunnable.device
+            || m_valgrindRunnable.device->type() == ProjectExplorer::Constants::DESKTOP_DEVICE_TYPE)
+        m_controllerProcess->start(controller);
+    else
+        m_controllerProcess->start(controller, m_valgrindRunnable.device);
 }
 
-void CallgrindController::processError(QProcess::ProcessError)
+void CallgrindController::setValgrindPid(qint64 pid)
 {
-    QTC_ASSERT(m_process, return);
+    m_pid = pid;
-    const QString error = m_process->errorString();
+}
+
+void CallgrindController::handleControllerProcessError(QProcess::ProcessError)
+{
+    QTC_ASSERT(m_controllerProcess, return);
+    const QString error = m_controllerProcess->errorString();
     emit statusMessage(tr("An error occurred while trying to run %1: %2").arg(CALLGRIND_CONTROL_BINARY).arg(error));
 
-    m_process->deleteLater();
+    m_controllerProcess->deleteLater();
-    m_process = 0;
+    m_controllerProcess = nullptr;
 }
 
-void CallgrindController::processFinished(int rc, QProcess::ExitStatus status)
+void CallgrindController::controllerProcessFinished(int rc, QProcess::ExitStatus status)
 {
-    QTC_ASSERT(m_process, return);
+    QTC_ASSERT(m_controllerProcess, return);
-    const QString error = m_process->errorString();
+    const QString error = m_controllerProcess->errorString();
 
-    m_process->deleteLater(); // Called directly from finished() signal in m_process
+    m_controllerProcess->deleteLater(); // Called directly from finished() signal in m_process
-    m_process = 0;
+    m_controllerProcess = nullptr;
 
     if (rc != 0 || status != QProcess::NormalExit) {
         qWarning() << "Controller exited abnormally:" << error;
@@ -175,23 +188,34 @@ void CallgrindController::processFinished(int rc, QProcess::ExitStatus status)
     m_lastOption = Unknown;
 }
 
-void CallgrindController::setValgrindProcess(ValgrindProcess *proc)
+void CallgrindController::controllerProcessClosed(bool success)
 {
-    m_valgrindProc = proc;
+    Q_UNUSED(success);
+    //    QTC_ASSERT(m_remote.m_process, return);
+
+//    m_remote.m_errorString = m_remote.m_process->errorString();
+//    if (status == QSsh::SshRemoteProcess::FailedToStart) {
+//        m_remote.m_error = QProcess::FailedToStart;
+//        emit ValgrindProcessX::error(QProcess::FailedToStart);
+//    } else if (status == QSsh::SshRemoteProcess::NormalExit) {
+//        emit finished(m_remote.m_process->exitCode(), QProcess::NormalExit);
+//    } else if (status == QSsh::SshRemoteProcess::CrashExit) {
+//        m_remote.m_error = QProcess::Crashed;
+//        emit finished(m_remote.m_process->exitCode(), QProcess::CrashExit);
+//    }
+     controllerProcessFinished(0, QProcess::NormalExit);
 }
 
 void CallgrindController::getLocalDataFile()
 {
-    QTC_ASSERT(m_valgrindProc, return);
-
     // we look for callgrind.out.PID, but there may be updated ones called ~.PID.NUM
-    QString baseFileName = QString::fromLatin1("callgrind.out.%1").
+    const QString baseFileName = QString("callgrind.out.%1").arg(m_pid);
-            arg(m_valgrindProc->pid());
+    const QString workingDir = m_valgrindRunnable.workingDirectory;
-    const QString workingDir = m_valgrindProc->workingDirectory();
     // first, set the to-be-parsed file to callgrind.out.PID
-    QString fileName = workingDir.isEmpty() ? baseFileName : (workingDir + QLatin1Char('/') + baseFileName);
+    QString fileName = workingDir.isEmpty() ? baseFileName : (workingDir + '/' + baseFileName);
 
-    if (!m_valgrindProc->isLocal()) {
+    if (m_valgrindRunnable.device
+            && m_valgrindRunnable.device->type() != ProjectExplorer::Constants::DESKTOP_DEVICE_TYPE) {
 //        ///TODO: error handling
 //        emit statusMessage(tr("Downloading remote profile data..."));
 //        m_ssh = m_valgrindProc->connection();
@@ -254,5 +278,11 @@ void CallgrindController::cleanupTempFile()
     m_tempDataFile.clear();
 }
 
+void CallgrindController::setValgrindRunnable(const Runnable &runnable)
+{
+    QTC_ASSERT(runnable.is<StandardRunnable>(), return);
+    m_valgrindRunnable = runnable.as<StandardRunnable>();
+}
+
 } // namespace Callgrind
 } // namespace Valgrind

src/plugins/valgrind/callgrind/callgrindcontroller.h

@@ -25,18 +25,15 @@
 
 #pragma once
 
-#include <QObject>
-
-#include <qprocess.h>
-
-#include <ssh/sshconnection.h>
 #include <ssh/sshremoteprocess.h>
 #include <ssh/sftpchannel.h>
+#include <ssh/sshconnection.h>
+
+#include <projectexplorer/runnables.h>
+
+#include <QProcess>
 
 namespace Valgrind {
-
-class ValgrindProcess;
-
 namespace Callgrind {
 
 class CallgrindController : public QObject
@@ -49,16 +46,14 @@ public:
         Unknown,
         Dump,
         ResetEventCounters,
-        Pause, UnPause
+        Pause,
+        UnPause
     };
 
-    explicit CallgrindController(QObject *parent = 0);
+    CallgrindController();
-    virtual ~CallgrindController();
+    ~CallgrindController() override;
 
-    void run(Valgrind::Callgrind::CallgrindController::Option option);
+    void run(Option option);
-
-    void setValgrindProcess(ValgrindProcess *process);
-    ValgrindProcess *valgrindProcess() { return m_valgrindProc; }
 
     /**
      * Make data file available locally, triggers @c localParseDataAvailable.
@@ -67,32 +62,34 @@ public:
      * downloads the data file first and returns a local path.
      */
     void getLocalDataFile();
+    void setValgrindPid(qint64 pid);
+    void setValgrindRunnable(const ProjectExplorer::Runnable &runnable);
 
 signals:
     void finished(Valgrind::Callgrind::CallgrindController::Option option);
-
     void localParseDataAvailable(const QString &file);
-
     void statusMessage(const QString &msg);
 
 private:
-    void processError(QProcess::ProcessError);
+    void handleControllerProcessError(QProcess::ProcessError);
-    void processFinished(int, QProcess::ExitStatus);
 
     void foundRemoteFile();
     void sftpInitialized();
     void sftpJobFinished(QSsh::SftpJobId job, const QString &error);
     void cleanupTempFile();
 
-    // callgrind_control process
+    void controllerProcessFinished(int, QProcess::ExitStatus);
-    Valgrind::ValgrindProcess *m_process;
+    void controllerProcessError(QProcess::ProcessError);
-    // valgrind process
+    void controllerProcessClosed(bool success);
-    Valgrind::ValgrindProcess *m_valgrindProc;
 
-    Option m_lastOption;
+    ProjectExplorer::ApplicationLauncher *m_controllerProcess;
+    ProjectExplorer::StandardRunnable m_valgrindRunnable;
+    qint64 m_pid = 0;
+
+    Option m_lastOption = Unknown;
 
     // remote callgrind support
-    QSsh::SshConnection *m_ssh;
+    QSsh::SshConnection *m_ssh = nullptr;
     QString m_tempDataFile;
     QSsh::SshRemoteProcess::Ptr m_findRemoteFile;
     QSsh::SftpChannel::Ptr m_sftp;

src/plugins/valgrind/callgrindengine.cpp

@@ -30,17 +30,21 @@
 
 #include <valgrind/callgrind/callgrindcontroller.h>
 #include <valgrind/callgrind/callgrindparser.h>
+#include <valgrind/valgrindrunner.h>
+#include <valgrind/valgrindprocess.h>
 
 #include <debugger/analyzer/analyzermanager.h>
 
 #include <utils/qtcassert.h>
 
 using namespace Debugger;
-using namespace Valgrind;
+using namespace ProjectExplorer;
-using namespace Valgrind::Internal;
 using namespace Valgrind::Callgrind;
 
-CallgrindToolRunner::CallgrindToolRunner(ProjectExplorer::RunControl *runControl)
+namespace Valgrind {
+namespace Internal {
+
+CallgrindToolRunner::CallgrindToolRunner(RunControl *runControl)
     : ValgrindToolRunner(runControl)
 {
     setDisplayName("CallgrindToolRunner");
@@ -58,14 +62,14 @@ CallgrindToolRunner::CallgrindToolRunner(ProjectExplorer::RunControl *runControl
     connect(&m_controller, &CallgrindController::statusMessage,
             this, &CallgrindToolRunner::showStatusMessage);
 
-    connect(&m_runner, &ValgrindRunner::extraStart, this, [this] {
+    connect(m_runner.valgrindProcess(), &ValgrindProcess::valgrindStarted,
-        m_controller.setValgrindProcess(m_runner.valgrindProcess());
+            &m_controller, &CallgrindController::setValgrindPid);
-    });
 
     connect(&m_runner, &ValgrindRunner::extraProcessFinished, this, [this] {
         triggerParse();
-        m_controller.setValgrindProcess(nullptr);
     });
+
+    m_controller.setValgrindRunnable(runnable());
 }
 
 QStringList CallgrindToolRunner::toolArguments() const
@@ -120,12 +124,10 @@ void CallgrindToolRunner::setPaused(bool paused)
     m_markAsPaused = paused;
 
     // call controller only if it is attached to a valgrind process
-    if (m_controller.valgrindProcess()) {
     if (paused)
         pause();
     else
         unpause();
-    }
 }
 
 void CallgrindToolRunner::setToggleCollectFunction(const QString &toggleCollectFunction)
@@ -202,3 +204,6 @@ void CallgrindToolRunner::controllerFinished(CallgrindController::Option option)
         break; // do nothing
     }
 }
+
+} // Internal
+} // Valgrind

src/plugins/valgrind/memcheckengine.cpp

@@ -86,7 +86,7 @@ MemcheckToolRunner::MemcheckToolRunner(RunControl *runControl, bool withGdb)
             this, &MemcheckToolRunner::suppressionCount);
 
     if (withGdb) {
-        connect(&m_runner, &ValgrindRunner::started,
+        connect(&m_runner, &ValgrindRunner::valgrindStarted,
                 this, &MemcheckToolRunner::startDebugger);
         connect(&m_runner, &ValgrindRunner::logMessageReceived,
                 this, &MemcheckToolRunner::appendLog);
@@ -163,10 +163,8 @@ QStringList MemcheckToolRunner::suppressionFiles() const
     return m_settings->suppressionFiles();
 }
 
-void MemcheckToolRunner::startDebugger()
+void MemcheckToolRunner::startDebugger(qint64 valgrindPid)
 {
-    const qint64 valgrindPid = m_runner.valgrindProcess()->pid();
-
     Debugger::DebuggerStartParameters sp;
     sp.inferior = runnable().as<StandardRunnable>();
     sp.startMode = Debugger::AttachToRemoteServer;

src/plugins/valgrind/memcheckengine.h

@@ -56,7 +56,7 @@ private:
     QString progressTitle() const override;
     QStringList toolArguments() const override;
 
-    void startDebugger();
+    void startDebugger(qint64 valgrindPid);
     void appendLog(const QByteArray &data);
 
     const bool m_withGdb;

src/plugins/valgrind/valgrindprocess.cpp

@@ -39,8 +39,7 @@ using namespace ProjectExplorer;
 
 namespace Valgrind {
 
-ValgrindProcess::ValgrindProcess(const IDevice::ConstPtr &device, QObject *parent)
+ValgrindProcess::ValgrindProcess()
-    : QObject(parent), m_device(device)
 {
 }
 
@@ -129,11 +128,6 @@ QProcess::ProcessError ValgrindProcess::processError() const
     return m_valgrindProcess.processError();
 }
 
-qint64 ValgrindProcess::pid() const
-{
-    return m_pid;
-}
-
 void ValgrindProcess::handleRemoteStderr(const QByteArray &b)
 {
     if (!b.isEmpty())
@@ -153,8 +147,8 @@ bool ValgrindProcess::isLocal() const
 
 void ValgrindProcess::localProcessStarted()
 {
-    m_pid = m_valgrindProcess.applicationPID().pid();
+    qint64 pid = m_valgrindProcess.applicationPID().pid();
-    emit started();
+    emit valgrindStarted(pid);
 }
 
 void ValgrindProcess::remoteProcessStarted()
@@ -196,14 +190,13 @@ void ValgrindProcess::findPIDOutputReceived(const QByteArray &out)
     if (out.isEmpty())
         return;
     bool ok;
-    m_pid = out.trimmed().toLongLong(&ok);
+    qint64 pid = out.trimmed().toLongLong(&ok);
     if (!ok) {
-        m_pid = 0;
 //        m_remote.m_errorString = tr("Could not determine remote PID.");
 //        emit ValgrindProcess::error(QProcess::FailedToStart);
 //        close();
     } else {
-        emit started();
+        emit valgrindStarted(pid);
     }
 }
 
@@ -216,6 +209,11 @@ QString ValgrindProcess::argumentString(Utils::OsType osType) const
     return arguments;
 }
 
+void ValgrindProcess::setDevice(const ProjectExplorer::IDevice::ConstPtr &device)
+{
+    m_device = device;
+}
+
 void ValgrindProcess::closed(bool success)
 {
     Q_UNUSED(success);

src/plugins/valgrind/valgrindprocess.h

@@ -45,7 +45,7 @@ class ValgrindProcess : public QObject
     Q_OBJECT
 
 public:
-    ValgrindProcess(const ProjectExplorer::IDevice::ConstPtr &device, QObject *parent);
+    ValgrindProcess();
     ~ValgrindProcess();
 
     bool isRunning() const;
@@ -64,12 +64,12 @@ public:
     QString workingDirectory() const;
 
     ProjectExplorer::IDevice::ConstPtr device() const { return m_device; }
+    void setDevice(const ProjectExplorer::IDevice::ConstPtr &device);
 
-    qint64 pid() const;
     bool isLocal() const;
 
 signals:
-    void started();
+    void valgrindStarted(qint64 pid);
     void finished(int, QProcess::ExitStatus);
     void error(QProcess::ProcessError);
     void processOutput(const QString &, Utils::OutputFormat format);
@@ -87,7 +87,6 @@ private:
 
     ProjectExplorer::StandardRunnable m_debuggee;
     ProjectExplorer::ApplicationLauncher m_valgrindProcess;
-    qint64 m_pid = 0;
     ProjectExplorer::IDevice::ConstPtr m_device;
 
     ProjectExplorer::ApplicationLauncher m_findPID;

src/plugins/valgrind/valgrindrunner.cpp

@@ -51,7 +51,7 @@ class ValgrindRunner::Private
 {
 public:
     QHostAddress localServerAddress;
-    ValgrindProcess *process = nullptr;
+    ValgrindProcess process;
     QProcess::ProcessChannelMode channelMode = QProcess::SeparateChannels;
     bool finished = false;
     QString valgrindExecutable;
@@ -75,7 +75,7 @@ ValgrindRunner::ValgrindRunner(QObject *parent)
 
 ValgrindRunner::~ValgrindRunner()
 {
-    if (d->process && d->process->isRunning()) {
+    if (d->process.isRunning()) {
         // make sure we don't delete the thread while it's still running
         waitForFinished();
     }
@@ -144,7 +144,7 @@ IDevice::ConstPtr ValgrindRunner::device() const
 
 void ValgrindRunner::waitForFinished() const
 {
-    if (d->finished || !d->process)
+    if (d->finished)
         return;
 
     QEventLoop loop;
@@ -165,26 +165,24 @@ bool ValgrindRunner::start()
         setValgrindArguments(memcheckLogArguments() + valgrindArguments());
     }
 
-    d->process = new ValgrindProcess(d->device, this);
+    d->process.setProcessChannelMode(d->channelMode);
-    d->process->setProcessChannelMode(d->channelMode);
+    d->process.setDevice(d->device);
     // consider appending our options last so they override any interfering user-supplied options
     // -q as suggested by valgrind manual
-    d->process->setValgrindExecutable(d->valgrindExecutable);
+    d->process.setValgrindExecutable(d->valgrindExecutable);
-    d->process->setValgrindArguments(fullValgrindArguments());
+    d->process.setValgrindArguments(fullValgrindArguments());
-    d->process->setDebuggee(d->debuggee);
+    d->process.setDebuggee(d->debuggee);
 
-    QObject::connect(d->process, &ValgrindProcess::processOutput,
+    QObject::connect(&d->process, &ValgrindProcess::processOutput,
                      this, &ValgrindRunner::processOutputReceived);
-    QObject::connect(d->process, &ValgrindProcess::started,
+    QObject::connect(&d->process, &ValgrindProcess::valgrindStarted,
-                     this, &ValgrindRunner::started);
+                     this, &ValgrindRunner::onValgrindStarted);
-    QObject::connect(d->process, &ValgrindProcess::finished,
+    QObject::connect(&d->process, &ValgrindProcess::finished,
                      this, &ValgrindRunner::processFinished);
-    QObject::connect(d->process, &ValgrindProcess::error,
+    QObject::connect(&d->process, &ValgrindProcess::error,
                      this, &ValgrindRunner::processError);
 
-    d->process->run(d->debuggee.runMode);
+    d->process.run(d->debuggee.runMode);
-
-    emit extraStart();
 
     return true;
 }
@@ -214,23 +212,22 @@ void ValgrindRunner::processFinished(int ret, QProcess::ExitStatus status)
     emit finished();
 
     if (ret != 0 || status == QProcess::CrashExit)
-        emit processErrorReceived(errorString(), d->process->processError());
+        emit processErrorReceived(errorString(), d->process.processError());
 }
 
 QString ValgrindRunner::errorString() const
 {
-    return d->process ? d->process->errorString() : QString();
+    return d->process.errorString();
 }
 
 void ValgrindRunner::stop()
 {
-    QTC_ASSERT(d->process, finished(); return);
+    d->process.close();
-    d->process->close();
 }
 
 ValgrindProcess *ValgrindRunner::valgrindProcess() const
 {
-    return d->process;
+    return &d->process;
 }
 
 XmlProtocol::ThreadedParser *ValgrindRunner::parser() const
@@ -246,6 +243,11 @@ void ValgrindRunner::disableXml()
     d->disableXml = true;
 }
 
+void ValgrindRunner::onValgrindStarted(qint64 pid)
+{
+    emit valgrindStarted(pid);
+}
+
 void ValgrindRunner::xmlSocketConnected()
 {
     QTcpSocket *socket = d->xmlServer.nextPendingConnection();

src/plugins/valgrind/valgrindrunner.h

@@ -76,17 +76,17 @@ public:
 
 signals:
     void logMessageReceived(const QByteArray &);
-    void extraStart();
 
     void processOutputReceived(const QString &, Utils::OutputFormat);
     void processErrorReceived(const QString &, QProcess::ProcessError);
-    void started();
+    void valgrindStarted(qint64 pid);
     void finished();
     void extraProcessFinished();
 
 private:
     bool startServers();
     QStringList memcheckLogArguments() const;
+    void onValgrindStarted(qint64 pid);
 
     void processError(QProcess::ProcessError);
     void processFinished(int, QProcess::ExitStatus);