From a6e1f67937c280961f7ec23b2a002ca7d057761d Mon Sep 17 00:00:00 2001 From: Sona Kurazyan Date: Thu, 24 Sep 2020 10:48:33 +0200 Subject: [PATCH] Fix QtConcurrent algorithms to work with temporary sequences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QtConcurrent algorithms are making an internal copy of the passed sequence, to make sure it won't be destroyed before the execution is finished. However, they were using iterators of the originally passed sequence. So, if the original sequence is deleted, QtConcurrent algorithms would use invalid iterators to a deleted sequence. This might work with Qt containers thanks to implicit-sharing, but with other containers will lead to unexpected results. Fixed them to work on the internal copy of the original sequence. Change-Id: I1d68692ed9746223c85f51bb05977bc1443b681d Reviewed-by: Andreas Buhr Reviewed-by: Mårten Nordheim Reviewed-by: Karsten Heimrich --- src/concurrent/qtconcurrentfunctionwrappers.h | 7 +++ src/concurrent/qtconcurrentmapkernel.h | 9 ++-- src/concurrent/qtconcurrentreducekernel.h | 35 +++++-------- .../tst_qtconcurrentfilter.cpp | 51 +++++++++++++++++++ .../qtconcurrentmap/tst_qtconcurrentmap.cpp | 51 +++++++++++++++++++ 5 files changed, 127 insertions(+), 26 deletions(-) diff --git a/src/concurrent/qtconcurrentfunctionwrappers.h b/src/concurrent/qtconcurrentfunctionwrappers.h index 34bbe89c860..b66f9c14b48 100644 --- a/src/concurrent/qtconcurrentfunctionwrappers.h +++ b/src/concurrent/qtconcurrentfunctionwrappers.h @@ -164,6 +164,13 @@ struct MapSequenceResultType, MapFunctor> #endif // QT_NO_TEMPLATE_TEMPLATE_PARAMETER +template +struct SequenceHolder +{ + SequenceHolder(const Sequence &s) : sequence(s) { } + Sequence sequence; +}; + } // namespace QtPrivate. diff --git a/src/concurrent/qtconcurrentmapkernel.h b/src/concurrent/qtconcurrentmapkernel.h index 11be2626288..0e6cfa4b9a4 100644 --- a/src/concurrent/qtconcurrentmapkernel.h +++ b/src/concurrent/qtconcurrentmapkernel.h @@ -219,20 +219,19 @@ inline ThreadEngineStarter startMapped(QThreadPool *pool, Iterator begin, sequence we are working on. */ template -struct SequenceHolder1 : public Base +struct SequenceHolder1 : private QtPrivate::SequenceHolder, public Base { SequenceHolder1(QThreadPool *pool, const Sequence &_sequence, Functor functor) - : Base(pool, _sequence.begin(), _sequence.end(), functor), sequence(_sequence) + : QtPrivate::SequenceHolder(_sequence), + Base(pool, this->sequence.cbegin(), this->sequence.cend(), functor) { } - Sequence sequence; - void finish() override { Base::finish(); // Clear the sequence to make sure all temporaries are destroyed // before finished is signaled. - sequence = Sequence(); + this->sequence = Sequence(); } }; diff --git a/src/concurrent/qtconcurrentreducekernel.h b/src/concurrent/qtconcurrentreducekernel.h index 694ac2b5a9c..333d358f189 100644 --- a/src/concurrent/qtconcurrentreducekernel.h +++ b/src/concurrent/qtconcurrentreducekernel.h @@ -41,6 +41,7 @@ #define QTCONCURRENT_REDUCEKERNEL_H #include +#include #if !defined(QT_NO_CONCURRENT) || defined(Q_CLANG_QDOC) @@ -222,37 +223,29 @@ public: }; template -struct SequenceHolder2 : public Base +struct SequenceHolder2 : private QtPrivate::SequenceHolder, public Base { - SequenceHolder2(QThreadPool *pool, - const Sequence &_sequence, - Functor1 functor1, - Functor2 functor2, - ReduceOptions reduceOptions) - : Base(pool, _sequence.begin(), _sequence.end(), functor1, functor2, reduceOptions), - sequence(_sequence) + SequenceHolder2(QThreadPool *pool, const Sequence &_sequence, Functor1 functor1, + Functor2 functor2, ReduceOptions reduceOptions) + : QtPrivate::SequenceHolder(_sequence), + Base(pool, this->sequence.cbegin(), this->sequence.cend(), functor1, functor2, + reduceOptions) { } - template - SequenceHolder2(QThreadPool *pool, - const Sequence &_sequence, - Functor1 functor1, - Functor2 functor2, - InitialValueType &&initialValue, - ReduceOptions reduceOptions) - : Base(pool, _sequence.begin(), _sequence.end(), functor1, functor2, - std::forward(initialValue), reduceOptions), - sequence(_sequence) + template + SequenceHolder2(QThreadPool *pool, const Sequence &_sequence, Functor1 functor1, + Functor2 functor2, InitialValueType &&initialValue, ReduceOptions reduceOptions) + : QtPrivate::SequenceHolder(_sequence), + Base(pool, this->sequence.cbegin(), this->sequence.cend(), functor1, functor2, + std::forward(initialValue), reduceOptions) { } - Sequence sequence; - void finish() override { Base::finish(); // Clear the sequence to make sure all temporaries are destroyed // before finished is signaled. - sequence = Sequence(); + this->sequence = Sequence(); } }; diff --git a/tests/auto/concurrent/qtconcurrentfilter/tst_qtconcurrentfilter.cpp b/tests/auto/concurrent/qtconcurrentfilter/tst_qtconcurrentfilter.cpp index 945f286c9d1..a2c5c207400 100644 --- a/tests/auto/concurrent/qtconcurrentfilter/tst_qtconcurrentfilter.cpp +++ b/tests/auto/concurrent/qtconcurrentfilter/tst_qtconcurrentfilter.cpp @@ -221,6 +221,13 @@ void tst_QtConcurrentFilter::filtered() CHECK_FAIL("member"); testFiltered(intList, intListEven, lambdaIsEven); CHECK_FAIL("lambda"); + + // rvalue sequences + auto future = QtConcurrent::filtered(std::vector { 1, 2, 3, 4 }, keepEvenIntegers); + QCOMPARE(future.results(), QList({ 2, 4 })); + + auto result = QtConcurrent::blockingFiltered(std::vector { 1, 2, 3, 4 }, keepEvenIntegers); + QCOMPARE(result, std::vector({ 2, 4 })); } template ({ 2, 4 })); + + auto result = + QtConcurrent::blockingFiltered(&pool, std::vector { 1, 2, 3, 4 }, keepEvenIntegers); + QCOMPARE(result, std::vector({ 2, 4 })); } template ({ 2, 4, 6 })); + + auto result = + QtConcurrent::blockingMapped>(std::vector { 1, 2, 3 }, multiplyBy2); + QCOMPARE(result, std::vector({ 2, 4, 6 })); } static QSemaphore semaphore(1); @@ -531,6 +539,14 @@ void tst_QtConcurrentMap::mappedThreadPool() CHECK_FAIL("function"); testMappedThreadPool(&pool, intList, intListMultipiedBy3, lambdaMultiplyBy3); CHECK_FAIL("lambda"); + + // rvalue sequences + auto future = QtConcurrent::mapped(&pool, std::vector { 1, 2, 3 }, multiplyBy2); + QCOMPARE(future.results(), QList({ 2, 4, 6 })); + + auto result = QtConcurrent::blockingMapped>(&pool, std::vector { 1, 2, 3 }, + multiplyBy2); + QCOMPARE(result, std::vector({ 2, 4, 6 })); } int intSquare(int x) @@ -657,6 +673,14 @@ void tst_QtConcurrentMap::mappedReduced() CHECK_FAIL("lambda-member"); testMappedReduced(intList, sumOfSquares, lambdaSquare, lambdaSumReduce); CHECK_FAIL("lambda-lambda"); + + // rvalue sequences + auto future = QtConcurrent::mappedReduced(std::vector { 1, 2, 3 }, intSquare, intSumReduce); + QCOMPARE(future, sumOfSquares); + + auto result = + QtConcurrent::blockingMappedReduced(std::vector { 1, 2, 3 }, intSquare, intSumReduce); + QCOMPARE(result, sumOfSquares); } template @@ -743,6 +767,15 @@ void tst_QtConcurrentMap::mappedReducedThreadPool() CHECK_FAIL("lambda-function"); testMappedReducedThreadPool(&pool, intList, sumOfCubes, lambdaCube, lambdaSumReduce); CHECK_FAIL("lambda-lambda"); + + // rvalue sequences + auto future = + QtConcurrent::mappedReduced(&pool, std::vector { 1, 2, 3 }, intCube, intSumReduce); + QCOMPARE(future, sumOfCubes); + + auto result = QtConcurrent::blockingMappedReduced(&pool, std::vector { 1, 2, 3 }, intCube, + intSumReduce); + QCOMPARE(result, sumOfCubes); } void tst_QtConcurrentMap::mappedReducedDifferentType() @@ -905,6 +938,15 @@ void tst_QtConcurrentMap::mappedReducedInitialValue() CHECK_FAIL("lambda-member"); testMappedReducedInitialValue(intList, sumOfSquares, lambdaSquare, lambdaSumReduce, intInitial); CHECK_FAIL("lambda-lambda"); + + // rvalue sequences + auto future = QtConcurrent::mappedReduced(std::vector { 1, 2, 3 }, intSquare, intSumReduce, + intInitial); + QCOMPARE(future, sumOfSquares); + + auto result = QtConcurrent::blockingMappedReduced(std::vector { 1, 2, 3 }, intSquare, + intSumReduce, intInitial); + QCOMPARE(result, sumOfSquares); } template @@ -990,6 +1032,15 @@ void tst_QtConcurrentMap::mappedReducedInitialValueThreadPool() testMappedReducedInitialValueThreadPool(&pool, intList, sumOfCubes, lambdaCube, lambdaSumReduce, intInitial); CHECK_FAIL("lambda-lambda"); + + // rvalue sequences + auto future = QtConcurrent::mappedReduced(&pool, std::vector { 1, 2, 3 }, intCube, intSumReduce, + intInitial); + QCOMPARE(future, sumOfCubes); + + auto result = QtConcurrent::blockingMappedReduced(&pool, std::vector { 1, 2, 3 }, intCube, + intSumReduce, intInitial); + QCOMPARE(result, sumOfCubes); } void tst_QtConcurrentMap::mappedReducedDifferentTypeInitialValue()