[Libreoffice-commits] core.git: Branch 'distro/collabora/cp-6.4' - sc/inc sc/qa sc/source
Luboš Luňák (via logerrit)
logerrit at kemper.freedesktop.org
Mon Jun 22 16:39:29 UTC 2020
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 8cd6f7ae48f5795778de512b2867a8ed3374ef12
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Mon Jun 22 11:42:22 2020 +0200
Commit: Dennis Francis <dennis.francis at collabora.com>
CommitDate: Mon Jun 22 18:38:54 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.francis at collabora.com>
Tested-by: Jenkins
(cherry picked from commit e1b6dcc97d6b5ff15b2e9341d3943ffc05aa3236)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/96802
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice at gmail.com>
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 );
More information about the Libreoffice-commits
mailing list