tdf#156677 if CheckComputeDependencies failed in expanded range...
it may have left a cell the original range needed in a dirty state,
leading to an assert when the threaded calculation is attempted which
would not have happened if the attempt to expand the range wasn't
attempted.
seen in crashtesting example: forum-mso-en4-784502.xlsx
to reproduce this, load sample document
after it has loaded:
(gdb) break column4.cxx:1946
(gdb) break column4.cxx:1966
data, recalculate, recalculate hard,
the first time breakpoint #1 gets hit:
at column4.cxx:1946 NeedsInterpret is true, so cell
{nRow = 1, nCol = 0, nTab = 3 } was dirty and is set
not-dirty by Interpret. mxGroup->mbPartOfCycle is false
so this returns successfully to allow threading.
the bt to there was:
#0 lcl_EvalDirty(mdds::mtv::soa::multi_type_vector<sc::CellStoreTraits>&, int, int, ScDocument&, boost::intrusive_ptr<ScFormulaCellGroup> const&, bool, bool, bool&, bool&)
(rCells=..., nRow1=1, nRow2=4, rDoc=..., mxGroup=..., bThreadingDepEval=true, bSkipRunning=false, bIsDirty=@0x7fffffff4a16: true, bAllowThreading=@0x7fffffff4a17: true)
at sc/source/core/data/column4.cxx:1963
#1 0x00007fff9eef70d1 in ScColumn::HandleRefArrayForParallelism(int, int, boost::intrusive_ptr<ScFormulaCellGroup> const&) (this=0x15ee480, nRow1=1, nRow2=4, mxGroup=...)
at sc/source/core/data/column4.cxx:2012
#2 0x00007fff9f44448a in ScTable::HandleRefArrayForParallelism(short, int, int, boost::intrusive_ptr<ScFormulaCellGroup> const&)
(this=0x1eebda0, nCol=0, nRow1=1, nRow2=4, mxGroup=...) at sc/source/core/data/table1.cxx:2567
#3 0x00007fff9f06b691 in ScDocument::HandleRefArrayForParallelism(ScAddress const&, int, boost::intrusive_ptr<ScFormulaCellGroup> const&)
(this=0x1ba9a40, rPos=..., nLength=4, mxGroup=...) at sc/source/core/data/document.cxx:1792
#4 0x00007fff9f3018f7 in (anonymous namespace)::ScDependantsCalculator::DoIt() (this=0x7fffffff4eb8) at sc/source/core/data/formulacell.cxx:4585
#5 0x00007fff9f30085a in ScFormulaCell::CheckComputeDependencies(sc::FormulaLogger::GroupScope&, bool, int, int, bool)
(this=0x2142cf0, rScope=..., fromFirstRow=false, nStartOffset=0, nEndOffset=3, bCalcDependencyOnly=false) at sc/source/core/data/formulacell.cxx:4720
#6 0x00007fff9f2ff392 in ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope&, bool&, bool&, int, int)
(this=0x2142cf0, aScope=..., bDependencyComputed=@0x7fffffff56d7: false, bDependencyCheckFailed=@0x7fffffff56d6: false, nStartOffset=0, nEndOffset=3)
at sc/source/core/data/formulacell.cxx:4829
so the CheckComputeDependencies at the start of
ScFormulaCell::InterpretFormulaGroupThreading is successful for aPos of
{nRow = 1, nCol = 1, nTab = 3 } and the cell is not dirty at that point
*however* in the following loop of
for (SCCOL nCurrCol = nColStart; nCurrCol <= nColEnd; ++nCurrCol)
in InterpretFormulaGroupThreading, CheckComputeDependencies for
column 3 is called and the breakpoint #2 is hit, in this case
mxGroup->mbPartOfCycle is true and the {nRow = 1, nCol = 0, nTab = 3 }
cell is set dirty again.
so later during the threaded calculation the cell is found dirty and the
ScFormulaCell::MaybeInterpret() asserts that
!rDocument.IsThreadedGroupCalcInProgress()
Change-Id: I40385f5e8240680c220249dd2966b196efaf5e0f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/155463
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>
diff --git a/sc/inc/column.hxx b/sc/inc/column.hxx
index fe3b848..0f30f64 100644
--- a/sc/inc/column.hxx
+++ b/sc/inc/column.hxx
@@ -648,7 +648,8 @@ public:
bool ResolveStaticReference( ScMatrix& rMat, SCCOL nMatCol, SCROW nRow1, SCROW nRow2 );
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 );
bool HandleRefArrayForParallelism( SCROW nRow1, SCROW nRow2,
const ScFormulaCellGroupRef& mxGroup, ScAddress* pDirtiedAddress );
#ifdef DBG_UTIL
void AssertNoInterpretNeeded( SCROW nRow1, SCROW nRow2 );
#endif
diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index 157a490..5e9163b 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -2574,7 +2574,8 @@ public:
formula::FormulaTokenRef ResolveStaticReference( const ScRange& rRange );
formula::VectorRefArray FetchVectorRefArray( const ScAddress& rPos, SCROW nLength );
bool HandleRefArrayForParallelism( const ScAddress& rPos, SCROW nLength, const ScFormulaCellGroupRef& mxGroup );
bool HandleRefArrayForParallelism( const ScAddress& rPos, SCROW nLength,
const ScFormulaCellGroupRef& mxGroup, ScAddress* pDirtiedAddress );
#ifdef DBG_UTIL
void AssertNoInterpretNeeded( const ScAddress& rPos, SCROW nLength );
#endif
diff --git a/sc/inc/formulacell.hxx b/sc/inc/formulacell.hxx
index 42b7f61..70f113d 100644
--- a/sc/inc/formulacell.hxx
+++ b/sc/inc/formulacell.hxx
@@ -160,7 +160,9 @@ private:
ScFormulaCell( const ScFormulaCell& ) = delete;
bool CheckComputeDependencies(sc::FormulaLogger::GroupScope& rScope, bool fromFirstRow,
SCROW nStartOffset, SCROW nEndOffset, bool bCalcDependencyOnly = false);
SCROW nStartOffset, SCROW nEndOffset, bool bCalcDependencyOnly = false,
ScRangeList* pSuccessfulDependencies = nullptr,
ScAddress* pFailedAndDirtiedAddress = nullptr);
bool InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope& aScope,
bool& bDependencyComputed,
bool& bDependencyCheckFailed,
diff --git a/sc/inc/table.hxx b/sc/inc/table.hxx
index 1742aa4..615a2f0 100644
--- a/sc/inc/table.hxx
+++ b/sc/inc/table.hxx
@@ -1074,7 +1074,8 @@ public:
formula::FormulaTokenRef ResolveStaticReference( SCCOL nCol, SCROW nRow );
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 );
bool HandleRefArrayForParallelism( SCCOL nCol, SCROW nRow1, SCROW nRow2,
const ScFormulaCellGroupRef& mxGroup, ScAddress* pDirtiedAddress );
#ifdef DBG_UTIL
void AssertNoInterpretNeeded( SCCOL nCol, SCROW nRow1, SCROW nRow2 );
#endif
diff --git a/sc/source/core/data/column4.cxx b/sc/source/core/data/column4.cxx
index 412e5b2..a1423b2 100644
--- a/sc/source/core/data/column4.cxx
+++ b/sc/source/core/data/column4.cxx
@@ -1858,7 +1858,7 @@ static bool lcl_InterpretSpan(sc::formula_block::const_iterator& rSpanIter, SCRO
static void lcl_EvalDirty(sc::CellStoreType& rCells, SCROW nRow1, SCROW nRow2, ScDocument& rDoc,
const ScFormulaCellGroupRef& mxGroup, bool bThreadingDepEval, bool bSkipRunning,
bool& bIsDirty, bool& bAllowThreading)
bool& bIsDirty, bool& bAllowThreading, ScAddress* pDirtiedAddress)
{
ScRecursionHelper& rRecursionHelper = rDoc.GetRecursionHelper();
std::pair<sc::CellStoreType::const_iterator,size_t> aPos = rCells.position(nRow1);
@@ -1964,6 +1964,8 @@ static void lcl_EvalDirty(sc::CellStoreType& rCells, SCROW nRow1, SCROW nRow2, S
{
// Set itCell as dirty as itCell may be interpreted in InterpretTail()
(*itCell)->SetDirtyVar();
if (pDirtiedAddress)
pDirtiedAddress->SetRow(nRow);
bAllowThreading = false;
return;
}
@@ -1999,17 +2001,17 @@ bool ScColumn::EnsureFormulaCellResults( SCROW nRow1, SCROW nRow2, bool bSkipRun
return false;
bool bAnyDirty = false, bTmp = false;
lcl_EvalDirty(maCells, nRow1, nRow2, GetDoc(), nullptr, false, bSkipRunning, bAnyDirty, bTmp);
lcl_EvalDirty(maCells, nRow1, nRow2, GetDoc(), nullptr, false, bSkipRunning, bAnyDirty, bTmp, nullptr);
return bAnyDirty;
}
bool ScColumn::HandleRefArrayForParallelism( SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup )
bool ScColumn::HandleRefArrayForParallelism( SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup, ScAddress* pDirtiedAddress )
{
if (nRow1 > nRow2)
return false;
bool bAllowThreading = true, bTmp = false;
lcl_EvalDirty(maCells, nRow1, nRow2, GetDoc(), mxGroup, true, false, bTmp, bAllowThreading);
lcl_EvalDirty(maCells, nRow1, nRow2, GetDoc(), mxGroup, true, false, bTmp, bAllowThreading, pDirtiedAddress);
return bAllowThreading;
}
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index cc6ee12..0c383ea 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -1785,11 +1785,19 @@ void ScDocument::UnlockAdjustHeight()
--nAdjustHeightLock;
}
bool ScDocument::HandleRefArrayForParallelism( const ScAddress& rPos, SCROW nLength, const ScFormulaCellGroupRef& mxGroup )
bool ScDocument::HandleRefArrayForParallelism( const ScAddress& rPos, SCROW nLength, const ScFormulaCellGroupRef& mxGroup, ScAddress* pDirtiedAddress)
{
SCTAB nTab = rPos.Tab();
if (ScTable* pTable = FetchTable(nTab))
return pTable->HandleRefArrayForParallelism(rPos.Col(), rPos.Row(), rPos.Row()+nLength-1, mxGroup);
{
bool bRet = pTable->HandleRefArrayForParallelism(rPos.Col(), rPos.Row(), rPos.Row()+nLength-1, mxGroup, pDirtiedAddress);
if (!bRet && pDirtiedAddress && pDirtiedAddress->Row() != -1)
{
pDirtiedAddress->SetCol(rPos.Col());
pDirtiedAddress->SetTab(nTab);
}
return bRet;
}
return false;
}
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index 4049145..4b97483 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -4432,7 +4432,7 @@ struct ScDependantsCalculator
return nRowLen;
}
bool DoIt()
bool DoIt(ScRangeList* pSuccessfulDependencies, ScAddress* pDirtiedAddress)
{
// Partially from ScGroupTokenConverter::convert in sc/source/core/data/grouptokenconverter.cxx
@@ -4583,7 +4583,7 @@ struct ScDependantsCalculator
nStartRow = 0;
}
if (!mrDoc.HandleRefArrayForParallelism(ScAddress(nCol, nStartRow, rRange.aStart.Tab()),
nLength, mxGroup))
nLength, mxGroup, pDirtiedAddress))
return false;
}
}
@@ -4591,6 +4591,9 @@ struct ScDependantsCalculator
if (bHasSelfReferences)
mxGroup->mbPartOfCycle = true;
if (pSuccessfulDependencies && !bHasSelfReferences)
*pSuccessfulDependencies = aRangeList;
return !bHasSelfReferences;
}
};
@@ -4688,7 +4691,9 @@ bool ScFormulaCell::InterpretFormulaGroup(SCROW nStartOffset, SCROW nEndOffset)
bool ScFormulaCell::CheckComputeDependencies(sc::FormulaLogger::GroupScope& rScope, bool fromFirstRow,
SCROW nStartOffset, SCROW nEndOffset,
bool bCalcDependencyOnly)
bool bCalcDependencyOnly,
ScRangeList* pSuccessfulDependencies,
ScAddress* pDirtiedAddress)
{
ScRecursionHelper& rRecursionHelper = rDocument.GetRecursionHelper();
// iterate over code in the formula ...
@@ -4701,7 +4706,7 @@ bool ScFormulaCell::CheckComputeDependencies(sc::FormulaLogger::GroupScope& rSco
// (We can only reach here from a multi-group dependency evaluation attempt).
// (These two have to be in pairs always for any given formula-group)
ScDependantsCalculator aCalculator(rDocument, *pCode, *this, mxGroup->mpTopCell->aPos, fromFirstRow, nStartOffset, nEndOffset);
return aCalculator.DoIt();
return aCalculator.DoIt(pSuccessfulDependencies, pDirtiedAddress);
}
bool bOKToParallelize = false;
@@ -4716,7 +4721,7 @@ bool ScFormulaCell::CheckComputeDependencies(sc::FormulaLogger::GroupScope& rSco
ScFormulaGroupDependencyComputeGuard aDepComputeGuard(rRecursionHelper);
ScDependantsCalculator aCalculator(rDocument, *pCode, *this, mxGroup->mpTopCell->aPos, fromFirstRow, nStartOffset, nEndOffset);
bOKToParallelize = aCalculator.DoIt();
bOKToParallelize = aCalculator.DoIt(pSuccessfulDependencies, pDirtiedAddress);
}
@@ -4822,7 +4827,8 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
pCode->IsEnabledForThreading() &&
ScCalcConfig::isThreadingEnabled())
{
if(!bDependencyComputed && !CheckComputeDependencies(aScope, false, nStartOffset, nEndOffset))
ScRangeList aOrigDependencies;
if(!bDependencyComputed && !CheckComputeDependencies(aScope, false, nStartOffset, nEndOffset, false, &aOrigDependencies))
{
bDependencyComputed = true;
bDependencyCheckFailed = true;
@@ -4899,6 +4905,8 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
nColEnd = lcl_probeLeftOrRightFGs(mxGroup, rDocument, aFGSet, aFGMap, false);
}
bool bFGOK = true;
ScAddress aDirtiedAddress(ScAddress::INITIALIZE_INVALID);
if (nColStart != nColEnd)
{
ScCheckIndependentFGGuard aGuard(rRecursionHelper, &aFGSet);
@@ -4907,7 +4915,8 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
if (nCurrCol == aPos.Col())
continue;
bool bFGOK = aFGMap[nCurrCol]->CheckComputeDependencies(aScope, false, nStartOffset, nEndOffset, true);
bFGOK = aFGMap[nCurrCol]->CheckComputeDependencies(aScope, false, nStartOffset, nEndOffset,
true, nullptr, &aDirtiedAddress);
if (!bFGOK || !aGuard.AreGroupsIndependent())
{
nColEnd = nColStart = aPos.Col();
@@ -4916,6 +4925,17 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
}
}
// tdf#156677 it is possible that if a check of a column in the new range fails that the check has
// now left a cell that the original range depended on in a Dirty state. So if the dirtied cell
// was part of the original dependencies re-run the initial CheckComputeDependencies to fix it.
if (!bFGOK && aDirtiedAddress.IsValid() && aOrigDependencies.Find(aDirtiedAddress))
{
SAL_WARN("sc.core.formulacell", "rechecking dependencies due to a dirtied cell during speculative probe");
const bool bRedoEntryCheckSucceeded = CheckComputeDependencies(aScope, false, nStartOffset, nEndOffset);
assert(bRedoEntryCheckSucceeded && "if it worked on the original range it should work again on that range");
(void)bRedoEntryCheckSucceeded;
}
std::vector<std::unique_ptr<ScInterpreter>> aInterpreters(nThreadCount);
{
assert(!rDocument.IsThreadedGroupCalcInProgress());
diff --git a/sc/source/core/data/table1.cxx b/sc/source/core/data/table1.cxx
index f6df907..4c0cf3e 100644
--- a/sc/source/core/data/table1.cxx
+++ b/sc/source/core/data/table1.cxx
@@ -2551,7 +2551,7 @@ void ScTable::AssertNoInterpretNeeded( SCCOL nCol, SCROW nRow1, SCROW nRow2 )
}
#endif
bool ScTable::HandleRefArrayForParallelism( SCCOL nCol, SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup )
bool ScTable::HandleRefArrayForParallelism( SCCOL nCol, SCROW nRow1, SCROW nRow2, const ScFormulaCellGroupRef& mxGroup, ScAddress* pDirtiedAddress )
{
if (nRow2 < nRow1)
return false;
@@ -2564,7 +2564,7 @@ bool ScTable::HandleRefArrayForParallelism( SCCOL nCol, SCROW nRow1, SCROW nRow2
mpFilteredCols->makeReady();
mpFilteredRows->makeReady();
return aCol[nCol].HandleRefArrayForParallelism(nRow1, nRow2, mxGroup);
return aCol[nCol].HandleRefArrayForParallelism(nRow1, nRow2, mxGroup, pDirtiedAddress);
}
ScRefCellValue ScTable::GetRefCellValue( SCCOL nCol, SCROW nRow )
