diff options
author | Luboš Luňák <l.lunak@collabora.com> | 2020-06-22 11:42:22 +0200 |
---|---|---|
committer | Dennis Francis <dennis.francis@collabora.com> | 2020-06-22 18:38:54 +0200 |
commit | 8cd6f7ae48f5795778de512b2867a8ed3374ef12 (patch) | |
tree | 00657cf9541d2666fd9771cdcb13a662068dceea /sc | |
parent | e428ca117b0510ac7b0fbbf233bd940c8cc464fd (diff) |
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.francis@collabora.com>
Tested-by: Jenkins
(cherry picked from commit e1b6dcc97d6b5ff15b2e9341d3943ffc05aa3236)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96802
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Diffstat (limited to 'sc')
-rw-r--r-- | sc/inc/recursionhelper.hxx | 10 | ||||
-rw-r--r-- | sc/qa/unit/parallelism.cxx | 54 | ||||
-rw-r--r-- | sc/source/core/data/column4.cxx | 2 | ||||
-rw-r--r-- | sc/source/core/data/formulacell.cxx | 15 | ||||
-rw-r--r-- | sc/source/core/tool/recursionhelper.cxx | 18 |
5 files changed, 95 insertions, 4 deletions
diff --git a/sc/inc/recursionhelper.hxx b/sc/inc/recursionhelper.hxx index 9988a676203b..477eb2a4d00e 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; std::unordered_set< 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 329fd93eecdb..a06dc0f0d939 100644 --- a/sc/source/core/data/column4.cxx +++ b/sc/source/core/data/column4.cxx @@ -1676,8 +1676,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 acfe85bd332b..dff86ec3682e 100644 --- a/sc/source/core/data/formulacell.cxx +++ b/sc/source/core/data/formulacell.cxx @@ -1517,6 +1517,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; @@ -1534,6 +1539,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; } @@ -1941,6 +1950,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 a13c60edf6fb..a7a8abb3b074 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 ); |