Commit 35c971ad authored by Milian Wolff's avatar Milian Wolff
Browse files

Fix the ownership and memory leaks of MI::MICommand

The test_midbus and test_gdb reported lots of memory leaks for these
commands because we passed around raw pointers and only sometimes
deleted them correctly.

Now we use std::unique_ptr and pass those along, which guarantees
we don't leak them ever.
parent 1256df11
Pipeline #202369 canceled with stage
in 6 minutes and 39 seconds
......@@ -12,16 +12,10 @@
using namespace KDevMI::MI;
CommandQueue::CommandQueue()
{
}
CommandQueue::~CommandQueue()
{
qDeleteAll(m_commandList);
}
CommandQueue::CommandQueue() = default;
CommandQueue::~CommandQueue() = default;
void CommandQueue::enqueue(MICommand* command)
void CommandQueue::enqueue(std::unique_ptr<MICommand> command)
{
++m_tokenCounter;
if (m_tokenCounter == 0)
......@@ -31,20 +25,20 @@ void CommandQueue::enqueue(MICommand* command)
// take the time when this command was added to the command queue
command->markAsEnqueued();
m_commandList.append(command);
if (command->flags() & (CmdImmediately | CmdInterrupt))
++m_immediatelyCounter;
rationalizeQueue(command);
m_commandList.push_back(std::move(command));
rationalizeQueue(m_commandList.back().get());
dumpQueue();
}
void CommandQueue::dumpQueue() const
{
qCDebug(DEBUGGERCOMMON) << "Pending commands" << m_commandList.count();
qCDebug(DEBUGGERCOMMON) << "Pending commands" << m_commandList.size();
unsigned commandNum = 0;
for (const MICommand* command : m_commandList) {
for (const auto& command : m_commandList) {
qCDebug(DEBUGGERCOMMON) << "Command" << commandNum << command->initialString();
++commandNum;
}
......@@ -54,60 +48,38 @@ void CommandQueue::rationalizeQueue(MICommand* command)
{
if ((command->type() >= ExecAbort && command->type() <= ExecUntil) &&
command->type() != ExecArguments ) {
// Changing execution location, abort any variable updates
removeVariableUpdates();
// ... and stack list updates
removeStackListUpdates();
}
}
void CommandQueue::removeVariableUpdates()
{
QMutableListIterator<MICommand*> it = m_commandList;
while (it.hasNext()) {
MICommand* command = it.next();
CommandType type = command->type();
if ((type >= VarEvaluateExpression && type <= VarListChildren) || type == VarUpdate) {
if (command->flags() & (CmdImmediately | CmdInterrupt))
--m_immediatelyCounter;
it.remove();
delete command;
}
}
}
void CommandQueue::removeStackListUpdates()
{
QMutableListIterator<MICommand*> it = m_commandList;
while (it.hasNext()) {
MICommand* command = it.next();
CommandType type = command->type();
if (type >= StackListArguments && type <= StackListLocals) {
if (command->flags() & (CmdImmediately | CmdInterrupt))
--m_immediatelyCounter;
it.remove();
delete command;
}
// Changing execution location, abort any variable updates and stack list updates
auto predicate = [this](const auto& command) {
const auto type = command->type();
const auto isVariableUpdate
= (type >= VarEvaluateExpression && type <= VarListChildren) || type == VarUpdate;
const auto isStackListUpdate = type >= StackListArguments && type <= StackListLocals;
if (isVariableUpdate || isStackListUpdate) {
if (command->flags() & (CmdImmediately | CmdInterrupt))
--m_immediatelyCounter;
return true;
}
return false;
};
m_commandList.erase(std::remove_if(m_commandList.begin(), m_commandList.end(), predicate), m_commandList.end());
}
}
void CommandQueue::clear()
{
qDeleteAll(m_commandList);
m_commandList.clear();
m_immediatelyCounter = 0;
}
int CommandQueue::count() const
{
return m_commandList.count();
return m_commandList.size();
}
bool CommandQueue::isEmpty() const
{
return m_commandList.isEmpty();
return m_commandList.empty();
}
bool CommandQueue::haveImmediateCommand() const
......@@ -115,12 +87,13 @@ bool CommandQueue::haveImmediateCommand() const
return m_immediatelyCounter > 0;
}
MICommand* CommandQueue::nextCommand()
std::unique_ptr<MICommand> CommandQueue::nextCommand()
{
if (m_commandList.isEmpty())
return nullptr;
if (m_commandList.empty())
return {};
MICommand* command = m_commandList.takeAt(0);
auto command = std::exchange(m_commandList.front(), {});
m_commandList.pop_front();
if (command->flags() & (CmdImmediately | CmdInterrupt))
--m_immediatelyCounter;
......
......@@ -9,7 +9,8 @@
#include "dbgglobal.h"
#include <QList>
#include <memory>
#include <deque>
namespace KDevMI { namespace MI {
......@@ -21,8 +22,7 @@ public:
CommandQueue();
~CommandQueue();
/// CommandQueue takes ownership of @p command.
void enqueue(MICommand* command);
void enqueue(std::unique_ptr<MICommand> command);
bool isEmpty() const;
int count() const;
......@@ -33,21 +33,18 @@ public:
/**
* Retrieve and remove the next command from the list.
* Ownership of the command is transferred to the caller.
* Returns @c nullptr if the list is empty.
*/
MICommand* nextCommand();
std::unique_ptr<MICommand> nextCommand();
private:
void rationalizeQueue(MICommand* command);
void removeVariableUpdates();
void removeStackListUpdates();
void dumpQueue() const;
private:
Q_DISABLE_COPY(CommandQueue)
QList<MICommand*> m_commandList;
std::deque<std::unique_ptr<MICommand>> m_commandList;
int m_immediatelyCounter = 0;
uint32_t m_tokenCounter = 0;
};
......
......@@ -64,9 +64,9 @@ MIDebugger::~MIDebugger()
}
}
void MIDebugger::execute(MICommand* command)
void MIDebugger::execute(std::unique_ptr<MICommand> command)
{
m_currentCmd = command;
m_currentCmd = std::move(command);
QString commandText = m_currentCmd->cmdToSend();
qCDebug(DEBUGGERCOMMON) << "SEND:" << commandText.trimmed();
......@@ -74,7 +74,7 @@ void MIDebugger::execute(MICommand* command)
QByteArray commandUtf8 = commandText.toUtf8();
m_process->write(commandUtf8);
command->markAsSubmitted();
m_currentCmd->markAsSubmitted();
QString prettyCmd = m_currentCmd->cmdToSend();
prettyCmd.remove(QRegExp(QStringLiteral("set prompt \032.\n")));
......@@ -111,7 +111,7 @@ void MIDebugger::interrupt()
MICommand* MIDebugger::currentCommand() const
{
return m_currentCmd;
return m_currentCmd.get();
}
void MIDebugger::kill()
......@@ -250,8 +250,7 @@ void MIDebugger::processLine(const QByteArray& line)
qCDebug(DEBUGGERCOMMON) << "Unhandled result code: " << result.reason;
}
delete m_currentCmd;
m_currentCmd = nullptr;
m_currentCmd.reset();
emit ready();
break;
}
......
......@@ -16,6 +16,8 @@
#include <QByteArray>
#include <QObject>
#include <memory>
class KConfigGroup;
class KProcess;
......@@ -43,7 +45,7 @@ public:
for 'ready' as well.
The ownership of 'command' is transferred to the debugger. */
void execute(MI::MICommand* command);
void execute(std::unique_ptr<MI::MICommand> command);
/** Returns true if 'execute' can be called immediately. */
bool isReady() const;
......@@ -123,7 +125,7 @@ protected:
QString m_debuggerExecutable;
KProcess* m_process = nullptr;
MI::MICommand* m_currentCmd = nullptr;
std::unique_ptr<MI::MICommand> m_currentCmd;
MI::MIParser m_parser;
/** The unprocessed output from debugger. Output is
......
......@@ -307,8 +307,7 @@ bool MIDebugSession::attachToProcess(int pid)
this, &MIDebugSession::handleTargetAttach,
CmdHandlesError);
addCommand(new SentinelCommand(breakpointController(),
&MIBreakpointController::initSendBreakpoints));
addCommand(std::make_unique<SentinelCommand>(breakpointController(), &MIBreakpointController::initSendBreakpoints));
raiseEvent(connected_to_program);
......@@ -672,7 +671,7 @@ void MIDebugSession::addUserCommand(const QString& cmd)
if (!usercmd)
return;
queueCmd(usercmd);
queueCmd(std::move(usercmd));
// User command can theoretically modify absolutely everything,
// so need to force a reload.
......@@ -683,28 +682,27 @@ void MIDebugSession::addUserCommand(const QString& cmd)
raiseEvent(program_state_changed);
}
MICommand *MIDebugSession::createUserCommand(const QString &cmd) const
std::unique_ptr<MICommand> MIDebugSession::createUserCommand(const QString& cmd) const
{
MICommand *res = nullptr;
if (!cmd.isEmpty() && cmd[0].isDigit()) {
// Add a space to the beginning, so debugger won't get confused if the
// command starts with a number (won't mix it up with command token added)
res = new UserCommand(MI::NonMI, QLatin1Char(' ') + cmd);
return std::make_unique<UserCommand>(MI::NonMI, QLatin1Char(' ') + cmd);
} else {
res = new UserCommand(MI::NonMI, cmd);
return std::make_unique<UserCommand>(MI::NonMI, cmd);
}
return res;
}
MICommand *MIDebugSession::createCommand(CommandType type, const QString& arguments,
CommandFlags flags) const
std::unique_ptr<MICommand> MIDebugSession::createCommand(CommandType type, const QString& arguments,
CommandFlags flags) const
{
return new MICommand(type, arguments, flags);
// uses protected ctor, only accessible to MIDebugSession via friendship - cannot use make_unique :(
return std::unique_ptr<MICommand>(new MICommand(type, arguments, flags));
}
void MIDebugSession::addCommand(MICommand* cmd)
void MIDebugSession::addCommand(std::unique_ptr<MICommand> cmd)
{
queueCmd(cmd);
queueCmd(std::move(cmd));
}
void MIDebugSession::addCommand(MI::CommandType type, const QString& arguments, MI::CommandFlags flags)
......@@ -718,7 +716,7 @@ void MIDebugSession::addCommand(MI::CommandType type, const QString& arguments,
{
auto cmd = createCommand(type, arguments, flags);
cmd->setHandler(handler);
queueCmd(cmd);
queueCmd(std::move(cmd));
}
void MIDebugSession::addCommand(MI::CommandType type, const QString& arguments,
......@@ -727,7 +725,7 @@ void MIDebugSession::addCommand(MI::CommandType type, const QString& arguments,
{
auto cmd = createCommand(type, arguments, flags);
cmd->setHandler(callback);
queueCmd(cmd);
queueCmd(std::move(cmd));
}
// Fairly obvious that we'll add whatever command you give me to a queue
......@@ -735,7 +733,7 @@ void MIDebugSession::addCommand(MI::CommandType type, const QString& arguments,
// information requests become redundent and must be removed.
// We also try and run whatever command happens to be at the head of
// the queue.
void MIDebugSession::queueCmd(MICommand *cmd)
void MIDebugSession::queueCmd(std::unique_ptr<MICommand> cmd)
{
if (debuggerStateIsOn(s_dbgNotStarted)) {
const QString messageText =
......@@ -749,8 +747,6 @@ void MIDebugSession::queueCmd(MICommand *cmd)
if (m_stateReloadInProgress)
cmd->setStateReloading(true);
m_commandQueue->enqueue(cmd);
qCDebug(DEBUGGERCOMMON) << "QUEUE: " << cmd->initialString()
<< (m_stateReloadInProgress ? "(state reloading)" : "")
<< m_commandQueue->count() << "pending";
......@@ -770,6 +766,8 @@ void MIDebugSession::queueCmd(MICommand *cmd)
qCDebug(DEBUGGERCOMMON) << "\t--frame will be added on execution";
}
m_commandQueue->enqueue(std::move(cmd));
setDebuggerStateOn(s_dbgBusy);
raiseEvent(debugger_busy);
......@@ -790,7 +788,7 @@ void MIDebugSession::executeCmd()
if (!m_debugger->isReady())
return;
MICommand* currentCmd = m_commandQueue->nextCommand();
auto currentCmd = m_commandQueue->nextCommand();
if (!currentCmd)
return;
......@@ -833,18 +831,14 @@ void MIDebugSession::executeCmd()
if (length == 0) {
// The command might decide it's no longer necessary to send
// it.
if (auto* sc = dynamic_cast<SentinelCommand*>(currentCmd))
{
if (auto* sc = dynamic_cast<SentinelCommand*>(currentCmd.get())) {
qCDebug(DEBUGGERCOMMON) << "SEND: sentinel command, not sending";
sc->invokeHandler();
}
else
{
} else {
qCDebug(DEBUGGERCOMMON) << "SEND: command " << currentCmd->initialString()
<< "changed its mind, not sending";
}
delete currentCmd;
executeCmd();
return;
} else {
......@@ -862,7 +856,7 @@ void MIDebugSession::executeCmd()
return;
}
m_debugger->execute(currentCmd);
m_debugger->execute(std::move(currentCmd));
}
void MIDebugSession::ensureDebuggerListening()
......
......@@ -172,9 +172,9 @@ public Q_SLOTS:
bool attachToProcess(int pid);
public:
virtual MI::MICommand *createCommand(MI::CommandType type, const QString& arguments,
MI::CommandFlags flags = {}) const;
virtual MI::MICommand *createUserCommand(const QString &cmd) const;
virtual std::unique_ptr<MI::MICommand> createCommand(MI::CommandType type, const QString& arguments,
MI::CommandFlags flags = {}) const;
virtual std::unique_ptr<MI::MICommand> createUserCommand(const QString& cmd) const;
/** Adds a command to the end of queue of commands to be executed
by debugger. The command will be actually sent to debugger only when
replies from all previous commands are received and full processed.
......@@ -186,7 +186,7 @@ public:
*/
void addUserCommand(const QString &cmd);
void addCommand(MI::MICommand* cmd);
void addCommand(std::unique_ptr<MI::MICommand> cmd);
/** Same as above, but internally constructs MICommand using createCommand() */
void addCommand(MI::CommandType type, const QString& arguments = QString(),
......@@ -239,7 +239,7 @@ protected Q_SLOTS:
void handleInferiorFinished(const QString &msg);
protected:
void queueCmd(MI::MICommand *cmd);
void queueCmd(std::unique_ptr<MI::MICommand> cmd);
/** Try to execute next command in the queue. If GDB is not
busy with previous command, and there's a command in the
......@@ -356,7 +356,7 @@ void MIDebugSession::addCommand(MI::CommandType type, const QString& arguments,
{
auto cmd = createCommand(type, arguments, flags);
cmd->setHandler(handler_this, handler_method);
queueCmd(cmd);
queueCmd(std::move(cmd));
}
} // end of namespace KDevMI
......
......@@ -135,8 +135,8 @@ void MIFrameStackModel::fetchFrames(int threadNumber, int from, int to)
{
//to+1 so we know if there are more
QString arg = QStringLiteral("%1 %2").arg(from).arg(to+1);
MICommand *c = session()->createCommand(StackListFrames, arg);
auto c = session()->createCommand(StackListFrames, arg);
c->setHandler(new FrameListHandler(this, threadNumber, to));
c->setThread(threadNumber);
session()->addCommand(c);
session()->addCommand(std::move(c));
}
......@@ -50,14 +50,14 @@ void TestMICommandQueue::testDestructor()
auto* commandQueue = new KDevMI::MI::CommandQueue;
// prepare
auto* command1 = new TestDummyCommand(KDevMI::MI::NonMI, QString(), KDevMI::MI::CmdImmediately);
auto* command2 = new TestDummyCommand(KDevMI::MI::NonMI, QString(), {});
auto command1 = std::make_unique<TestDummyCommand>(KDevMI::MI::NonMI, QString(), KDevMI::MI::CmdImmediately);
auto command2 = std::make_unique<TestDummyCommand>(KDevMI::MI::NonMI, QString(), KDevMI::MI::CommandFlags {});
QSignalSpy command1Spy(command1, &QObject::destroyed);
QSignalSpy command2Spy(command2, &QObject::destroyed);
QSignalSpy command1Spy(command1.get(), &QObject::destroyed);
QSignalSpy command2Spy(command2.get(), &QObject::destroyed);
commandQueue->enqueue(command1);
commandQueue->enqueue(command2);
commandQueue->enqueue(std::move(command1));
commandQueue->enqueue(std::move(command2));
// execute
delete commandQueue;
......@@ -90,19 +90,20 @@ void TestMICommandQueue::addAndTake()
KDevMI::MI::CommandQueue commandQueue;
auto command = std::make_unique<TestDummyCommand>(KDevMI::MI::NonMI, QString(), flags);
auto c = command.get();
// add
commandQueue.enqueue(command.get());
commandQueue.enqueue(std::move(command));
// check
QVERIFY(command->token() != 0);
QVERIFY(c->token() != 0);
QCOMPARE(commandQueue.count(), 1);
QCOMPARE(commandQueue.isEmpty(), false);
QCOMPARE(commandQueue.haveImmediateCommand(), isImmediate);
// take
auto* nextCommand = commandQueue.nextCommand();
auto nextCommand = commandQueue.nextCommand();
// check
QCOMPARE(nextCommand, command.get());
QCOMPARE(nextCommand.get(), c);
QVERIFY(nextCommand->token() != 0);
QCOMPARE(commandQueue.count(), 0);
QCOMPARE(commandQueue.isEmpty(), true);
......@@ -114,14 +115,14 @@ void TestMICommandQueue::clearQueue()
KDevMI::MI::CommandQueue commandQueue;
// prepare
auto* command1 = new TestDummyCommand(KDevMI::MI::NonMI, QString(), KDevMI::MI::CmdImmediately);
auto* command2 = new TestDummyCommand(KDevMI::MI::NonMI, QString(), {});
auto command1 = std::make_unique<TestDummyCommand>(KDevMI::MI::NonMI, QString(), KDevMI::MI::CmdImmediately);
auto command2 = std::make_unique<TestDummyCommand>(KDevMI::MI::NonMI, QString(), KDevMI::MI::CommandFlags {});
QSignalSpy command1Spy(command1, &QObject::destroyed);
QSignalSpy command2Spy(command2, &QObject::destroyed);
QSignalSpy command1Spy(command1.get(), &QObject::destroyed);
QSignalSpy command2Spy(command2.get(), &QObject::destroyed);
commandQueue.enqueue(command1);
commandQueue.enqueue(command2);
commandQueue.enqueue(std::move(command1));
commandQueue.enqueue(std::move(command2));
// execute
commandQueue.clear();
......
......@@ -88,7 +88,8 @@ void DebugSession::initializeDebugger()
{
//addCommand(new GDBCommand(GDBMI::EnableTimings, "yes"));
addCommand(new CliCommand(MI::GdbShow, QStringLiteral("version"), this, &DebugSession::handleVersion));
addCommand(
std::make_unique<CliCommand>(MI::GdbShow, QStringLiteral("version"), this, &DebugSession::handleVersion));
// This makes gdb pump a variable out on one line.
addCommand(MI::GdbSet, QStringLiteral("width 0"));
......@@ -219,19 +220,20 @@ bool DebugSession::execInferior(KDevelop::ILaunchConfiguration *cfg, IExecutePlu
// Future: the shell script should be able to pass info (like pid)
// to the gdb script...
addCommand(new SentinelCommand([this, runGdbScript]() {
breakpointController()->initSendBreakpoints();
addCommand(std::make_unique<SentinelCommand>(
[this, runGdbScript]() {
breakpointController()->initSendBreakpoints();
breakpointController()->setDeleteDuplicateBreakpoints(true);
qCDebug(DEBUGGERGDB) << "Running gdb script " << KShell::quoteArg(runGdbScript.toLocalFile());
breakpointController()->setDeleteDuplicateBreakpoints(true);
qCDebug(DEBUGGERGDB) << "Running gdb script " << KShell::quoteArg(runGdbScript.toLocalFile());
addCommand(MI::NonMI, QLatin1String("source ") + runGdbScript.toLocalFile(),
[this](const MI::ResultRecord&) {
breakpointController()->setDeleteDuplicateBreakpoints(false);
},
CmdMaybeStartsRunning);
raiseEvent(connected_to_program);
}, CmdMaybeStartsRunning));
addCommand(
MI::NonMI, QLatin1String("source ") + runGdbScript.toLocalFile(),
[this](const MI::ResultRecord&) { breakpointController()->setDeleteDuplicateBreakpoints(false); },
CmdMaybeStartsRunning);
raiseEvent(connected_to_program);
},
CmdMaybeStartsRunning));
} else {
// normal local debugging
addCommand(MI::FileExecAndSymbols, KShell::quoteArg(executable),
......@@ -239,10 +241,12 @@ bool DebugSession::execInferior(KDevelop::ILaunchConfiguration *cfg, IExecutePlu
CmdHandlesError);
raiseEvent(connected_to_program);
addCommand(new SentinelCommand([this]() {
breakpointController()->initSendBreakpoints();
addCommand(MI::ExecRun, QString(), CmdMaybeStartsRunning);
}, CmdMaybeStartsRunning));
addCommand(std::make_unique<SentinelCommand>(
[this]() {
breakpointController()->initSendBreakpoints();
addCommand(MI::ExecRun, QString(), CmdMaybeStartsRunning);
},
CmdMaybeStartsRunning));
}
return true;
}
......
......@@ -194,7 +194,7 @@ void MemoryView::slotChangeMemoryRange()
if(amount.isEmpty())
amount = QStringLiteral("sizeof(%1)").arg(m_rangeSelector->startAddressLineEdit->text());
session->addCommand(new MI::ExpressionValueCommand(amount, this, &MemoryView::sizeComputed));
session->addCommand(std::make_unique<MI::ExpressionValueCommand>(amount, this, &MemoryView::sizeComputed));
}
void MemoryView::sizeComputed(const QString& size)
......
......@@ -318,7 +318,7 @@ void GdbTest::testUpdateBreakpoint()
// breakpoint 2: real line 32: const char *x = "Hello";
//insert custom command as user might do it using GDB console
session->addCommand(new MI::UserCommand(MI::NonMI, "break "+debugeeFileName+":32"));
session->addCommand(std::make_unique<MI::UserCommand>(MI::NonMI, "break " + debugeeFileName + ":32"));
WAIT_FOR_STATE_AND_IDLE(session, DebugSession::PausedState); // stop at breakpoint 1, with custom command handled
QCOMPARE(session->currentLine(), 28);
......@@ -1729,8 +1729,8 @@ void GdbTest::testThreadAndFrameInfo()
QSignalSpy outputSpy(session, &TestDebugSession::debuggerUserCommandOutput);
session->addCommand(new MI::UserCommand(MI::ThreadInfo, QString()));
session->addCommand(new MI::UserCommand(MI::StackListLocals, QStringLiteral("0")));
session->addCommand(std::make_unique<MI::UserCommand>(MI::ThreadInfo, QString()));
session->addCommand(std::make_unique<MI::UserCommand>(MI::StackListLocals, QStringLiteral("0")));
WAIT_FOR_STATE_AND_IDLE(session, DebugSession::PausedState); // wait for command finish