Valgrind: Don't leak ParseData

Use shared pointer for ParseData.

Change-Id: Ief46feac2e7902405724b56b89c4acea5db411cd
Reviewed-by: hjk <hjk@qt.io>

src/plugins/valgrind/callgrind/callgrindcallmodel.cpp

@@ -5,7 +5,6 @@
 
 #include "callgrindfunctioncall.h"
 #include "callgrindfunction.h"
-#include "callgrindparsedata.h"
 #include "../valgrindtr.h"
 
 #include <utils/qtcassert.h>
@@ -17,7 +16,7 @@ namespace Valgrind::Callgrind {
 class CallModel::Private
 {
 public:
-    const ParseData *m_data = nullptr;
+    ParseDataPtr m_data = nullptr;
     QList<const FunctionCall *> m_calls;
     int m_event = 0;
     const Function *m_function = nullptr;
@@ -69,7 +68,7 @@ int CallModel::costEvent() const
     return d->m_event;
 }
 
-void CallModel::setParseData(const ParseData *data)
+void CallModel::setParseData(const ParseDataPtr &data)
 {
     if (d->m_data == data)
         return;
@@ -80,7 +79,7 @@ void CallModel::setParseData(const ParseData *data)
     d->m_data = data;
 }
 
-const ParseData *CallModel::parseData() const
+ParseDataPtr CallModel::parseData() const
 {
     return d->m_data;
 }

src/plugins/valgrind/callgrind/callgrindcallmodel.h

@@ -5,6 +5,8 @@
 
 #include "callgrindabstractmodel.h"
 
+#include "callgrindparsedata.h"
+
 #include <QAbstractItemModel>
 
 namespace Valgrind::Callgrind {
@@ -29,8 +31,8 @@ public:
     int costEvent() const;
     void setCostEvent(int event);
 
-    void setParseData(const ParseData *data);
+    void setParseData(const ParseDataPtr &data);
-    const ParseData *parseData() const;
+    ParseDataPtr parseData() const;
 
     void setCalls(const QList<const FunctionCall *> &calls, const Function *function);
     QList<const FunctionCall *> calls() const;

src/plugins/valgrind/callgrind/callgrindcostitem.cpp

@@ -3,8 +3,8 @@
 
 #include "callgrindcostitem.h"
 
-#include "callgrindparsedata.h"
 #include "callgrindfunctioncall.h"
+#include "callgrindparsedata.h"
 
 #include <QStringList>
 
@@ -13,18 +13,18 @@ namespace Valgrind::Callgrind {
 class CostItem::Private
 {
 public:
-    Private(ParseData *data);
+    Private(const ParseData *data);
     ~Private();
 
     QList<quint64> m_positions;
     QList<quint64> m_events;
     const FunctionCall *m_call = nullptr;
 
-    const ParseData *m_data = nullptr;
+    const ParseData *m_data;
     qint64 m_differingFileId = -1;
 };
 
-CostItem::Private::Private(ParseData *data)
+CostItem::Private::Private(const ParseData *data)
     : m_positions(data->positions().size(), 0)
     , m_events(data->events().size(), 0)
     , m_data(data)
@@ -36,9 +36,8 @@ CostItem::Private::~Private()
     delete m_call;
 }
 
-
 //BEGIN CostItem
-CostItem::CostItem(ParseData *data)
+CostItem::CostItem(const ParseData *data)
 : d(new Private(data))
 {
 }

src/plugins/valgrind/callgrind/callgrindcostitem.h

@@ -18,7 +18,7 @@ class CostItem
 public:
     /// @p data the file data this cost item was parsed in.
     ///         required for decompression of string data like differing source file information
-    explicit CostItem(ParseData *data);
+    explicit CostItem(const ParseData *data);
     ~CostItem();
 
     /**

src/plugins/valgrind/callgrind/callgrinddatamodel.cpp

@@ -3,9 +3,8 @@
 
 #include "callgrinddatamodel.h"
 
-#include "callgrindparsedata.h"
-#include "callgrindfunction.h"
 #include "callgrindcostitem.h"
+#include "callgrindfunction.h"
 #include "../valgrindtr.h"
 
 #include <utils/algorithm.h>
@@ -21,7 +20,7 @@ class DataModel::Private
 public:
     void updateFunctions();
 
-    const ParseData *m_data = nullptr;
+    ParseDataPtr m_data;
     int m_event = 0;
     bool m_verboseToolTips = true;
     bool m_cycleDetection = false;
@@ -51,7 +50,7 @@ DataModel::~DataModel()
     delete d;
 }
 
-void DataModel::setParseData(const ParseData *data)
+void DataModel::setParseData(const ParseDataPtr &data)
 {
     if (d->m_data == data)
         return;
@@ -73,7 +72,7 @@ bool DataModel::verboseToolTipsEnabled() const
     return d->m_verboseToolTips;
 }
 
-const ParseData *DataModel::parseData() const
+ParseDataPtr DataModel::parseData() const
 {
     return d->m_data;
 }

src/plugins/valgrind/callgrind/callgrinddatamodel.h

@@ -5,12 +5,13 @@
 
 #include "callgrindabstractmodel.h"
 
+#include "callgrindparsedata.h"
+
 #include <QAbstractItemModel>
 
 namespace Valgrind::Callgrind {
 
 class Function;
-class ParseData;
 
 class DataModel : public QAbstractItemModel
 {
@@ -20,8 +21,8 @@ public:
     DataModel();
     ~DataModel() override;
 
-    void setParseData(const ParseData *data);
+    void setParseData(const ParseDataPtr &data);
-    const ParseData *parseData() const;
+    ParseDataPtr parseData() const;
 
     void setVerboseToolTipsEnabled(bool enabled);
     bool verboseToolTipsEnabled() const;

src/plugins/valgrind/callgrind/callgrindparsedata.cpp

@@ -116,8 +116,7 @@ void ParseData::Private::cycleDetection()
 
 ParseData::ParseData(const QString &fileName)
     : d(new Private(this, fileName))
-{
+{}
-}
 
 ParseData::~ParseData()
 {

src/plugins/valgrind/callgrind/callgrindparsedata.h

@@ -109,4 +109,6 @@ private:
     Private *d;
 };
 
+using ParseDataPtr = std::shared_ptr<const ParseData>;
+
 } // namespace Valgrind::Callgrind

src/plugins/valgrind/callgrind/callgrindparser.cpp

@@ -120,11 +120,6 @@ public:
     {
     }
 
-    ~Private()
-    {
-        delete data;
-    }
-
     void parse(const FilePath &filePath);
     void parseHeader(QIODevice *device);
 
@@ -145,7 +140,7 @@ public:
     int addressValuesCount = 0;
     int costValuesCount = 0;
 
-    ParseData *data = nullptr;
+    std::shared_ptr<ParseData> data;
     Function *currentFunction = nullptr;
     qint64 lastObject = -1;
     qint64 lastFile = -1;
@@ -174,17 +169,12 @@ public:
 
 void Parser::Private::parse(const FilePath &filePath)
 {
-    // be sure to clean up existing data before re-allocating
-    // the callee might not have taken the parse data
-    delete data;
-    data = nullptr;
-
     const QString path = filePath.path(); // FIXME: Works only accidentally for docker
     QFile file(path);
     if (!file.open(QIODevice::ReadOnly))
         qWarning() << "Could not open file for parsing:" << filePath.toUserOutput();
 
-    data = new ParseData(path);
+    data = std::make_shared<ParseData>(path);
     parseHeader(&file);
     while (!file.atEnd()) {
         const QByteArray line = file.readLine();
@@ -426,7 +416,7 @@ void Parser::Private::parseCostItem(const char *begin, const char *end)
     const char *current = begin;
 
     QTC_ASSERT(currentDifferingFile == -1 || currentDifferingFile != currentFunction->fileId(), return);
-    auto costItem = new CostItem(data);
+    auto costItem = new CostItem(data.get());
     costItem->setDifferingFile(currentDifferingFile);
     FunctionCall *call = nullptr;
     if (isParsingFunctionCall) {
@@ -530,7 +520,7 @@ void Parser::Private::parseSourceFile(const char *begin, const char *end)
 
 void Parser::Private::parseFunction(const char *begin, const char *end)
 {
-    currentFunction = new Function(data);
+    currentFunction = new Function(data.get());
     currentFunction->setFile(lastFile);
     currentFunction->setObject(lastObject);
 
@@ -634,11 +624,9 @@ Parser::~Parser()
     delete d;
 }
 
-ParseData *Parser::takeData()
+ParseDataPtr Parser::parserData() const
 {
-    ParseData *data = d->data;
+    return d->data;
-    d->data = nullptr;
-    return data;
 }
 
 } // namespace Valgrind::Callgrind

src/plugins/valgrind/callgrind/callgrindparser.h

@@ -3,6 +3,8 @@
 
 #pragma once
 
+#include "callgrindparsedata.h"
+
 #include <QObject>
 
 namespace Utils { class FilePath; }
@@ -30,7 +32,7 @@ public:
 
     // get and take ownership of the parsing results. If this function is not called the repository
     // will be destroyed when the parser is destroyed. Subsequent calls return null.
-    ParseData *takeData();
+    ParseDataPtr parserData() const;
     void parse(const Utils::FilePath &filePath);
 
 signals:

src/plugins/valgrind/callgrind/callgrindproxymodel.cpp

@@ -119,7 +119,7 @@ bool DataProxyModel::filterAcceptsRow(int source_row, const QModelIndex &source_
     // check minimum inclusive costs
     DataModel *model = dataModel();
     QTC_ASSERT(model, return false); // as always: this should never happen
-    const ParseData *data = model->parseData();
+    const ParseDataPtr data = model->parseData();
     QTC_ASSERT(data, return false);
     if (m_minimumInclusiveCostRatio != 0.0) {
         const quint64 totalCost = data->totalCost(0);

src/plugins/valgrind/callgrindengine.cpp

@@ -117,9 +117,9 @@ void CallgrindToolRunner::setToggleCollectFunction(const QString &toggleCollectF
     m_argumentForToggleCollect = "--toggle-collect=" + toggleCollectFunction;
 }
 
-Callgrind::ParseData *CallgrindToolRunner::takeParserData()
+Callgrind::ParseDataPtr CallgrindToolRunner::parserData() const
 {
-    return m_parser.takeData();
+    return m_parser.parserData();
 }
 
 void CallgrindToolRunner::showStatusMessage(const QString &message)

src/plugins/valgrind/callgrindengine.h

@@ -23,7 +23,7 @@ public:
 
     void start() override;
 
-    Valgrind::Callgrind::ParseData *takeParserData();
+    Callgrind::ParseDataPtr parserData() const;
 
     /// controller actions
     void dump() { run(Dump); }

src/plugins/valgrind/callgrindtool.cpp

@@ -95,10 +95,9 @@ public:
 
     void setupRunner(CallgrindToolRunner *runner);
 
-    void setParseData(ParseData *data);
     CostDelegate::CostFormat costFormat() const;
 
-    void doClear(bool clearParseData);
+    void doClear();
     void updateEventCombo();
 
 signals:
@@ -136,10 +135,11 @@ public:
     void calleeFunctionSelected(const QModelIndex &index);
     void callerFunctionSelected(const QModelIndex &index);
     void visualisationFunctionSelected(const Function *function);
-    void showParserResults(const ParseData *data);
+    void showParserResults(const ParseDataPtr &data);
 
     void takeParserDataFromRunControl(CallgrindToolRunner *rc);
-    void takeParserData(ParseData *data);
+    void setParserData(const ParseDataPtr &data);
+    void doSetParseData(const ParseDataPtr &data);
     void engineFinished();
 
     void editorOpened(IEditor *);
@@ -385,7 +385,7 @@ CallgrindTool::CallgrindTool(QObject *parent)
     action->setToolTip(Tr::tr("Discard Data"));
     connect(action, &QAction::triggered, this, [this](bool) {
         clearTextMarks();
-        doClear(true);
+        doClear();
     });
 
     // navigation
@@ -494,10 +494,9 @@ CallgrindTool::~CallgrindTool()
     delete m_visualization;
 }
 
-void CallgrindTool::doClear(bool clearParseData)
+void CallgrindTool::doClear()
 {
-    if (clearParseData) // Crashed when done from destructor.
+    doSetParseData({});
-        setParseData(nullptr);
 
     // clear filters
     if (m_filterProjectCosts)
@@ -664,25 +663,20 @@ void CallgrindTool::visualisationFunctionSelected(const Function *function)
         selectFunction(function);
 }
 
-void CallgrindTool::setParseData(ParseData *data)
+void CallgrindTool::doSetParseData(const ParseDataPtr &data)
 {
     // we have new parse data, invalidate filters in the proxy model
     if (m_visualization)
         m_visualization->setFunction(nullptr);
 
-    // invalidate parse data in the data model
+    // might happen if the user cancelled the profile run
-    delete m_dataModel.parseData();
+    // callgrind then sometimes produces empty callgrind.out.PID files
+    const ParseDataPtr newData = data && !data->events().isEmpty() ? data : ParseDataPtr();
 
-    if (data && data->events().isEmpty()) {
+    m_lastFileName = newData ? newData->fileName() : QString();
-        // might happen if the user cancelled the profile run
+    m_dataModel.setParseData(newData);
-        // callgrind then sometimes produces empty callgrind.out.PID files
+    m_calleesModel.setParseData(newData);
-        delete data;
+    m_callersModel.setParseData(newData);
-        data = nullptr;
-    }
-    m_lastFileName = data ? data->fileName() : QString();
-    m_dataModel.setParseData(data);
-    m_calleesModel.setParseData(data);
-    m_callersModel.setParseData(data);
 
     if (m_eventCombo)
         updateEventCombo();
@@ -700,7 +694,7 @@ void CallgrindTool::updateEventCombo()
 
     m_eventCombo->clear();
 
-    const ParseData *data = m_dataModel.parseData();
+    const ParseDataPtr data = m_dataModel.parseData();
     if (!data || data->events().isEmpty()) {
         m_eventCombo->hide();
         return;
@@ -749,7 +743,7 @@ void CallgrindTool::setupRunner(CallgrindToolRunner *toolRunner)
     m_dumpAction->setEnabled(true);
     m_loadExternalLogFile->setEnabled(false);
     clearTextMarks();
-    doClear(true);
+    doClear();
 }
 
 void CallgrindTool::updateRunActions()
@@ -783,7 +777,7 @@ void CallgrindTool::engineFinished()
     m_dumpAction->setEnabled(false);
     m_loadExternalLogFile->setEnabled(true);
 
-    const ParseData *data = m_dataModel.parseData();
+    const ParseDataPtr data = m_dataModel.parseData();
     if (data)
         showParserResults(data);
     else
@@ -792,7 +786,7 @@ void CallgrindTool::engineFinished()
     setBusyCursor(false);
 }
 
-void CallgrindTool::showParserResults(const ParseData *data)
+void CallgrindTool::showParserResults(const ParseDataPtr &data)
 {
     QString msg;
     if (data) {
@@ -878,15 +872,15 @@ void CallgrindTool::loadExternalLogFile()
 
     Parser parser;
     parser.parse(filePath);
-    takeParserData(parser.takeData());
+    setParserData(parser.parserData());
 }
 
 void CallgrindTool::takeParserDataFromRunControl(CallgrindToolRunner *rc)
 {
-    takeParserData(rc->takeParserData());
+    setParserData(rc->parserData());
 }
 
-void CallgrindTool::takeParserData(ParseData *data)
+void CallgrindTool::setParserData(const ParseDataPtr &data)
 {
     showParserResults(data);
 
@@ -895,9 +889,9 @@ void CallgrindTool::takeParserData(ParseData *data)
 
     // clear first
     clearTextMarks();
-    doClear(true);
+    doClear();
+    doSetParseData(data);
 
-    setParseData(data);
     const FilePath kcachegrindExecutable = globalSettings().kcachegrindExecutable();
     const FilePath found = kcachegrindExecutable.searchInPath();
     const bool kcachegrindExists = found.isExecutableFile();

tests/auto/valgrind/callgrind/callgrindparsertests.cpp

@@ -80,17 +80,16 @@ void CallgrindParserTests::cleanup()
 {
 }
 
-ParseData* parseDataFile(const QString &dataFile)
+ParseDataPtr parseDataFile(const QString &dataFile)
 {
     Parser p;
     p.parse(Utils::FilePath::fromString(dataFile));
-
+    return p.parserData();
-    return p.takeData();
 }
 
 void CallgrindParserTests::testHeaderData()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("simpleFunction.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("simpleFunction.out")));
 
     QCOMPARE(data->command(), QLatin1String("ls"));
     QCOMPARE(data->creator(), QLatin1String("callgrind-3.6.0.SVN-Debian"));
@@ -109,7 +108,7 @@ void CallgrindParserTests::testHeaderData()
 
 void CallgrindParserTests::testSimpleFunction()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("simpleFunction.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("simpleFunction.out")));
 
     QCOMPARE(data->functions().size(), 4);
 
@@ -178,7 +177,7 @@ void CallgrindParserTests::testSimpleFunction()
 
 void CallgrindParserTests::testCallee()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("calleeFunctions.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("calleeFunctions.out")));
 
     QCOMPARE(data->functions().size(), 3);
 
@@ -253,7 +252,7 @@ void CallgrindParserTests::testCallee()
 
 void CallgrindParserTests::testInlinedCalls()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("inlinedFunctions.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("inlinedFunctions.out")));
     QCOMPARE(data->functions().size(), 3);
 
     const Function *main = data->functions().first();
@@ -279,7 +278,7 @@ void CallgrindParserTests::testInlinedCalls()
 
 void CallgrindParserTests::testMultiCost()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("multiCost.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("multiCost.out")));
     QCOMPARE(data->functions().size(), 2);
 
     QCOMPARE(data->positions(), QStringList() << "line");
@@ -298,7 +297,7 @@ void CallgrindParserTests::testMultiCost()
 
 void CallgrindParserTests::testMultiPos()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("multiPos.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("multiPos.out")));
     QCOMPARE(data->functions().size(), 2);
 
     QCOMPARE(data->positions(), QStringList() << "line" << "memAddr");
@@ -316,7 +315,7 @@ void CallgrindParserTests::testMultiPos()
 
 void CallgrindParserTests::testMultiPosAndCost()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("multiCostAndPos.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("multiCostAndPos.out")));
     QCOMPARE(data->functions().size(), 2);
 
     QCOMPARE(data->positions(), QStringList() << "line" << "memAddr");
@@ -345,7 +344,7 @@ const Function *findFunction(const QString &needle, const QVector<const Function
 
 void CallgrindParserTests::testCycle()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("cycle.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("cycle.out")));
     QCOMPARE(data->functions().size(), 4);
 
     const Function *main = data->functions().at(0);
@@ -377,7 +376,7 @@ void CallgrindParserTests::testCycle()
 
 void CallgrindParserTests::testRecursiveCycle()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("recursiveCycle.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("recursiveCycle.out")));
     QCOMPARE(data->functions().size(), 5);
 
     const Function *main = findFunction(QLatin1String("main"), data->functions());
@@ -418,7 +417,7 @@ void CallgrindParserTests::testRecursiveCycle()
 
 void CallgrindParserTests::testRecursion()
 {
-    QScopedPointer<const ParseData> data(parseDataFile(dataFile("recursion.out")));
+    const ParseDataPtr data(parseDataFile(dataFile("recursion.out")));
     QCOMPARE(data->functions().size(), 3);
     QCOMPARE(data->totalCost(0), quint64(35700972));