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

Fix item repo issues with hash clashes and monster buckets.

1) Do not alter bucketPtr before freeing it up.

Before, we deleted an item and then sometimes changed the bucketPtr
when there where clashing items. If the new bucketPtr was then a
monster, we assumed its item was deleted and assert that. As we
deleted the item in a different bucket, the assertion did not hold
and we crashed...

The assertion happens when a normal item is deleted and another
monster item exists with the same hash in a different bucket.

2) Unset next bucket for hash, before deconverting it.

This is probably just a workaround, as a comment above hints at
a general problem here, that this should be done more regularly.
Calling setNextBucketForHash(hash, 0) always though leads to other,
new problems and assertions. So I'll leave it as is for now.

The assertion happens when a monster item is deleted and another
item exists with the same hash in a different bucket.

BUG: 272408
parent d344fd4e
......@@ -1483,10 +1483,11 @@ class ItemRepository : public AbstractItemRepository {
//Put the next item in the nextBucketForHash chain into m_firstBucketForHash that has a hash clashing in that array.
Q_ASSERT(*bucketHashPosition == bucket);
IF_ENSURE_REACHABLE(unsigned short previous = bucket;)
while(!bucketPtr->hasClashingItem(hash, bucketHashSize))
auto nextBucket = bucketPtr;
while(!nextBucket->hasClashingItem(hash, bucketHashSize))
{
// Q_ASSERT(!bucketPtr->hasClashingItemReal(hash, bucketHashSize));
unsigned short next = bucketPtr->nextBucketForHash(hash);
unsigned short next = nextBucket->nextBucketForHash(hash);
ENSURE_REACHABLE(next);
ENSURE_REACHABLE(previous);
......@@ -1498,12 +1499,12 @@ class ItemRepository : public AbstractItemRepository {
IF_ENSURE_REACHABLE(previous = next;)
if(next) {
bucketPtr = m_fastBuckets[next];
nextBucket = m_fastBuckets[next];
if(!bucketPtr)
if(!nextBucket)
{
initializeBucket(next);
bucketPtr = m_fastBuckets[next];
nextBucket = m_fastBuckets[next];
}
}else{
break;
......@@ -1536,6 +1537,13 @@ class ItemRepository : public AbstractItemRepository {
//Convert the monster-bucket back to multiple normal buckets, and put them into the free list
uint newBuckets = bucketPtr->monsterBucketExtent()+1;
Q_ASSERT(bucketPtr->isEmpty());
if (!previousBucketNumber) {
// see https://bugs.kde.org/show_bug.cgi?id=272408
// the monster bucket will be deleted and new smaller ones created
// the next bucket for this hash is invalid anyways as done above
// but calling the below unconditionally leads to other issues...
bucketPtr->setNextBucketForHash(hash, 0);
}
convertMonsterBucket(bucket, 0);
for(uint created = bucket; created < bucket + newBuckets; ++created) {
putIntoFreeList(created, bucketForIndex(created));
......
......@@ -22,21 +22,26 @@ struct TestItem {
unsigned int itemSize() const {
return sizeof(TestItem) + m_dataSize;
}
void verifySame(const TestItem* rhs) {
QVERIFY(rhs->m_hash == m_hash);
QCOMPARE(itemSize(), rhs->itemSize());
QVERIFY(memcmp((char*)this, rhs, itemSize()) == 0);
bool equals(const TestItem* rhs) const
{
return rhs->m_hash == m_hash
&& itemSize() == rhs->itemSize()
&& memcmp((char*)this, rhs, itemSize()) == 0;
}
uint m_hash;
uint m_dataSize;
};
struct TestItemRequest {
TestItem& m_item;
bool m_compareData;
TestItemRequest(TestItem& item) : m_item(item) {
TestItemRequest(TestItem& item, bool compareData = false)
: m_item(item)
, m_compareData(compareData)
{
}
enum {
AverageSize = 700 //This should be the approximate average size of an Item
......@@ -65,7 +70,7 @@ struct TestItemRequest {
//Should return whether the here requested item equals the given item
bool equals(const TestItem* item) const {
return hash() == item->hash();
return hash() == item->hash() && (!m_compareData || m_item.equals(item));
}
};
......@@ -125,7 +130,7 @@ class TestItemRepository : public QObject {
itemSize = (rand() % 1000) + sizeof(TestItem);
TestItem* item = createItem(++itemId, itemSize);
Q_ASSERT(item->hash() == itemId);
item->verifySame(item);
QVERIFY(item->equals(item));
uint index = repository.index(TestItemRequest(*item));
if(index > highestSeenIndex)
highestSeenIndex = index;
......@@ -145,7 +150,7 @@ class TestItemRepository : public QObject {
uint index = repository.findIndex(*realItemsById[pick]);
QVERIFY(index);
QVERIFY(realItemsByIndex.contains(index));
realItemsByIndex[index]->verifySame(repository.itemFromIndex(index));
QVERIFY(realItemsByIndex[index]->equals(repository.itemFromIndex(index)));
if((uint) (rand() % 100) < deletionProbability) {
++totalDeletions;
......@@ -154,7 +159,7 @@ class TestItemRepository : public QObject {
QVERIFY(!repository.findIndex(*realItemsById[pick]));
uint newIndex = repository.index(*realItemsById[pick]);
QVERIFY(newIndex);
realItemsByIndex[index]->verifySame(repository.itemFromIndex(newIndex));
QVERIFY(realItemsByIndex[index]->equals(repository.itemFromIndex(newIndex)));
#ifdef POSITION_TEST
//Since we have previously deleted the item, there must be enough space
......@@ -208,6 +213,37 @@ class TestItemRepository : public QObject {
QCOMPARE(qString, strings[i]);
}
}
void deleteClashingMonsterBucket()
{
KDevelop::ItemRepository<TestItem, TestItemRequest> repository("TestItemRepository");
const uint hash = 1235;
QScopedPointer<TestItem> monsterItem(createItem(hash, KDevelop::ItemRepositoryBucketSize + 10));
QScopedPointer<TestItem> smallItem(createItem(hash, 20));
QVERIFY(!monsterItem->equals(smallItem.data()));
uint smallIndex = repository.index(TestItemRequest(*smallItem, true));
uint monsterIndex = repository.index(TestItemRequest(*monsterItem, true));
QVERIFY(monsterIndex != smallIndex);
repository.deleteItem(smallIndex);
QVERIFY(!repository.findIndex(TestItemRequest(*smallItem, true)));
QCOMPARE(monsterIndex, repository.findIndex(TestItemRequest(*monsterItem, true)));
repository.deleteItem(monsterIndex);
// now in reverse order, with different data see: https://bugs.kde.org/show_bug.cgi?id=272408
monsterItem.reset(createItem(hash + 1, KDevelop::ItemRepositoryBucketSize + 10));
smallItem.reset(createItem(hash + 1, 20));
QVERIFY(!monsterItem->equals(smallItem.data()));
monsterIndex = repository.index(TestItemRequest(*monsterItem, true));
smallIndex = repository.index(TestItemRequest(*smallItem, true));
repository.deleteItem(monsterIndex);
QCOMPARE(smallIndex, repository.findIndex(TestItemRequest(*smallItem, true)));
QVERIFY(!repository.findIndex(TestItemRequest(*monsterItem, true)));
repository.deleteItem(smallIndex);
}
};
#include "test_itemrepository.moc"
......
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