sc/inc/recursionhelper.hxx | 10 ++++-
sc/qa/unit/parallelism.cxx | 54 ++++++++++++++++++++++++++++++++
sc/source/core/data/column4.cxx | 2 -
sc/source/core/data/formulacell.cxx | 15 ++++++++
sc/source/core/tool/recursionhelper.cxx | 18 ++++++++++
5 files changed, 95 insertions(+), 4 deletions(-)
New commits:
commit afb6dd43af5d1179c2e3cd8f00794cd4c3d75b2d
Author: Luboš Luňák <l.lu...@collabora.com>
AuthorDate: Mon Jun 22 11:42:22 2020 +0200
Commit: Luboš Luňák <l.lu...@collabora.com>
CommitDate: Mon Jun 22 19:18:56 2020 +0200
failed cell dependency check should not set invalid values (tdf#132451)
Calc's dependency check done before parallel formula cell group
calculation tries to ensure valid cell values for all the dependencies
of the group's cell, and if it detects a problem such as a cycle
it bails out. But since ScFormulaCell::Interpret() simply bailed out
without doing anything, other cells could use that cell's possibly
incorrect value for their calculation and get their dirty flag reset.
This fix adds a flag to mark that bailing out is in progress, which
ensures the bail-out is short-circuited and no cell values are set.
Change-Id: Ia93c70d456682e19ce533abd2cf65ce35ffed9ca
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96838
Reviewed-by: Dennis Francis <dennis.fran...@collabora.com>
Tested-by: Jenkins
(cherry picked from commit 82803ef4736fbed89dd8ae0723f2c4f30e37ba8e)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96801
diff --git a/sc/inc/recursionhelper.hxx b/sc/inc/recursionhelper.hxx
index 2c7a6605042e..14abbcf0a052 100644
--- a/sc/inc/recursionhelper.hxx
+++ b/sc/inc/recursionhelper.hxx
@@ -64,6 +64,7 @@ class ScRecursionHelper
bool bInIterationReturn;
bool bConverging;
bool bGroupsIndependent;
+ bool bAbortingDependencyComputation;
std::vector< ScFormulaCell* > aTemporaryGroupCells;
o3tl::sorted_vector< ScFormulaCellGroup* >* pFGSet;
@@ -77,8 +78,8 @@ public:
void IncRecursionCount() { ++nRecursionCount; }
void DecRecursionCount() { --nRecursionCount; }
sal_uInt16 GetDepComputeLevel() const { return
nDependencyComputationLevel; }
- void IncDepComputeLevel() { ++nDependencyComputationLevel; }
- void DecDepComputeLevel() { --nDependencyComputationLevel; }
+ void IncDepComputeLevel();
+ void DecDepComputeLevel();
/// A pure recursion return, no iteration.
bool IsInRecursionReturn() const { return bInRecursionReturn &&
!bInIterationReturn; }
@@ -116,6 +117,11 @@ public:
bool AnyCycleMemberInDependencyEvalMode(ScFormulaCell* pCell);
bool AnyParentFGInCycle();
void SetFormulaGroupDepEvalMode(bool bSet);
+ // When dependency computation detects a cycle, it may not compute proper
cell values.
+ // This sets a flag that ScFormulaCell will use to avoid setting those new
values
+ // and resetting the dirty flag, until the dependency computation bails
out.
+ void AbortDependencyComputation();
+ bool IsAbortingDependencyComputation() const { return
bAbortingDependencyComputation; }
void AddTemporaryGroupCell(ScFormulaCell* cell);
void CleanTemporaryGroupCells();
diff --git a/sc/qa/unit/parallelism.cxx b/sc/qa/unit/parallelism.cxx
index 9af1daaca9e0..7bd370db3a54 100644
--- a/sc/qa/unit/parallelism.cxx
+++ b/sc/qa/unit/parallelism.cxx
@@ -48,6 +48,7 @@ public:
void testFormulaGroupWithForwardSelfReference();
void testFormulaGroupsInCyclesAndWithSelfReference();
void testFormulaGroupsInCyclesAndWithSelfReference2();
+ void testFormulaGroupsInCyclesAndWithSelfReference3();
CPPUNIT_TEST_SUITE(ScParallelismTest);
CPPUNIT_TEST(testSUMIFS);
@@ -66,6 +67,7 @@ public:
CPPUNIT_TEST(testFormulaGroupWithForwardSelfReference);
CPPUNIT_TEST(testFormulaGroupsInCyclesAndWithSelfReference);
CPPUNIT_TEST(testFormulaGroupsInCyclesAndWithSelfReference2);
+ CPPUNIT_TEST(testFormulaGroupsInCyclesAndWithSelfReference3);
CPPUNIT_TEST_SUITE_END();
private:
@@ -979,6 +981,58 @@ void
ScParallelismTest::testFormulaGroupsInCyclesAndWithSelfReference2()
m_pDoc->DeleteTab(0);
}
+void ScParallelismTest::testFormulaGroupsInCyclesAndWithSelfReference3()
+{
+ sc::AutoCalcSwitch aACSwitch(*m_pDoc, false);
+ m_pDoc->InsertTab(0, "1");
+
+ m_pDoc->SetValue(1, 1, 0, 2.0); // B2 <== 2
+ for (size_t nRow = 1; nRow < 105; ++nRow)
+ {
+ // Formula-group in B3:B104 with first cell "=D2+0.001"
+ if( nRow != 1 )
+ m_pDoc->SetFormula(ScAddress(1, nRow, 0), "=D" +
OUString::number(nRow) + "+0.001",
+ formula::FormulaGrammar::GRAM_NATIVE_UI);
+ // Formula-group in C2:C104 with first cell "=B2*1.01011"
+ m_pDoc->SetFormula(ScAddress(2, nRow, 0), "=B" + OUString::number(nRow
+ 1) + "*1.01011",
+ formula::FormulaGrammar::GRAM_NATIVE_UI);
+ // Formula-group in D2:C104 with first cell "=C2*1.02"
+ m_pDoc->SetFormula(ScAddress(3, nRow, 0), "=C" + OUString::number(nRow
+ 1) + "*1.02",
+ formula::FormulaGrammar::GRAM_NATIVE_UI);
+ }
+
+ m_xDocShell->DoHardRecalc();
+
+ // What happens with tdf#132451 is that the copy&paste C6->C5 really just
sets the dirty flag
+ // for C5 and all the cells that depend on it (D5,B6,C6,D6,B7,...), and it
also resets
+ // flags marking the C formula group as disabled for parallel calculation
because of the cycle.
+ m_pDoc->SetFormula(ScAddress(2, 4, 0), "=B5*1.01011",
formula::FormulaGrammar::GRAM_NATIVE_UI);
+ m_pDoc->GetFormulaCell(ScAddress(2,4,0))->GetCellGroup()->mbPartOfCycle =
false;
+ m_pDoc->GetFormulaCell(ScAddress(2,4,0))->GetCellGroup()->meCalcState =
sc::GroupCalcEnabled;
+
+ m_pDoc->SetAutoCalc(true);
+ // Without the fix, getting value of C5 would try to parallel-interpret
formula group in B
+ // from its first dirty cell (B6), which depends on D5, which depends on
C5, where the cycle
+ // would be detected and dependency check would bail out. But the result
from Interpret()-ing
+ // D5 would be used and D5's dirty flag reset, with D5 value incorrect.
+ m_pDoc->GetValue(2,4,0);
+
+ double fExpected[2][3] = {
+ { 2.19053373572776, 2.21268003179597, 2.25693363243189 },
+ { 2.25793363243189, 2.28076134145577, 2.32637656828489 }
+ };
+ for (size_t nCol = 1; nCol < 4; ++nCol)
+ {
+ for (size_t nRow = 4; nRow < 6; ++nRow)
+ {
+ OString aMsg = "Value at Cell (Col = " + OString::number(nCol) +
", Row = " + OString::number(nRow) + ", Tab = 0)";
+ ASSERT_DOUBLES_EQUAL_MESSAGE(aMsg.getStr(), fExpected[nRow -
4][nCol - 1], m_pDoc->GetValue(nCol, nRow, 0));
+ }
+ }
+
+ m_pDoc->DeleteTab(0);
+}
+
CPPUNIT_TEST_SUITE_REGISTRATION(ScParallelismTest);
CPPUNIT_PLUGIN_IMPLEMENT();
diff --git a/sc/source/core/data/column4.cxx b/sc/source/core/data/column4.cxx
index 1b23ff713ce5..3ca9a2892357 100644
--- a/sc/source/core/data/column4.cxx
+++ b/sc/source/core/data/column4.cxx
@@ -1677,8 +1677,6 @@ static bool
lcl_InterpretSpan(sc::formula_block::const_iterator& rSpanIter, SCRO
// if intergroup dependency is found, return early.
if ((mxParentGroup && mxParentGroup->mbPartOfCycle) ||
!rRecursionHelper.AreGroupsIndependent())
{
- // Set pCellStart as dirty as pCellStart may be interpreted in
InterpretTail()
- pCellStart->SetDirtyVar();
bAllowThreading = false;
return bAnyDirty;
}
diff --git a/sc/source/core/data/formulacell.cxx
b/sc/source/core/data/formulacell.cxx
index 328b881b516c..78f397f0f87b 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -1528,6 +1528,11 @@ bool ScFormulaCell::Interpret(SCROW nStartOffset, SCROW
nEndOffset)
ScRecursionHelper& rRecursionHelper = pDocument->GetRecursionHelper();
bool bGroupInterpreted = false;
+ // The result would possibly depend on a cell without a valid value, bail
out
+ // the entire dependency computation.
+ if (rRecursionHelper.IsAbortingDependencyComputation())
+ return false;
+
if ((mxGroup && !rRecursionHelper.CheckFGIndependence(mxGroup.get())) ||
!rRecursionHelper.AreGroupsIndependent())
return bGroupInterpreted;
@@ -1545,6 +1550,10 @@ bool ScFormulaCell::Interpret(SCROW nStartOffset, SCROW
nEndOffset)
// Reaching here does not necessarily mean a circular reference, so
don't set Err:522 here yet.
// If there is a genuine circular reference, it will be marked so when
all groups
// in the cycle get out of dependency evaluation mode.
+ // But returning without calculation a new value means other cells
depending
+ // on this one would use a possibly invalid value, so ensure the
dependency
+ // computation is aborted without resetting the dirty flag of any cell.
+ rRecursionHelper.AbortDependencyComputation();
return bGroupInterpreted;
}
@@ -1955,6 +1964,12 @@ void ScFormulaCell::InterpretTail( ScInterpreterContext&
rContext, ScInterpretTa
}
bRunning = bOldRunning;
+ // The result may be invalid or depend on another invalid result, just
abort
+ // without updating the cell value. Since the dirty flag will not be
reset,
+ // the proper value will be computed later.
+ if(pDocument->GetRecursionHelper().IsAbortingDependencyComputation())
+ return;
+
// #i102616# For single-sheet saving consider only content changes,
not format type,
// because format type isn't set on loading (might be changed later)
bool bContentChanged = false;
diff --git a/sc/source/core/tool/recursionhelper.cxx
b/sc/source/core/tool/recursionhelper.cxx
index efbf924ab853..d50fca1d87f2 100644
--- a/sc/source/core/tool/recursionhelper.cxx
+++ b/sc/source/core/tool/recursionhelper.cxx
@@ -15,6 +15,7 @@ void ScRecursionHelper::Init()
nRecursionCount = 0;
nDependencyComputationLevel = 0;
bInRecursionReturn = bDoingRecursion = bInIterationReturn = false;
+ bAbortingDependencyComputation = false;
aInsertPos = GetIterationEnd();
ResetIteration();
// Must not force clear aFGList ever.
@@ -195,6 +196,23 @@ void ScRecursionHelper::SetFormulaGroupDepEvalMode(bool
bSet)
aInDependencyEvalMode.back() = bSet;
}
+void ScRecursionHelper::AbortDependencyComputation()
+{
+ assert( nDependencyComputationLevel > 0 );
+ bAbortingDependencyComputation = true;
+}
+
+void ScRecursionHelper::IncDepComputeLevel()
+{
+ ++nDependencyComputationLevel;
+}
+
+void ScRecursionHelper::DecDepComputeLevel()
+{
+ --nDependencyComputationLevel;
+ bAbortingDependencyComputation = false;
+}
+
void ScRecursionHelper::AddTemporaryGroupCell(ScFormulaCell* cell)
{
aTemporaryGroupCells.push_back( cell );
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
