Calc threading : Check for "self" references...

...on indirect dependencies too.

Here a self reference to any formula-group
means if there are any references in a formula
(of the formula-group itself or any of its dependencies)
that points to any element inside the formula-group.
If there are any self-references, then that formula-group
can't be computed in parallel.

For example, with this patch we can detect the following case:-
Suppose the formula-group that we want to check is:
"=(F2+G2-10)*10.0" spanning A2:A100. Let the formula-group
starting at F2 be "=A1*0.1-10". The indirect dependency
formula-group starting at F2, references back the elements of
our original formula-group at A2. This makes the F.G at
A2 unsafe for parallel computation.

Concretly, this patch fixes a recalc crash on tdf#63638/1

Change-Id: I7b999a34571b191d2f70da6a3831f78b24a6b0a7
Reviewed-on: https://gerrit.libreoffice.org/54433
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Jenkins <ci@libreoffice.org>

4 files changed, 67 insertions, 2 deletions

diff --git a/sc/inc/formulacell.hxx b/sc/inc/formulacell.hxx
index 7656e7a3efec..cee9fec13fc2 100644
--- a/sc/inc/formulacell.hxx
+++ b/sc/inc/formulacell.hxx
@@ -66,6 +66,7 @@ public:
     SvNumFormatType mnFormatType;
     bool mbInvariant:1;
     bool mbSubTotal:1;
+    bool mbSeenInPath:1; // For detecting cycle of formula groups
 
     sal_uInt8 meCalcState;
 
diff --git a/sc/inc/recursionhelper.hxx b/sc/inc/recursionhelper.hxx
index 560b66ab357e..b8ca1d087509 100644
--- a/sc/inc/recursionhelper.hxx
+++ b/sc/inc/recursionhelper.hxx
@@ -23,9 +23,11 @@
 #include "formularesult.hxx"
 
 #include <list>
+#include <vector>
 #include <stack>
 
 class ScFormulaCell;
+struct ScFormulaCellGroup;
 
 struct ScFormulaRecursionEntry
 {
@@ -44,10 +46,12 @@ typedef ::std::list< ScFormulaRecursionEntry > ScFormulaRecursionList;
 class ScRecursionHelper
 {
     typedef ::std::stack< ScFormulaCell* >  ScRecursionInIterationStack;
+    typedef ::std::vector< ScFormulaCellGroup* > ScFGList;
     ScFormulaRecursionList              aRecursionFormulas;
     ScFormulaRecursionList::iterator    aInsertPos;
     ScFormulaRecursionList::iterator    aLastIterationStart;
     ScRecursionInIterationStack         aRecursionInIterationStack;
+    ScFGList                            aFGList;
     sal_uInt16                              nRecursionCount;
     sal_uInt16                              nIteration;
     bool                                bInRecursionReturn;
@@ -94,6 +98,10 @@ public:
     ScRecursionInIterationStack&    GetRecursionInIterationStack()  { return aRecursionInIterationStack; }
 
     void Clear();
+
+    /** Detects whether the formula-group is part of a simple cycle of formula-groups. */
+    bool PushFormulaGroup(ScFormulaCellGroup* pGrp);
+    void PopFormulaGroup();
 };
 
 #endif // INCLUDED_SC_INC_RECURSIONHELPER_HXX
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index 9731f8a6f297..daaa80e5b921 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -528,6 +528,7 @@ ScFormulaCellGroup::ScFormulaCellGroup() :
     mnFormatType(SvNumFormatType::NUMBER),
     mbInvariant(false),
     mbSubTotal(false),
+    mbSeenInPath(false),
     meCalcState(sc::GroupCalcEnabled)
 {
 }
@@ -1527,6 +1528,14 @@ void ScFormulaCell::Interpret()
     else
     {
         pDocument->IncInterpretLevel();
+
+        bool bCheckForFGCycle = mxGroup && !pDocument->mbThreadedGroupCalcInProgress &&
+            mxGroup->meCalcState == sc::GroupCalcEnabled &&
+            ScCalcConfig::isThreadingEnabled();
+        bool bPopFormulaGroup = false;
+        if (bCheckForFGCycle)
+            bPopFormulaGroup = rRecursionHelper.PushFormulaGroup(mxGroup.get());
+
 #if DEBUG_CALCULATION
         aDC.enterGroup();
         bool bGroupInterpreted = InterpretFormulaGroup();
@@ -1537,6 +1546,10 @@ void ScFormulaCell::Interpret()
         if (!InterpretFormulaGroup())
             InterpretTail( pDocument->GetNonThreadedContext(), SCITP_NORMAL);
 #endif
+
+        if (bPopFormulaGroup)
+            rRecursionHelper.PopFormulaGroup();
+
         pDocument->DecInterpretLevel();
     }
 
@@ -4129,14 +4142,14 @@ struct ScDependantsCalculator
 
         SCROW nEndRow = mrPos.Row() + mnLen - 1;
 
-        if (nRelRow < 0)
+        if (nRelRow <= 0)
         {
             SCROW nTest = nEndRow;
             nTest += nRelRow;
             if (nTest >= mrPos.Row())
                 return true;
         }
-        else if (nRelRow > 0)
+        else
         {
             SCROW nTest = mrPos.Row(); // top row.
             nTest += nRelRow;
@@ -4406,6 +4419,12 @@ bool ScFormulaCell::InterpretFormulaGroup()
             return false;
         }
 
+        if (mxGroup->meCalcState == sc::GroupCalcDisabled)
+        {
+            aScope.addMessage("found circular formula-group dependencies");
+            return false;
+        }
+
         static bool bHyperThreadingActive = tools::cpuid::hasHyperThreading();
 
         // Then do the threaded calculation
diff --git a/sc/source/core/tool/recursionhelper.cxx b/sc/source/core/tool/recursionhelper.cxx
index 25591f41e823..de75e8557605 100644
--- a/sc/source/core/tool/recursionhelper.cxx
+++ b/sc/source/core/tool/recursionhelper.cxx
@@ -8,6 +8,7 @@
  */
 
 #include <recursionhelper.hxx>
+#include <formulacell.hxx>
 
 void ScRecursionHelper::Init()
 {
@@ -15,6 +16,7 @@ void ScRecursionHelper::Init()
     bInRecursionReturn = bDoingRecursion = bInIterationReturn = false;
     aInsertPos = GetIterationEnd();
     ResetIteration();
+    aFGList.clear();
 }
 
 void ScRecursionHelper::ResetIteration()
@@ -94,4 +96,39 @@ void ScRecursionHelper::Clear()
     Init();
 }
 
+bool ScRecursionHelper::PushFormulaGroup(ScFormulaCellGroup* pGrp)
+{
+    assert(pGrp);
+    if (pGrp->mbSeenInPath)
+    {
+        // Found a simple cycle of formula-groups.
+        // Disable group calc for all elements of this cycle.
+        sal_Int32 nIdx = aFGList.size();
+        assert(nIdx > 0);
+        do
+        {
+            --nIdx;
+            assert(nIdx >= 0);
+            auto& eCalcState = aFGList[nIdx]->meCalcState;
+            if (eCalcState == sc::GroupCalcEnabled)
+                eCalcState = sc::GroupCalcDisabled;
+        } while (aFGList[nIdx] != pGrp);
+
+        return false;
+    }
+
+    pGrp->mbSeenInPath = true;
+    aFGList.push_back(pGrp);
+    return true;
+}
+
+void ScRecursionHelper::PopFormulaGroup()
+{
+    if (aFGList.empty())
+        return;
+    ScFormulaCellGroup* pGrp = aFGList.back();
+    pGrp->mbSeenInPath = false;
+    aFGList.pop_back();
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */