Commit 56f3cc14 authored by Milian Wolff's avatar Milian Wolff
Browse files

Refactor the PersistentSymbolTable API to prevent unsafe access

Instead of handing out raw iterators to data that is stored within
the symbol table, force users to pass a visitor instead.

This has the nice side effect that we can hide way more of the
persistent symbol table internals. We can now also get rid of the
extra mutex for the caches, as we only touch them while we have
the repository mutex locked.

The downside is that the callee loops are all a bit more tedious
to write, as we can't just `break` or `continue` directly anymore
and instead have to return a special `Break` or `Continue` value
from the visitor instead.

Fixes: #10
parent 5a246795
......@@ -267,25 +267,23 @@ bool DocumentClassesFolder::updateDocument(const KDevelop::IndexedString& a_file
DUChainReadLocker readLock(DUChain::lock());
uint declsCount = 0;
const IndexedDeclaration* decls;
PersistentSymbolTable::self().declarations(parentIdentifier, declsCount, decls);
for (uint i = 0; i < declsCount; ++i) {
// Look for the first valid declaration.
if (auto decl = decls[i].declaration()) {
// See if it should be namespaced.
if (decl->kind() == Declaration::Namespace) {
// This should create the namespace folder and add it to the cache.
parentNode = namespaceFolder(parentIdentifier);
// Add to the locally created namespaces.
declaredNamespaces.insert(parentIdentifier);
PersistentSymbolTable::self().visitDeclarations(
parentIdentifier, [&](const IndexedDeclaration& indexedDeclaration) {
// Look for the first valid declaration.
if (auto declaration = indexedDeclaration.declaration()) {
// See if it should be namespaced.
if (declaration->kind() == Declaration::Namespace) {
// This should create the namespace folder and add it to the cache.
parentNode = namespaceFolder(parentIdentifier);
// Add to the locally created namespaces.
declaredNamespaces.insert(parentIdentifier);
}
return PersistentSymbolTable::VisitorState::Break;
}
break;
}
}
return PersistentSymbolTable::VisitorState::Continue;
});
}
} else
{
......@@ -297,16 +295,15 @@ bool DocumentClassesFolder::updateDocument(const KDevelop::IndexedString& a_file
if (parentNode != nullptr) {
// Create the new node and add it.
IndexedDeclaration decl;
uint count = 0;
const IndexedDeclaration* declarations;
DUChainReadLocker lock;
PersistentSymbolTable::self().declarations(item.id, count, declarations);
for (uint i = 0; i < count; ++i) {
if (declarations[i].indexedTopContext().url() == a_file) {
decl = declarations[i];
break;
}
}
PersistentSymbolTable::self().visitDeclarations(
item.id, [&](const IndexedDeclaration& indexedDeclaration) {
if (indexedDeclaration.indexedTopContext().url() == a_file) {
decl = indexedDeclaration;
return PersistentSymbolTable::VisitorState::Break;
}
return PersistentSymbolTable::VisitorState::Continue;
});
if (decl.isValid()) {
newNode = new ClassNode(decl.declaration(), m_model);
......
......@@ -319,25 +319,25 @@ void TemplateClassGenerator::addBaseClass(const QString& base)
cd.baseClasses << desc;
setDescription(cd);
DUChainReadLocker lock;
PersistentSymbolTable::Declarations decl =
PersistentSymbolTable::self().declarations(IndexedQualifiedIdentifier(QualifiedIdentifier(desc.baseType)));
//Search for all super classes
for (PersistentSymbolTable::Declarations::Iterator it = decl.iterator(); it; ++it) {
DeclarationPointer declaration = DeclarationPointer(it->declaration());
auto visitor = [&](const IndexedDeclaration& indexedDeclaration) {
auto declaration = DeclarationPointer(indexedDeclaration.declaration());
if (!declaration || declaration->isForwardDeclaration()) {
continue;
return PersistentSymbolTable::VisitorState::Continue;
}
// Check if it's a class/struct/etc
if (declaration->type<StructureType>()) {
d->fetchSuperClasses(declaration);
d->directBaseClasses << declaration;
break;
return PersistentSymbolTable::VisitorState::Break;
}
}
return PersistentSymbolTable::VisitorState::Continue;
};
const auto id = IndexedQualifiedIdentifier(QualifiedIdentifier(desc.baseType));
DUChainReadLocker lock;
PersistentSymbolTable::self().visitDeclarations(id, visitor);
}
void TemplateClassGenerator::setBaseClasses(const QList<QString>& bases)
......
......@@ -90,35 +90,29 @@ KDevVarLengthArray<Declaration*> DeclarationId::declarations(const TopDUContext*
//Find the declaration by its qualified identifier and additionalIdentity
QualifiedIdentifier id(m_indirectData.identifier);
auto visitDeclaration = [&](const IndexedDeclaration& indexedDecl) {
auto decl = indexedDecl.data();
if (decl && m_indirectData.additionalIdentity == decl->additionalIdentity()) {
// Hit
ret.append(decl);
}
return PersistentSymbolTable::VisitorState::Continue;
};
if (top) {
//Do filtering
PersistentSymbolTable::FilteredDeclarationIterator filter =
PersistentSymbolTable::self().filteredDeclarations(id, top->recursiveImportIndices());
for (; filter; ++filter) {
Declaration* decl = filter->data();
if (decl && m_indirectData.additionalIdentity == decl->additionalIdentity()) {
//Hit
ret.append(decl);
}
}
PersistentSymbolTable::self().visitFilteredDeclarations(id, top->recursiveImportIndices(),
visitDeclaration);
} else {
//Just accept anything
PersistentSymbolTable::Declarations decls = PersistentSymbolTable::self().declarations(id);
PersistentSymbolTable::Declarations::Iterator decl = decls.iterator();
for (; decl; ++decl) {
const IndexedDeclaration& iDecl(*decl);
PersistentSymbolTable::self().visitDeclarations(id, [&](const IndexedDeclaration& indexedDecl) {
///@todo think this over once we don't pull in all imported top-context any more
//Don't trigger loading of top-contexts from here, it will create a lot of problems
if ((!DUChain::self()->isInMemory(iDecl.topContextIndex())))
continue;
Declaration* decl = iDecl.data();
if (decl && m_indirectData.additionalIdentity == decl->additionalIdentity()) {
//Hit
ret.append(decl);
if (DUChain::self()->isInMemory(indexedDecl.topContextIndex())) {
return visitDeclaration(indexedDecl);
}
}
return PersistentSymbolTable::VisitorState::Continue;
});
}
} else {
Declaration* decl = m_directData.declaration();
......@@ -147,39 +141,32 @@ Declaration* DeclarationId::declaration(const TopDUContext* top, bool instantiat
//Find the declaration by its qualified identifier and additionalIdentity
QualifiedIdentifier id(m_indirectData.identifier);
if (top) {
//Do filtering
PersistentSymbolTable::FilteredDeclarationIterator filter =
PersistentSymbolTable::self().filteredDeclarations(id, top->recursiveImportIndices());
for (; filter; ++filter) {
Declaration* decl = filter->data();
if (decl && m_indirectData.additionalIdentity == decl->additionalIdentity()) {
//Hit
ret = decl;
if (!ret->isForwardDeclaration())
break;
auto visitDeclaration = [&](const IndexedDeclaration& indexedDecl) {
auto decl = indexedDecl.data();
if (decl && m_indirectData.additionalIdentity == decl->additionalIdentity()) {
// Hit
ret = decl;
if (!ret->isForwardDeclaration()) {
return PersistentSymbolTable::VisitorState::Break;
}
}
return PersistentSymbolTable::VisitorState::Continue;
};
if (top) {
// Do filtering
PersistentSymbolTable::self().visitFilteredDeclarations(id, top->recursiveImportIndices(),
visitDeclaration);
} else {
//Just accept anything
PersistentSymbolTable::Declarations decls = PersistentSymbolTable::self().declarations(id);
PersistentSymbolTable::Declarations::Iterator decl = decls.iterator();
for (; decl; ++decl) {
const IndexedDeclaration& iDecl(*decl);
PersistentSymbolTable::self().visitDeclarations(id, [&](const IndexedDeclaration& indexedDecl) {
///@todo think this over once we don't pull in all imported top-context any more
//Don't trigger loading of top-contexts from here, it will create a lot of problems
if ((!DUChain::self()->isInMemory(iDecl.topContextIndex())))
continue;
Declaration* decl = iDecl.data();
if (decl && m_indirectData.additionalIdentity == decl->additionalIdentity()) {
//Hit
ret = decl;
if (!ret->isForwardDeclaration())
break;
if (DUChain::self()->isInMemory(indexedDecl.topContextIndex())) {
return visitDeclaration(indexedDecl);
}
}
return PersistentSymbolTable::VisitorState::Continue;
});
}
} else {
//Find the declaration by m_topContext and m_declaration
......
......@@ -1291,7 +1291,6 @@ void DUChain::initialize()
// being 0 and hence crashes at some point later when accessing the contents via
// read. See https://bugs.kde.org/show_bug.cgi?id=250779
RecursiveImportRepository::repository();
RecursiveImportCacheRepository::repository();
ItemRepositoryFor<EnvironmentInformationListItem>::init();
ItemRepositoryFor<EnvironmentInformationItem>::init();
......
......@@ -437,18 +437,20 @@ KDevelop::DUContext* DUChainUtils::argumentContext(KDevelop::Declaration* decl)
}
QList<IndexedDeclaration> DUChainUtils::collectAllVersions(Declaration* decl) {
QList<IndexedDeclaration> ret;
ret << IndexedDeclaration(decl);
if(decl->inSymbolTable())
{
uint count;
const IndexedDeclaration* allDeclarations;
PersistentSymbolTable::self().declarations(decl->qualifiedIdentifier(), count, allDeclarations);
for(uint a = 0; a < count; ++a)
if(!(allDeclarations[a] == IndexedDeclaration(decl)))
ret << allDeclarations[a];
}
const auto indexedDecl = IndexedDeclaration(decl);
QList<IndexedDeclaration> ret;
ret << indexedDecl;
if (decl->inSymbolTable()) {
auto visitor = [&](const IndexedDeclaration& indexed) {
if (indexed != indexedDecl) {
ret << indexed;
}
return PersistentSymbolTable::VisitorState::Continue;
};
PersistentSymbolTable::self().visitDeclarations(decl->qualifiedIdentifier(), visitor);
}
return ret;
}
......@@ -483,19 +485,20 @@ static QList<Declaration*> inheritersInternal(const Declaration* decl, uint& max
}
if(collectVersions && decl->inSymbolTable()) {
uint count;
const IndexedDeclaration* allDeclarations;
PersistentSymbolTable::self().declarations(decl->qualifiedIdentifier(), count, allDeclarations);
for(uint a = 0; a < count; ++a) {
++maxAllowedSteps;
if(allDeclarations[a].data() && allDeclarations[a].data() != decl) {
ret += inheritersInternal(allDeclarations[a].data(), maxAllowedSteps, false);
}
if(maxAllowedSteps == 0)
return ret;
}
auto visitor = [&](const IndexedDeclaration& indexedDeclaration) {
++maxAllowedSteps;
auto declaration = indexedDeclaration.data();
if (declaration && declaration != decl) {
ret += inheritersInternal(declaration, maxAllowedSteps, false);
}
if (maxAllowedSteps == 0) {
return PersistentSymbolTable::VisitorState::Break;
} else {
return PersistentSymbolTable::VisitorState::Continue;
}
};
PersistentSymbolTable::self().visitDeclarations(decl->qualifiedIdentifier(), visitor);
}
return ret;
......
......@@ -629,13 +629,10 @@ void DUContext::findLocalDeclarationsInternal(const IndexedIdentifier& identifie
TopDUContext* top = topContext();
uint count;
const IndexedDeclaration* declarations;
PersistentSymbolTable::self().declarations(id, count, declarations);
for (uint a = 0; a < count; ++a) {
PersistentSymbolTable::self().visitDeclarations(id, [&](const IndexedDeclaration& indexedDecl) {
///@todo Eventually do efficient iteration-free filtering
if (declarations[a].topContextIndex() == top->ownIndex()) {
Declaration* decl = declarations[a].declaration();
if (indexedDecl.topContextIndex() == top->ownIndex()) {
Declaration* decl = indexedDecl.declaration();
if (decl && contextIsChildOrEqual(decl->context(), this)) {
Declaration* checked = checker.check(decl);
if (checked) {
......@@ -643,7 +640,8 @@ void DUContext::findLocalDeclarationsInternal(const IndexedIdentifier& identifie
}
}
}
}
return PersistentSymbolTable::VisitorState::Continue;
});
} else {
//Iterate through all declarations
DUContextDynamicData::VisibleDeclarationIterator it(m_dynamicData);
......
......@@ -42,6 +42,8 @@ public:
return m_topContext == rhs.m_topContext && m_declarationIndex == rhs.m_declarationIndex;
}
inline bool operator!=(const IndexedDeclaration& rhs) const { return !operator==(rhs); }
inline uint hash() const
{
if (isDummy())
......
......@@ -175,25 +175,24 @@ QString AbstractDeclarationNavigationContext::html(bool shorten)
modifyHtml() += i18n("(unresolved forward-declaration) ");
QualifiedIdentifier id = forwardDec->qualifiedIdentifier();
const auto& forwardDecFile = forwardDec->topContext()->parsingEnvironmentFile();
uint count;
const IndexedDeclaration* decls;
PersistentSymbolTable::self().declarations(id, count, decls);
for (uint a = 0; a < count; ++a) {
auto dec = decls[a].data();
auto visitor = [&](const IndexedDeclaration& indexedDec) {
auto dec = indexedDec.data();
if (!dec || dec->isForwardDeclaration()) {
continue;
return PersistentSymbolTable::VisitorState::Continue;
}
const auto& decFile = forwardDec->topContext()->parsingEnvironmentFile();
if ((static_cast<bool>(decFile) != static_cast<bool>(forwardDecFile)) ||
(decFile && forwardDecFile && decFile->language() != forwardDecFile->language())) {
// the language of the declarations must match
continue;
return PersistentSymbolTable::VisitorState::Continue;
}
modifyHtml() += QStringLiteral("<br />");
makeLink(i18n("possible resolution from"), DeclarationPointer(
dec), NavigationAction::NavigateDeclaration);
modifyHtml() += QLatin1Char(' ') + dec->url().str();
}
return PersistentSymbolTable::VisitorState::Continue;
};
PersistentSymbolTable::self().visitDeclarations(id, visitor);
}
}
modifyHtml() += QStringLiteral("<br />");
......
......@@ -17,35 +17,82 @@
#include "topducontext.h"
#include "duchain.h"
#include "duchainlock.h"
#include <util/embeddedfreetree.h>
//For now, just _always_ use the cache
const uint MinimumCountForCache = 1;
#include <util/convenientfreelist.h>
#include <util/embeddedfreetree.h>
namespace {
QDebug fromTextStream(const QTextStream& out)
{
if (out.device())
return {out.device()};
return {out.string()};
}
}
#include <language/util/setrepository.h>
namespace KDevelop {
namespace {
// For now, just _always_ use the cache
const uint MinimumCountForCache = 1;
using TextStreamFunction = QTextStream& (*)(QTextStream&);
#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
constexpr TextStreamFunction endl = Qt::endl;
#else
constexpr TextStreamFunction endl = ::endl;
#endif
Utils::BasicSetRepository* RecursiveImportCacheRepository::repository()
QDebug fromTextStream(const QTextStream& out)
{
static QRecursiveMutex mutex;
static Utils::BasicSetRepository recursiveImportCacheRepositoryObject(QStringLiteral("Recursive Imports Cache"),
&mutex, nullptr, false);
return &recursiveImportCacheRepositoryObject;
if (out.device())
return {out.device()};
return {out.string()};
}
struct IndexedDeclarationHandler {
inline static int leftChild(const IndexedDeclaration& m_data) { return ((int)(m_data.dummyData().first)) - 1; }
inline static void setLeftChild(IndexedDeclaration& m_data, int child)
{
m_data.setDummyData(qMakePair((uint)(child + 1), m_data.dummyData().second));
}
inline static int rightChild(const IndexedDeclaration& m_data) { return ((int)m_data.dummyData().second) - 1; }
inline static void setRightChild(IndexedDeclaration& m_data, int child)
{
m_data.setDummyData(qMakePair(m_data.dummyData().first, (uint)(child + 1)));
}
inline static void createFreeItem(IndexedDeclaration& data)
{
data = IndexedDeclaration();
data.setIsDummy(true);
data.setDummyData(qMakePair(0u, 0u)); // Since we subtract 1, this equals children -1, -1
}
// Copies this item into the given one
inline static void copyTo(const IndexedDeclaration& m_data, IndexedDeclaration& data) { data = m_data; }
inline static bool isFree(const IndexedDeclaration& m_data) { return m_data.isDummy(); }
inline static bool equals(const IndexedDeclaration& m_data, const IndexedDeclaration& rhs) { return m_data == rhs; }
};
struct DeclarationTopContextExtractor {
inline static IndexedTopDUContext extract(const IndexedDeclaration& decl) { return decl.indexedTopContext(); }
};
struct DUContextTopContextExtractor {
inline static IndexedTopDUContext extract(const IndexedDUContext& ctx) { return ctx.indexedTopContext(); }
};
struct RecursiveImportCacheRepository {
static Utils::BasicSetRepository* repository()
{
static QRecursiveMutex mutex;
static Utils::BasicSetRepository recursiveImportCacheRepositoryObject(QStringLiteral("Recursive Imports Cache"),
&mutex, nullptr, false);
return &recursiveImportCacheRepositoryObject;
}
};
struct CacheEntry {
using Data = KDevVarLengthArray<IndexedDeclaration>;
using DataHash = QHash<TopDUContext::IndexedRecursiveImports, Data>;
DataHash m_hash;
};
}
DEFINE_LIST_MEMBER_HASH(PersistentSymbolTableItem, declarations, IndexedDeclaration)
......@@ -140,17 +187,25 @@ public:
const PersistentSymbolTableItem& m_item;
};
template <class ValueType>
struct CacheEntry
using Declarations = ConstantConvenientEmbeddedSet<IndexedDeclaration, IndexedDeclarationHandler>;
using CachedIndexedRecursiveImports =
Utils::StorableSet<IndexedTopDUContext, IndexedTopDUContextIndexConversion, RecursiveImportCacheRepository, true>;
using FilteredDeclarationIterator =
ConvenientEmbeddedSetTreeFilterIterator<IndexedDeclaration, IndexedDeclarationHandler, IndexedTopDUContext,
CachedIndexedRecursiveImports, DeclarationTopContextExtractor>;
// Maps declaration-ids to declarations, together with some caches
class PersistentSymbolTableRepo : public ItemRepository<PersistentSymbolTableItem, PersistentSymbolTableRequestItem>
{
using Data = KDevVarLengthArray<ValueType>;
using DataHash = QHash<TopDUContext::IndexedRecursiveImports, Data>;
using ItemRepository::ItemRepository;
DataHash m_hash;
};
public:
QHash<IndexedQualifiedIdentifier, CacheEntry> declarationsCache;
// Maps declaration-ids to declarations
using PersistentSymbolTableRepo = ItemRepository<PersistentSymbolTableItem, PersistentSymbolTableRequestItem>;
// 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;
};
template <>
class ItemRepositoryFor<PersistentSymbolTableItem>
......@@ -167,54 +222,32 @@ public:
static void init() { repo(); }
};
class PersistentSymbolTablePrivate
{
public:
mutable QMutex m_cacheMutex;
mutable QHash<IndexedQualifiedIdentifier, CacheEntry<IndexedDeclaration>> m_declarationsCache;
//We cache the imports so the currently used nodes are very close in memory, which leads to much better CPU cache utilization
mutable QHash<TopDUContext::IndexedRecursiveImports, PersistentSymbolTable::CachedIndexedRecursiveImports> m_importsCache;
};
void PersistentSymbolTable::clearCache()
{
Q_D(PersistentSymbolTable);
QMutexLocker cacheLock(&d->m_cacheMutex);
d->m_importsCache.clear();
d->m_declarationsCache.clear();
LockedItemRepository::write<PersistentSymbolTableItem>([](PersistentSymbolTableRepo& repo) {
repo.importsCache.clear();
repo.declarationsCache.clear();
});
}
PersistentSymbolTable::PersistentSymbolTable()
: d_ptr(new PersistentSymbolTablePrivate())
{
ItemRepositoryFor<PersistentSymbolTableItem>::init();
RecursiveImportCacheRepository::repository();
}
PersistentSymbolTable::~PersistentSymbolTable()
{
//Workaround for a strange destruction-order related crash duing shutdown
//We just let the data leak. This doesn't hurt, as there is no meaningful destructors.
// TODO: analyze and fix it
// delete d;
}
PersistentSymbolTable::~PersistentSymbolTable() = default;
void PersistentSymbolTable::addDeclaration(const IndexedQualifiedIdentifier& id, const IndexedDeclaration& declaration)
{
Q_D(PersistentSymbolTable);
ENSURE_CHAIN_WRITE_LOCKED
{
QMutexLocker cacheLock(&d->m_cacheMutex);
d->m_declarationsCache.remove(id);
}
PersistentSymbolTableItem item;
item.id = id;
LockedItemRepository::write<PersistentSymbolTableItem>([&item, &declaration](PersistentSymbolTableRepo& repo) {
repo.declarationsCache.remove(item.id);
uint index = repo.findIndex(item);
if (index) {
......@@ -257,20 +290,14 @@ void PersistentSymbolTable::addDeclaration(const IndexedQualifiedIdentifier& id,
void PersistentSymbolTable::removeDeclaration(const IndexedQualifiedIdentifier& id,
const IndexedDeclaration& declaration)
{
Q_D(PersistentSymbolTable);
ENSURE_CHAIN_WRITE_LOCKED
{
QMutexLocker cacheLock(&d->m_cacheMutex);
d->m_declarationsCache.remove(id);
Q_ASSERT(!d->m_declarationsCache.contains(id));
}
PersistentSymbolTableItem item;
item.id = id;