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@collabora.com>

9 files changed, 107 insertions, 9 deletions

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)