From a0494a2092d7512a2b0d568a21058ed77b0cea11 Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Fri, 17 Jan 2014 11:45:37 +0100 Subject: V4: fix range splitting when split is between intervals. Also added some "white-box" unit tests and sprinkled in a bit of documentation. The case that went wrong is covered by the test rangeSplitting_1: before the fix, the new interval would have two ranges: [66-64],[70-71]. The first range is invalid and should not be there at all. Change-Id: If0742f4e6a96d98ea5d696f95126886ba66f92bb Reviewed-by: Simon Hausmann --- src/qml/compiler/qv4jsir_p.h | 4 +- src/qml/compiler/qv4regalloc.cpp | 4 +- src/qml/compiler/qv4ssa.cpp | 47 +++++++++-- src/qml/compiler/qv4ssa_p.h | 16 +++- tests/auto/qml/qml.pro | 3 +- tests/auto/qml/v4misc/tst_v4misc.cpp | 157 +++++++++++++++++++++++++++++++++++ tests/auto/qml/v4misc/v4misc.pro | 9 ++ 7 files changed, 227 insertions(+), 13 deletions(-) create mode 100644 tests/auto/qml/v4misc/tst_v4misc.cpp create mode 100644 tests/auto/qml/v4misc/v4misc.pro diff --git a/src/qml/compiler/qv4jsir_p.h b/src/qml/compiler/qv4jsir_p.h index 2eba3405fe..46aea546ab 100644 --- a/src/qml/compiler/qv4jsir_p.h +++ b/src/qml/compiler/qv4jsir_p.h @@ -248,7 +248,7 @@ struct MemberExpressionResolver unsigned int isQObjectResolver; // neede for IR dump helpers }; -struct Expr { +struct Q_AUTOTEST_EXPORT Expr { Type type; Expr(): type(UnknownType) {} @@ -380,7 +380,7 @@ struct Name: Expr { virtual void dump(QTextStream &out) const; }; -struct Temp: Expr { +struct Q_AUTOTEST_EXPORT Temp: Expr { enum Kind { Formal = 0, ScopedFormal, diff --git a/src/qml/compiler/qv4regalloc.cpp b/src/qml/compiler/qv4regalloc.cpp index 3521d0c27a..048aad8497 100644 --- a/src/qml/compiler/qv4regalloc.cpp +++ b/src/qml/compiler/qv4regalloc.cpp @@ -913,8 +913,9 @@ private: } } if (!moveFrom) { +#if defined(QT_NO_DEBUG) Q_UNUSED(lifeTimeHole); -#if !defined(QT_NO_DEBUG) +#else Q_ASSERT(!_info->isPhiTarget(it->temp()) || it->isSplitFromInterval() || lifeTimeHole); if (_info->def(it->temp()) != successorStart && !it->isSplitFromInterval()) { const int successorEnd = successor->statements.last()->id; @@ -1492,6 +1493,7 @@ int RegisterAllocator::nextUse(const Temp &t, int startPosition) const static inline void insertSorted(QVector &intervals, const LifeTimeInterval &newInterval) { + newInterval.validate(); for (int i = 0, ei = intervals.size(); i != ei; ++i) { if (LifeTimeInterval::lessThan(newInterval, intervals.at(i))) { intervals.insert(i, newInterval); diff --git a/src/qml/compiler/qv4ssa.cpp b/src/qml/compiler/qv4ssa.cpp index 7113dc7c26..6b4d7e7434 100644 --- a/src/qml/compiler/qv4ssa.cpp +++ b/src/qml/compiler/qv4ssa.cpp @@ -3634,12 +3634,20 @@ LifeTimeInterval LifeTimeInterval::split(int atPosition, int newStart) LifeTimeInterval newInterval = *this; newInterval.setSplitFromInterval(true); + // search where to split the interval for (int i = 0, ei = _ranges.size(); i < ei; ++i) { - if (_ranges[i].covers(atPosition)) { - while (_ranges.size() > i + 1) - _ranges.removeLast(); - while (newInterval._ranges.size() > ei - i) - newInterval._ranges.removeFirst(); + if (_ranges[i].start <= atPosition) { + if (_ranges[i].end >= atPosition) { + // split happens in the middle of a range. Keep this range in both the old and the + // new interval, and correct the end/start later + _ranges.resize(i + 1); + newInterval._ranges.remove(0, i); + break; + } + } else { + // split happens between two ranges. + _ranges.resize(i); + newInterval._ranges.remove(0, i); break; } } @@ -3648,14 +3656,37 @@ LifeTimeInterval LifeTimeInterval::split(int atPosition, int newStart) newInterval._ranges.removeFirst(); if (newStart == Invalid) { - newInterval._ranges.clear(); - newInterval._end = Invalid; + // the temp stays inactive for the rest of its lifetime + newInterval = LifeTimeInterval(); } else { + // find the first range where the temp will get active again: + while (!newInterval._ranges.isEmpty()) { + const Range &range = newInterval._ranges.first(); + if (range.start > newStart) { + // The split position is before the start of the range. Either we managed to skip + // over the correct range, or we got an invalid split request. Either way, this + // Should Never Happen . + Q_ASSERT(range.start > newStart); + return LifeTimeInterval(); + } else if (range.start <= newStart && range.end >= newStart) { + // yay, we found the range that should be the new first range in the new interval! + break; + } else { + // the temp stays inactive for this interval, so remove it. + newInterval._ranges.removeFirst(); + } + } + Q_ASSERT(!newInterval._ranges.isEmpty()); newInterval._ranges.first().start = newStart; _end = newStart; } - _ranges.last().end = atPosition; + // if we're in the middle of a range, set the end to the split position + if (_ranges.last().end > atPosition) + _ranges.last().end = atPosition; + + validate(); + newInterval.validate(); return newInterval; } diff --git a/src/qml/compiler/qv4ssa_p.h b/src/qml/compiler/qv4ssa_p.h index f90fc5b05b..65d4f0834a 100644 --- a/src/qml/compiler/qv4ssa_p.h +++ b/src/qml/compiler/qv4ssa_p.h @@ -51,7 +51,7 @@ class QQmlEnginePrivate; namespace QQmlJS { namespace V4IR { -class LifeTimeInterval { +class Q_AUTOTEST_EXPORT LifeTimeInterval { public: struct Range { int start; @@ -121,6 +121,20 @@ public: void dump(QTextStream &out) const; static bool lessThan(const LifeTimeInterval &r1, const LifeTimeInterval &r2); static bool lessThanForTemp(const LifeTimeInterval &r1, const LifeTimeInterval &r2); + + void validate() const { +#if !defined(QT_NO_DEBUG) + // Validate the new range + if (_end != Invalid) { + Q_ASSERT(!_ranges.isEmpty()); + foreach (const Range &range, _ranges) { + Q_ASSERT(range.start >= 0); + Q_ASSERT(range.end >= 0); + Q_ASSERT(range.start <= range.end); + } + } +#endif + } }; class Optimizer diff --git a/tests/auto/qml/qml.pro b/tests/auto/qml/qml.pro index eec6584472..a330cd43aa 100644 --- a/tests/auto/qml/qml.pro +++ b/tests/auto/qml/qml.pro @@ -58,7 +58,8 @@ PRIVATETESTS += \ qqmltimer \ qqmlinstantiator \ qv4debugger \ - qqmlenginecleanup + qqmlenginecleanup \ + v4misc qtHaveModule(widgets) { PUBLICTESTS += \ diff --git a/tests/auto/qml/v4misc/tst_v4misc.cpp b/tests/auto/qml/v4misc/tst_v4misc.cpp new file mode 100644 index 0000000000..da2b793b71 --- /dev/null +++ b/tests/auto/qml/v4misc/tst_v4misc.cpp @@ -0,0 +1,157 @@ +/**************************************************************************** +** +** Copyright (C) 2013 Digia Plc and/or its subsidiary(-ies). +** Contact: http://www.qt-project.org/legal +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:LGPL$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and Digia. For licensing terms and +** conditions see http://qt.digia.com/licensing. For further information +** use the contact form at http://qt.digia.com/contact-us. +** +** GNU Lesser General Public License Usage +** Alternatively, this file may be used under the terms of the GNU Lesser +** General Public License version 2.1 as published by the Free Software +** Foundation and appearing in the file LICENSE.LGPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU Lesser General Public License version 2.1 requirements +** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. +** +** In addition, as a special exception, Digia gives you certain additional +** rights. These rights are described in the Digia Qt LGPL Exception +** version 1.1, included in the file LGPL_EXCEPTION.txt in this package. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3.0 as published by the Free Software +** Foundation and appearing in the file LICENSE.GPL included in the +** packaging of this file. Please review the following information to +** ensure the GNU General Public License version 3.0 requirements will be +** met: http://www.gnu.org/copyleft/gpl.html. +** +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#include + +#include + +class tst_v4misc: public QObject +{ + Q_OBJECT + +private slots: + void initTestCase(); + + void rangeSplitting_1(); + void rangeSplitting_2(); + void rangeSplitting_3(); +}; + +QT_BEGIN_NAMESPACE +// Avoid QHash randomization so that the temp numbering is stable. +extern Q_CORE_EXPORT QBasicAtomicInt qt_qhash_seed; // from qhash.cpp +QT_END_NAMESPACE + +using namespace QT_PREPEND_NAMESPACE(QQmlJS::V4IR); + +void tst_v4misc::initTestCase() +{ + qt_qhash_seed.store(0); + QCOMPARE(qt_qhash_seed.load(), 0); +} + +// split between two ranges +void tst_v4misc::rangeSplitting_1() +{ + LifeTimeInterval interval; + interval.addRange(59, 59); + interval.addRange(61, 62); + interval.addRange(64, 64); + interval.addRange(69, 71); + interval.validate(); + QCOMPARE(interval.end(), 71); + + LifeTimeInterval newInterval = interval.split(66, 70); + interval.validate(); + newInterval.validate(); + QVERIFY(newInterval.isSplitFromInterval()); + + QCOMPARE(interval.ranges().size(), 3); + QCOMPARE(interval.ranges()[0].start, 59); + QCOMPARE(interval.ranges()[0].end, 59); + QCOMPARE(interval.ranges()[1].start, 61); + QCOMPARE(interval.ranges()[1].end, 62); + QCOMPARE(interval.ranges()[2].start, 64); + QCOMPARE(interval.ranges()[2].end, 64); + QCOMPARE(interval.end(), 70); + + QCOMPARE(newInterval.ranges().size(), 1); + QCOMPARE(newInterval.ranges()[0].start, 70); + QCOMPARE(newInterval.ranges()[0].end, 71); + QCOMPARE(newInterval.end(), 71); +} + +// split in the middle of a range +void tst_v4misc::rangeSplitting_2() +{ + LifeTimeInterval interval; + interval.addRange(59, 59); + interval.addRange(61, 64); + interval.addRange(69, 71); + interval.validate(); + QCOMPARE(interval.end(), 71); + + LifeTimeInterval newInterval = interval.split(62, 64); + interval.validate(); + newInterval.validate(); + QVERIFY(newInterval.isSplitFromInterval()); + + QCOMPARE(interval.ranges().size(), 2); + QCOMPARE(interval.ranges()[0].start, 59); + QCOMPARE(interval.ranges()[0].end, 59); + QCOMPARE(interval.ranges()[1].start, 61); + QCOMPARE(interval.ranges()[1].end, 62); + QCOMPARE(interval.end(), 64); + + QCOMPARE(newInterval.ranges().size(), 2); + QCOMPARE(newInterval.ranges()[0].start, 64); + QCOMPARE(newInterval.ranges()[0].end, 64); + QCOMPARE(newInterval.ranges()[1].start, 69); + QCOMPARE(newInterval.ranges()[1].end, 71); + QCOMPARE(newInterval.end(), 71); +} + +// split in the middle of a range, and let it never go back to active again +void tst_v4misc::rangeSplitting_3() +{ + LifeTimeInterval interval; + interval.addRange(59, 59); + interval.addRange(61, 64); + interval.addRange(69, 71); + interval.validate(); + QCOMPARE(interval.end(), 71); + + LifeTimeInterval newInterval = interval.split(64, LifeTimeInterval::Invalid); + interval.validate(); + newInterval.validate(); + QVERIFY(!newInterval.isValid()); + + QCOMPARE(interval.ranges().size(), 2); + QCOMPARE(interval.ranges()[0].start, 59); + QCOMPARE(interval.ranges()[0].end, 59); + QCOMPARE(interval.ranges()[1].start, 61); + QCOMPARE(interval.ranges()[1].end, 64); + QCOMPARE(interval.end(), 71); +} + +QTEST_MAIN(tst_v4misc) + +#include "tst_v4misc.moc" diff --git a/tests/auto/qml/v4misc/v4misc.pro b/tests/auto/qml/v4misc/v4misc.pro new file mode 100644 index 0000000000..d68025f410 --- /dev/null +++ b/tests/auto/qml/v4misc/v4misc.pro @@ -0,0 +1,9 @@ +CONFIG += testcase +TARGET = tst_v4misc +macx:CONFIG -= app_bundle + +SOURCES += tst_v4misc.cpp + +CONFIG += parallel_test +QT += core-private qml-private testlib +DEFINES += QT_DISABLE_DEPRECATED_BEFORE=0 -- cgit v1.2.3