[Libreoffice-commits] core.git: sc/inc sc/source
Libreoffice Gerrit user
logerrit at kemper.freedesktop.org
Mon Dec 3 14:33:11 UTC 2018
sc/inc/column.hxx | 3 ++
sc/inc/document.hxx | 3 ++
sc/inc/formulacell.hxx | 2 -
sc/inc/table.hxx | 3 ++
sc/source/core/data/column2.cxx | 29 ++++++++++++++++++++
sc/source/core/data/document.cxx | 9 ++++++
sc/source/core/data/formulacell.cxx | 39 ++++++++++++++++++++++------
sc/source/core/data/grouptokenconverter.cxx | 19 +++++++++++++
sc/source/core/data/table1.cxx | 9 ++++++
9 files changed, 107 insertions(+), 9 deletions(-)
New commits:
commit 99014ec9ded70a679220fe59e09ab4073512c249
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Wed Nov 28 15:32:20 2018 +0100
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Mon Dec 3 15:32:45 2018 +0100
make sure FetchVectorRefArray() never triggers Interpret()
Test::testFormulaRefUpdateRange could trigger this, leading to recursion
that wasn't handled properly by the code, since it wasn't expected
to happen at late time (ScDependantsCalculator should have already
caught it). This is all caused by the fact that FetchVectorRefArray()
fetches also all rows before the given rows (to make the caching simpler
I suppose). But that fetching could lead to Interpret() calls.
Therefore, make ScDependantsCalculator in OpenCL mode check also all
rows above.
Change-Id: Iaecc105663df21b01443759287cec605470d34a5
Reviewed-on: https://gerrit.libreoffice.org/64236
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak at collabora.com>
diff --git a/sc/inc/column.hxx b/sc/inc/column.hxx
index bdaeccde0afe..0a328f001ce4 100644
--- a/sc/inc/column.hxx
+++ b/sc/inc/column.hxx
@@ -575,6 +575,9 @@ public:
void FillMatrix( ScMatrix& rMat, size_t nMatCol, SCROW nRow1, SCROW nRow2, svl::SharedStringPool* pPool ) const;
formula::VectorRefArray FetchVectorRefArray( SCROW nRow1, SCROW nRow2 );
bool HandleRefArrayForParallelism( SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup );
+#ifdef DBG_UTIL
+ void AssertNoInterpretNeeded( SCROW nRow1, SCROW nRow2 );
+#endif
void SetFormulaResults( SCROW nRow, const double* pResults, size_t nLen );
void CalculateInThread( ScInterpreterContext& rContext, SCROW nRow, size_t nLen, unsigned nThisThread, unsigned nThreadsTotal );
diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index dd9757aac61b..04ff9a0c1319 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -2398,6 +2398,9 @@ public:
formula::VectorRefArray FetchVectorRefArray( const ScAddress& rPos, SCROW nLength );
bool HandleRefArrayForParallelism( const ScAddress& rPos, SCROW nLength, const ScFormulaCellGroupRef& mxGroup );
+#ifdef DBG_UTIL
+ void AssertNoInterpretNeeded( const ScAddress& rPos, SCROW nLength );
+#endif
/**
* Call this before any operations that might trigger one or more formula
diff --git a/sc/inc/formulacell.hxx b/sc/inc/formulacell.hxx
index f5b4da63b65c..f344b46034da 100644
--- a/sc/inc/formulacell.hxx
+++ b/sc/inc/formulacell.hxx
@@ -143,7 +143,7 @@ private:
ScFormulaCell( const ScFormulaCell& ) = delete;
- bool CheckComputeDependencies(sc::FormulaLogger::GroupScope& rScope);
+ bool CheckComputeDependencies(sc::FormulaLogger::GroupScope& rScope, bool fromFirstRow = false);
bool InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope& aScope,
bool& bDependencyComputed,
bool& bDependencyCheckFailed);
diff --git a/sc/inc/table.hxx b/sc/inc/table.hxx
index 0e5b62837b92..aae436ea8756 100644
--- a/sc/inc/table.hxx
+++ b/sc/inc/table.hxx
@@ -992,6 +992,9 @@ public:
formula::FormulaTokenRef ResolveStaticReference( SCCOL nCol1, SCROW nRow1, SCCOL nCol2, SCROW nRow2 );
formula::VectorRefArray FetchVectorRefArray( SCCOL nCol, SCROW nRow1, SCROW nRow2 );
bool HandleRefArrayForParallelism( SCCOL nCol, SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup );
+#ifdef DBG_UTIL
+ void AssertNoInterpretNeeded( SCCOL nCol, SCROW nRow1, SCROW nRow2 );
+#endif
void SplitFormulaGroups( SCCOL nCol, std::vector<SCROW>& rRows );
void UnshareFormulaCells( SCCOL nCol, std::vector<SCROW>& rRows );
diff --git a/sc/source/core/data/column2.cxx b/sc/source/core/data/column2.cxx
index a4078e75427d..d2e81de5de3b 100644
--- a/sc/source/core/data/column2.cxx
+++ b/sc/source/core/data/column2.cxx
@@ -2834,6 +2834,35 @@ formula::VectorRefArray ScColumn::FetchVectorRefArray( SCROW nRow1, SCROW nRow2
return formula::VectorRefArray(formula::VectorRefArray::Invalid);
}
+#ifdef DBG_UTIL
+static void assertNoInterpretNeededHelper( const sc::CellStoreType::value_type& node,
+ size_t nOffset, size_t nDataSize )
+{
+ switch (node.type)
+ {
+ case sc::element_type_formula:
+ {
+ sc::formula_block::const_iterator it = sc::formula_block::begin(*node.data);
+ std::advance(it, nOffset);
+ sc::formula_block::const_iterator itEnd = it;
+ std::advance(itEnd, nDataSize);
+ for (; it != itEnd; ++it)
+ {
+ const ScFormulaCell* pCell = *it;
+ assert( !pCell->NeedsInterpret());
+ }
+ break;
+ }
+ }
+}
+void ScColumn::AssertNoInterpretNeeded( SCROW nRow1, SCROW nRow2 )
+{
+ assert(nRow2 >= nRow1);
+ sc::ParseBlock( maCells.begin(), maCells, assertNoInterpretNeededHelper, 0, nRow2 );
+}
+#endif
+
+
bool ScColumn::HandleRefArrayForParallelism( SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup )
{
if (nRow1 > nRow2)
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index abda9f677f36..9693edffda3e 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -1774,6 +1774,15 @@ formula::VectorRefArray ScDocument::FetchVectorRefArray( const ScAddress& rPos,
return maTabs[nTab]->FetchVectorRefArray(rPos.Col(), rPos.Row(), rPos.Row()+nLength-1);
}
+#ifdef DBG_UTIL
+void ScDocument::AssertNoInterpretNeeded( const ScAddress& rPos, SCROW nLength )
+{
+ SCTAB nTab = rPos.Tab();
+ assert(TableExists(nTab));
+ return maTabs[nTab]->AssertNoInterpretNeeded(rPos.Col(), rPos.Row(), rPos.Row()+nLength-1);
+}
+#endif
+
void ScDocument::UnlockAdjustHeight()
{
assert(nAdjustHeightLock > 0);
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index 622313e2cc63..83da566d4b4b 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -4235,17 +4235,24 @@ struct ScDependantsCalculator
const ScFormulaCellGroupRef& mxGroup;
const SCROW mnLen;
const ScAddress& mrPos;
+ const bool mFromFirstRow;
- ScDependantsCalculator(ScDocument& rDoc, const ScTokenArray& rCode, const ScFormulaCell& rCell, const ScAddress& rPos) :
+ ScDependantsCalculator(ScDocument& rDoc, const ScTokenArray& rCode, const ScFormulaCell& rCell,
+ const ScAddress& rPos, bool fromFirstRow) :
mrDoc(rDoc),
mrCode(rCode),
mxGroup(rCell.GetCellGroup()),
mnLen(mxGroup->mnLength),
- mrPos(rPos)
+ mrPos(rPos),
+ // ScColumn::FetchVectorRefArray() always fetches data from row 0, even if the data is used
+ // only from further rows. This data fetching could also lead to Interpret() calls, so
+ // in OpenCL mode the formula in practice depends on those cells too.
+ mFromFirstRow(fromFirstRow)
{
}
// FIXME: copy-pasted from ScGroupTokenConverter. factor out somewhere else
+ // (note already modified a bit, mFromFirstRow)
// I think what this function does is to check whether the relative row reference nRelRow points
// to a row that is inside the range of rows covered by the formula group.
@@ -4270,6 +4277,9 @@ struct ScDependantsCalculator
nTest += nRelRow;
if (nTest <= nEndRow)
return true;
+ // If pointing below the formula, it's always included if going from first row.
+ if (mFromFirstRow)
+ return true;
}
return false;
@@ -4290,7 +4300,8 @@ struct ScDependantsCalculator
if (rRefPos.Row() < mrPos.Row())
return false;
- if (rRefPos.Row() > nEndRow)
+ // If pointing below the formula, it's always included if going from first row.
+ if (rRefPos.Row() > nEndRow && !mFromFirstRow)
return false;
return true;
@@ -4326,6 +4337,11 @@ struct ScDependantsCalculator
nRefStartRow <= nStartRow && nRefEndRow >= nEndRow)
return true;
+ // If going from first row, the referenced range must be entirely above the formula,
+ // otherwise the formula would be included.
+ if (mFromFirstRow && nRefEndRow >= nStartRow)
+ return true;
+
return false;
}
@@ -4482,8 +4498,15 @@ struct ScDependantsCalculator
assert(rRange.aStart.Tab() == rRange.aEnd.Tab());
for (auto nCol = rRange.aStart.Col(); nCol <= rRange.aEnd.Col(); nCol++)
{
- if (!mrDoc.HandleRefArrayForParallelism(ScAddress(nCol, rRange.aStart.Row(), rRange.aStart.Tab()),
- rRange.aEnd.Row() - rRange.aStart.Row() + 1, mxGroup))
+ SCROW nStartRow = rRange.aStart.Row();
+ SCROW nLength = rRange.aEnd.Row() - rRange.aStart.Row() + 1;
+ if( mFromFirstRow )
+ { // include also all previous rows
+ nLength += nStartRow;
+ nStartRow = 0;
+ }
+ if (!mrDoc.HandleRefArrayForParallelism(ScAddress(nCol, nStartRow, rRange.aStart.Tab()),
+ nLength, mxGroup))
return false;
}
}
@@ -4564,7 +4587,7 @@ bool ScFormulaCell::InterpretFormulaGroup()
return false;
}
-bool ScFormulaCell::CheckComputeDependencies(sc::FormulaLogger::GroupScope& rScope)
+bool ScFormulaCell::CheckComputeDependencies(sc::FormulaLogger::GroupScope& rScope, bool fromFirstRow)
{
ScRecursionHelper& rRecursionHelper = pDocument->GetRecursionHelper();
// iterate over code in the formula ...
@@ -4582,7 +4605,7 @@ bool ScFormulaCell::CheckComputeDependencies(sc::FormulaLogger::GroupScope& rSco
}
ScFormulaGroupDependencyComputeGuard aDepComputeGuard(rRecursionHelper);
- ScDependantsCalculator aCalculator(*pDocument, *pCode, *this, mxGroup->mpTopCell->aPos);
+ ScDependantsCalculator aCalculator(*pDocument, *pCode, *this, mxGroup->mpTopCell->aPos, fromFirstRow);
bOKToParallelize = aCalculator.DoIt();
}
@@ -4763,7 +4786,7 @@ bool ScFormulaCell::InterpretFormulaGroupOpenCL(sc::FormulaLogger::GroupScope& a
if (bDependencyCheckFailed)
return false;
- if(!bDependencyComputed && !CheckComputeDependencies(aScope))
+ if(!bDependencyComputed && !CheckComputeDependencies(aScope, true))
{
bDependencyComputed = true;
bDependencyCheckFailed = true;
diff --git a/sc/source/core/data/grouptokenconverter.cxx b/sc/source/core/data/grouptokenconverter.cxx
index 1e9f5e5aa8c9..9da4dc476ea2 100644
--- a/sc/source/core/data/grouptokenconverter.cxx
+++ b/sc/source/core/data/grouptokenconverter.cxx
@@ -135,7 +135,20 @@ bool ScGroupTokenConverter::convert( const ScTokenArray& rCode, sc::FormulaLogge
formula::VectorRefArray aArray;
if (nTrimLen)
+ {
+#ifdef DBG_UTIL
+ // All the necessary Interpret() calls for all the cells
+ // should have been already handled by ScDependantsCalculator
+ // calling HandleRefArrayForParallelism(), and that handling also checks
+ // for cycles etc. Recursively calling Interpret() from here (which shouldn't
+ // happen) could lead to unhandled problems.
+ // Also, because of caching FetchVectorRefArray() fetches values for all rows
+ // up to the maximum one, so check those too.
+ mrDoc.AssertNoInterpretNeeded(
+ ScAddress(aRefPos.Col(), 0, aRefPos.Tab()), nTrimLen + aRefPos.Row());
+#endif
aArray = mrDoc.FetchVectorRefArray(aRefPos, nTrimLen);
+ }
if (!aArray.isValid())
return false;
@@ -230,7 +243,13 @@ bool ScGroupTokenConverter::convert( const ScTokenArray& rCode, sc::FormulaLogge
aRefPos.SetCol(i);
formula::VectorRefArray aArray;
if (nArrayLength)
+ {
+#ifdef DBG_UTIL
+ mrDoc.AssertNoInterpretNeeded(
+ ScAddress(aRefPos.Col(), 0, aRefPos.Tab()), nArrayLength + aRefPos.Row());
+#endif
aArray = mrDoc.FetchVectorRefArray(aRefPos, nArrayLength);
+ }
if (!aArray.isValid())
return false;
diff --git a/sc/source/core/data/table1.cxx b/sc/source/core/data/table1.cxx
index 48a9ec2d4f36..ede4096b7f3d 100644
--- a/sc/source/core/data/table1.cxx
+++ b/sc/source/core/data/table1.cxx
@@ -2336,6 +2336,15 @@ formula::VectorRefArray ScTable::FetchVectorRefArray( SCCOL nCol, SCROW nRow1, S
return aCol[nCol].FetchVectorRefArray(nRow1, nRow2);
}
+#ifdef DBG_UTIL
+void ScTable::AssertNoInterpretNeeded( SCCOL nCol, SCROW nRow1, SCROW nRow2 )
+{
+ assert( nRow2 >= nRow1 );
+ assert( IsColValid( nCol ) && ValidRow( nRow1 ) && ValidRow( nRow2 ) );
+ return aCol[nCol].AssertNoInterpretNeeded(nRow1, nRow2);
+}
+#endif
+
bool ScTable::HandleRefArrayForParallelism( SCCOL nCol, SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup )
{
if (nRow2 < nRow1)
More information about the Libreoffice-commits
mailing list