Commit fec7895a authored by Milian Wolff's avatar Milian Wolff
Browse files

Use a recursive mutex for the PersistentSymbolTable

The visitDeclarations and visitFilteredDeclarations might be nested,
which means we have to account for potentially recursive mutex
locking. The kdev-php plugin exhibits this behavior when it calls
`KDevelop::DUContext::findDeclarations`, which might lead to
recursive `visitFilteredDeclarations` within the
`KDevelop::TopDUContext::applyAliases` method. I.e. the deadlock
in the `uses` unit test suite of kdev-php occurred within:

```
(gdb) bt
#0  0x00007ffff2b1959d in syscall () at /usr/lib/libc.so.6
#1  0x00007ffff30dfb06 in QBasicMutex::lockInternal() () at /usr/lib/libQt5Core.so.5
#2  0x00007ffff5c95d87 in QMutexLocker::QMutexLocker(QBasicMutex*) (this=0x7fffffff7808, m=0x7ffff75ede50 <KDevelop::ItemRepositoryFor<KDevelop::PersistentSymbolTable>::repo()::mutex>)
    at /usr/include/qt/QtCore/qmutex.h:238
#3  0x00007ffff5dc0d0e in KDevelop::LockedItemRepository::write<KDevelop::PersistentSymbolTable, KDevelop::PersistentSymbolTable::visitFilteredDeclarations(const KDevelop::IndexedQualifiedIdentifier&, const KDevelop::TopDUContext::IndexedRecursiveImports&, const DeclarationVisitor&) const::<lambda(KDevelop::(anonymous namespace)::PersistentSymbolTableRepo&)> >(struct {...} &&) (op=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:2310
#4  0x00007ffff5dc0df8 in KDevelop::PersistentSymbolTable::visitFilteredDeclarations(KDevelop::IndexedQualifiedIdentifier const&, Utils::StorableSet<KDevelop::IndexedTopDUContext, KDevelop::IndexedTopDUContextIndexConversion, KDevelop::RecursiveImportRepository, true, Utils::DummyLocker> const&, std::function<KDevelop::PersistentSymbolTable::VisitorState (KDevelop::IndexedDeclaration const&)> const&) const (this=0x7ffff77ec5c8 <KDevelop::PersistentSymbolTable::self()::ret>, id=..., visibility=..., visitor=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/persistentsymboltable.cpp:361
#5  0x00007ffff5d2eb11 in KDevelop::TopDUContext::FindDeclarationsAcceptor::operator()(KDevelop::QualifiedIdentifier const&) (this=0x7fffffff8ff0, id=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/topducontext.cpp:688
#6  0x00007ffff5d3542c in KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const (this=0x555555b80260, previous=..., identifier=..., accept=..., position=..., canBeNamespace=false, buddy=0x7fffffff7b00, recursionDepth=1)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/topducontext.cpp:857
#7  0x00007ffff5d34aed in KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const::{lambda(KDevelop::IndexedDeclaration const&)#1}::operator()(KDevelop::IndexedDeclaration const&) const (__closure=0x555555a86fe0, indexedAliasDecl=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/topducontext.cpp:839
#8  0x00007ffff5d3becf in std::__invoke_impl<KDevelop::PersistentSymbolTable::VisitorState, KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const::{lambda(KDevelop::IndexedDeclaration const&)#1}&, KDevelop::IndexedDeclaration const&>(std::__invoke_other, KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const::{lambda(KDevelop::IndexedDeclaration const&)#1}&, KDevelop::IndexedDeclaration const&) (__f=...) at /usr/include/c++/12.2.0/bits/invoke.h:61
#9  0x00007ffff5d3b932 in std::__invoke_r<KDevelop::PersistentSymbolTable::VisitorState, KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const::{lambda(KDevelop::IndexedDeclaration const&)#1}&, KDevelop::IndexedDeclaration const&>(KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const::{lambda(KDevelop::IndexedDeclaration const&)#1}&, KDevelop::IndexedDeclaration const&) (__fn=...) at /usr/include/c++/12.2.0/bits/invoke.h:114
#10 0x00007ffff5d3adbe in std::_Function_handler<KDevelop::PersistentSymbolTable::VisitorState (KDevelop::IndexedDeclaration const&), KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const::{lambda(KDevelop::IndexedDeclaration const&)#1}>::_M_invoke(std::_Any_data const&, KDevelop::IndexedDeclaration const&) (__functor=..., __args#0=...) at /usr/include/c++/12.2.0/bits/std_function.h:290
#11 0x00007ffff5dcf371 in std::function<KDevelop::PersistentSymbolTable::VisitorState (KDevelop::IndexedDeclaration const&)>::operator()(KDevelop::IndexedDeclaration const&) const
    (this=0x7fffffff8ed0, __args#0=...) at /usr/include/c++/12.2.0/bits/std_function.h:591
#12 0x00007ffff5dc0c5d in operator()(KDevelop::(anonymous namespace)::PersistentSymbolTableRepo&) const (__closure=0x7fffffff8d70, repo=...)
#13 0x00007ffff5dc0d21 in KDevelop::LockedItemRepository::write<KDevelop::PersistentSymbolTable, KDevelop::PersistentSymbolTable::visitFilteredDeclarations(const KDevelop::IndexedQualifiedIdentifier&, const KDevelop::TopDUContext::IndexedRecursiveImports&, const DeclarationVisitor&) const::<lambda(KDevelop::(anonymous namespace)::PersistentSymbolTableRepo&)> >(struct {...} &&) (op=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:2311
#14 0x00007ffff5dc0df8 in KDevelop::PersistentSymbolTable::visitFilteredDeclarations(KDevelop::IndexedQualifiedIdentifier const&, Utils::StorableSet<KDevelop::IndexedTopDUContext, KDevelop::IndexedTopDUContextIndexConversion, KDevelop::RecursiveImportRepository, true, Utils::DummyLocker> const&, std::function<KDevelop::PersistentSymbolTable::VisitorState (KDevelop::IndexedDeclaration const&)> const&) const (this=0x7ffff77ec5c8 <KDevelop::PersistentSymbolTable::self()::ret>, id=..., visibility=..., visitor=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/persistentsymboltable.cpp:361
#15 0x00007ffff5d35372 in KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevelop::QualifiedIdentifier const&, QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool, KDevelop::TopDUContext::ApplyAliasesBuddyInfo*, unsigned int) const (this=0x555555b80260, previous=..., identifier=..., accept=..., position=..., canBeNamespace=false, buddy=0x0, recursionDepth=0)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/topducontext.cpp:795
#16 0x00007ffff5d30e98 in KDevelop::TopDUContext::applyAliases<KDevelop::TopDUContext::FindDeclarationsAcceptor>(KDevVarLengthArray<QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem>, 256> const&, KDevelop::TopDUContext::FindDeclarationsAcceptor&, KDevelop::CursorInRevision const&, bool) const
    (this=0x555555b80260, identifiers=..., acceptor=..., position=..., canBeNamespace=false) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/topducontext.cpp:942
#17 0x00007ffff5d2b2fe in KDevelop::TopDUContext::findDeclarationsInternal(KDevVarLengthArray<QExplicitlySharedDataPointer<KDevelop::DUContext::SearchItem>, 256> const&, KDevelop::CursorInRevision const&, KDevelop::TypePtr<KDevelop::AbstractType> const&, QList<KDevelop::Declaration*>&, KDevelop::TopDUContext const*, QFlags<KDevelop::DUContext::SearchFlag>, unsigned int) const (this=0x555555b80260, identifiers=..., position=..., dataType=..., ret=QList<KDevelop::Declaration *> (size = 0), flags=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/topducontext.cpp:724
#18 0x00007ffff5d00345 in KDevelop::DUContext::findDeclarations(KDevelop::QualifiedIdentifier const&, KDevelop::CursorInRevision const&, KDevelop::TypePtr<KDevelop::AbstractType> const&, KDevelop::TopDUContext const*, QFlags<KDevelop::DUContext::SearchFlag>) const (this=0x555555b80260, identifier=..., position=..., dataType=..., topContext=0x0, flags=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/ducontext.cpp:793
...
```

This patch here fixes that deadlock. To prevent unexpected API
abuse, we do add some checks to ensure we don't mutate the symbol
table from within a visitor.
parent b2c451fa
......@@ -23,6 +23,14 @@
#include <language/util/setrepository.h>
#if defined(QT_NO_DEBUG) && !defined(QT_FORCE_ASSERTS)
#define VERIFY_VISIT_NESTING 0
#define ifVerifyVisitNesting(x)
#else
#define VERIFY_VISIT_NESTING 1
#define ifVerifyVisitNesting(x) x
#endif
namespace KDevelop {
namespace {
......@@ -181,7 +189,8 @@ using FilteredDeclarationIterator =
CachedIndexedRecursiveImports, DeclarationTopContextExtractor>;
// Maps declaration-ids to declarations, together with some caches
class PersistentSymbolTableRepo : public ItemRepository<PersistentSymbolTableItem, PersistentSymbolTableRequestItem>
class PersistentSymbolTableRepo
: public ItemRepository<PersistentSymbolTableItem, PersistentSymbolTableRequestItem, true, QRecursiveMutex>
{
using ItemRepository::ItemRepository;
......@@ -191,7 +200,32 @@ public:
// We cache the imports so the currently used nodes are very close in memory, which leads to much better CPU cache
// utilization
QHash<TopDUContext::IndexedRecursiveImports, CachedIndexedRecursiveImports> importsCache;
/// Counts how many recursive calls to PersistentSymbolTable::visit* functions are ongoing
/// and hold the repository's mutex lock. Is used only to assert correct API use.
mutable uint ongoingIterations = 0;
};
#if VERIFY_VISIT_NESTING
struct IterationCounter
{
explicit IterationCounter(const PersistentSymbolTableRepo& repo)
: repo(repo)
{
++repo.ongoingIterations;
}
~IterationCounter()
{
--repo.ongoingIterations;
}
private:
Q_DISABLE_COPY_MOVE(IterationCounter)
const PersistentSymbolTableRepo& repo;
};
#endif
}
template<>
......@@ -200,7 +234,7 @@ class ItemRepositoryFor<PersistentSymbolTable>
friend struct LockedItemRepository;
static PersistentSymbolTableRepo& repo()
{
static QMutex mutex;
static QRecursiveMutex mutex;
static PersistentSymbolTableRepo repo { QStringLiteral("Persistent Declaration Table"), &mutex };
return repo;
}
......@@ -209,6 +243,8 @@ class ItemRepositoryFor<PersistentSymbolTable>
void PersistentSymbolTable::clearCache()
{
LockedItemRepository::write<PersistentSymbolTable>([](PersistentSymbolTableRepo& repo) {
Q_ASSERT_X(repo.ongoingIterations == 0, Q_FUNC_INFO, "don't call clearCache directly from a visitor");
repo.importsCache.clear();
repo.declarationsCache.clear();
});
......@@ -232,6 +268,8 @@ void PersistentSymbolTable::addDeclaration(const IndexedQualifiedIdentifier& id,
item.id = id;
LockedItemRepository::write<PersistentSymbolTable>([&item, &declaration](PersistentSymbolTableRepo& repo) {
Q_ASSERT_X(repo.ongoingIterations == 0, Q_FUNC_INFO, "don't call addDeclaration directly from a visitor");
repo.declarationsCache.remove(item.id);
uint index = repo.findIndex(item);
......@@ -282,6 +320,8 @@ void PersistentSymbolTable::removeDeclaration(const IndexedQualifiedIdentifier&
item.id = id;
LockedItemRepository::write<PersistentSymbolTable>([&item, &declaration](PersistentSymbolTableRepo& repo) {
Q_ASSERT_X(repo.ongoingIterations == 0, Q_FUNC_INFO, "don't call removeDeclaration directly from a visitor");
repo.declarationsCache.remove(item.id);
uint index = repo.findIndex(item);
......@@ -331,6 +371,8 @@ void PersistentSymbolTable::visitDeclarations(const IndexedQualifiedIdentifier&
item.id = id;
LockedItemRepository::read<PersistentSymbolTable>([&item, &visitor](const PersistentSymbolTableRepo& repo) {
ifVerifyVisitNesting(const auto guard = IterationCounter(repo);)
uint index = repo.findIndex(item);
if (!index) {
......@@ -359,6 +401,8 @@ void PersistentSymbolTable::visitFilteredDeclarations(const IndexedQualifiedIden
item.id = id;
LockedItemRepository::write<PersistentSymbolTable>([&](PersistentSymbolTableRepo& repo) {
ifVerifyVisitNesting(const auto guard = IterationCounter(repo);)
uint index = repo.findIndex(item);
if (!index) {
return;
......@@ -480,6 +524,8 @@ void PersistentSymbolTable::dump(const QTextStream& out)
DebugVisitor v(out);
LockedItemRepository::read<PersistentSymbolTable>([&](const PersistentSymbolTableRepo& repo) {
Q_ASSERT_X(repo.ongoingIterations == 0, Q_FUNC_INFO, "don't call dump directly from a visitor");
repo.visitAllItems(v);
qout << "Statistics:" << Qt::endl;
......
......@@ -41,7 +41,8 @@ public:
Break,
Continue,
};
///@warning The visitor must not call any other PersistentSymbolTable API as that would deadlock
///@warning The visitor must not call any PersistentSymbolTable API that would mutate its contents
/// i.e. do not call `addDeclaration` or `removeDeclaration` directly from the visitor.
using DeclarationVisitor = std::function<VisitorState(const IndexedDeclaration&)>;
/// Iterate over all the declarations for a given IndexedQualifiedIdentifier in an efficient way.
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment