sc/inc/column.hxx | 3 ++-
sc/inc/document.hxx | 3 ++-
sc/inc/formulacell.hxx | 4 +++-
sc/inc/table.hxx | 3 ++-
sc/source/core/data/column4.cxx | 10 ++++++----
sc/source/core/data/document.cxx | 12 ++++++++++--
sc/source/core/data/formulacell.cxx | 34 +++++++++++++++++++++++++++-------
sc/source/core/data/table1.cxx | 4 ++--
8 files changed, 54 insertions(+), 19 deletions(-)
New commits:
commit 4403b4e6bac19d89afded080d80de049aaa294ca
Author: Caolán McNamara <caolan.mcnam...@collabora.com>
AuthorDate: Tue Aug 8 17:02:40 2023 +0100
Commit: Caolán McNamara <caolan.mcnam...@collabora.com>
CommitDate: Mon Aug 14 10:32:16 2023 +0200
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.mcnam...@collabora.com>
diff --git a/sc/inc/column.hxx b/sc/inc/column.hxx
index fe3b84842af2..0f30f64cb672 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 157a49099184..5e9163b28fd4 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 42b7f6120149..70f113d50715 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 1742aa4a43fd..615a2f03e5cd 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 412e5b2e9627..a1423b2e1c67 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 cc6ee1236098..0c383ea12c99 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 404914565efd..4b974831992b 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 f6df90738c72..4c0cf3e02a9a 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 )
