[llvm] r231214 - Revert "[ADT] fail-fast iterators for DenseMap"
Sanjoy Das
sanjoy at playingwithpointers.com
Wed Mar 4 01:17:43 PST 2015
The problem is very likely due to inconsistencies in how lldb treats
the NDEBUG macro. For instance
(lldb) p sizeof(llvm::DenseMapBase<llvm::DenseMap<clang::ExternalASTSource*,
lldb_private::ClangExternalASTSourceCommon*,
llvm::DenseMapInfo<clang::ExternalASTSource*>,
llvm::detail::DenseMapPair<clang::ExternalASTSource*,
lldb_private::ClangExternalASTSourceCommon*> >,
clang::ExternalASTSource*,
lldb_private::ClangExternalASTSourceCommon*,
llvm::DenseMapInfo<clang::ExternalASTSource*>,
llvm::detail::DenseMapPair<clang::ExternalASTSource*,
lldb_private::ClangExternalASTSourceCommon*> >)
(unsigned long) $8 = 1
(lldb) p sizeof(DebugEpochBase)
(unsigned long) $9 = 8
when DenseMapBase<...> inherits DebugEpochBase. This is the reason
why incrementEpoch() ends up incrementing the "Buckets" pointer
instead of the actual Epoch word.
On Wed, Mar 4, 2015 at 12:21 AM, Sanjoy Das
<sanjoy at playingwithpointers.com> wrote:
> I'm at a loss as to how this could have happened. Looks like we
> somehow end up with DenseMap::Buckets as 0x1 which is clearly not a
> valid pointer.
>
> I have a few theories that I've already discussed with Zachary on IRC:
>
> * there is some racy accesses going on to the DenseMap and that is
> producing incorrect behavior
>
> * some dlopen oddity that leads to an improperly constructed DenseMap
>
> * for some reason the incrementEpoch(); in InsertIntoBucketImpl bumps
> the "Buckets" pointer by 1 instead of Epoch. I don't see how this
> could happen except maybe due to a bug in the C++ compiler.
>
> Anyway, I'll try to reproduce this tomorrow (explicit directions on
> how to do so will be very helpful). Please let me know if you have
> any other ideas.
>
> Thanks,
> -- Sanjoy
>
> On Tue, Mar 3, 2015 at 10:05 PM, Chaoren Lin <chaorenl at google.com> wrote:
>> Author: chaoren
>> Date: Wed Mar 4 00:05:37 2015
>> New Revision: 231214
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=231214&view=rev
>> Log:
>> Revert "[ADT] fail-fast iterators for DenseMap"
>>
>> This reverts commit 4b7263d855006988854036b4a4891fcf19aebe65.
>>
>> r231125 http://reviews.llvm.org/D7931
>>
>> This was causing many LLDB tests to fail on OS X, Linux, and FreeBSD.
>>
>> https://bpaste.net/show/6a23e1f53623
>>
>> Removed:
>> llvm/trunk/include/llvm/ADT/EpochTracker.h
>> Modified:
>> llvm/trunk/include/llvm/ADT/DenseMap.h
>>
>> Modified: llvm/trunk/include/llvm/ADT/DenseMap.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/DenseMap.h?rev=231214&r1=231213&r2=231214&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ADT/DenseMap.h (original)
>> +++ llvm/trunk/include/llvm/ADT/DenseMap.h Wed Mar 4 00:05:37 2015
>> @@ -15,7 +15,6 @@
>> #define LLVM_ADT_DENSEMAP_H
>>
>> #include "llvm/ADT/DenseMapInfo.h"
>> -#include "llvm/ADT/EpochTracker.h"
>> #include "llvm/Support/AlignOf.h"
>> #include "llvm/Support/Compiler.h"
>> #include "llvm/Support/MathExtras.h"
>> @@ -51,7 +50,7 @@ class DenseMapIterator;
>>
>> template <typename DerivedT, typename KeyT, typename ValueT, typename KeyInfoT,
>> typename BucketT>
>> -class DenseMapBase : public DebugEpochBase {
>> +class DenseMapBase {
>> public:
>> typedef unsigned size_type;
>> typedef KeyT key_type;
>> @@ -63,17 +62,16 @@ public:
>> const_iterator;
>> inline iterator begin() {
>> // When the map is empty, avoid the overhead of AdvancePastEmptyBuckets().
>> - return empty() ? end() : iterator(getBuckets(), getBucketsEnd(), *this);
>> + return empty() ? end() : iterator(getBuckets(), getBucketsEnd());
>> }
>> inline iterator end() {
>> - return iterator(getBucketsEnd(), getBucketsEnd(), *this, true);
>> + return iterator(getBucketsEnd(), getBucketsEnd(), true);
>> }
>> inline const_iterator begin() const {
>> - return empty() ? end()
>> - : const_iterator(getBuckets(), getBucketsEnd(), *this);
>> + return empty() ? end() : const_iterator(getBuckets(), getBucketsEnd());
>> }
>> inline const_iterator end() const {
>> - return const_iterator(getBucketsEnd(), getBucketsEnd(), *this, true);
>> + return const_iterator(getBucketsEnd(), getBucketsEnd(), true);
>> }
>>
>> bool LLVM_ATTRIBUTE_UNUSED_RESULT empty() const {
>> @@ -83,13 +81,11 @@ public:
>>
>> /// Grow the densemap so that it has at least Size buckets. Does not shrink
>> void resize(size_type Size) {
>> - incrementEpoch();
>> if (Size > getNumBuckets())
>> grow(Size);
>> }
>>
>> void clear() {
>> - incrementEpoch();
>> if (getNumEntries() == 0 && getNumTombstones() == 0) return;
>>
>> // If the capacity of the array is huge, and the # elements used is small,
>> @@ -122,13 +118,13 @@ public:
>> iterator find(const KeyT &Val) {
>> BucketT *TheBucket;
>> if (LookupBucketFor(Val, TheBucket))
>> - return iterator(TheBucket, getBucketsEnd(), *this, true);
>> + return iterator(TheBucket, getBucketsEnd(), true);
>> return end();
>> }
>> const_iterator find(const KeyT &Val) const {
>> const BucketT *TheBucket;
>> if (LookupBucketFor(Val, TheBucket))
>> - return const_iterator(TheBucket, getBucketsEnd(), *this, true);
>> + return const_iterator(TheBucket, getBucketsEnd(), true);
>> return end();
>> }
>>
>> @@ -141,14 +137,14 @@ public:
>> iterator find_as(const LookupKeyT &Val) {
>> BucketT *TheBucket;
>> if (LookupBucketFor(Val, TheBucket))
>> - return iterator(TheBucket, getBucketsEnd(), *this, true);
>> + return iterator(TheBucket, getBucketsEnd(), true);
>> return end();
>> }
>> template<class LookupKeyT>
>> const_iterator find_as(const LookupKeyT &Val) const {
>> const BucketT *TheBucket;
>> if (LookupBucketFor(Val, TheBucket))
>> - return const_iterator(TheBucket, getBucketsEnd(), *this, true);
>> + return const_iterator(TheBucket, getBucketsEnd(), true);
>> return end();
>> }
>>
>> @@ -167,13 +163,12 @@ public:
>> std::pair<iterator, bool> insert(const std::pair<KeyT, ValueT> &KV) {
>> BucketT *TheBucket;
>> if (LookupBucketFor(KV.first, TheBucket))
>> - return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true),
>> + return std::make_pair(iterator(TheBucket, getBucketsEnd(), true),
>> false); // Already in map.
>>
>> // Otherwise, insert the new element.
>> TheBucket = InsertIntoBucket(KV.first, KV.second, TheBucket);
>> - return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true),
>> - true);
>> + return std::make_pair(iterator(TheBucket, getBucketsEnd(), true), true);
>> }
>>
>> // Inserts key,value pair into the map if the key isn't already in the map.
>> @@ -182,15 +177,14 @@ public:
>> std::pair<iterator, bool> insert(std::pair<KeyT, ValueT> &&KV) {
>> BucketT *TheBucket;
>> if (LookupBucketFor(KV.first, TheBucket))
>> - return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true),
>> + return std::make_pair(iterator(TheBucket, getBucketsEnd(), true),
>> false); // Already in map.
>> -
>> +
>> // Otherwise, insert the new element.
>> TheBucket = InsertIntoBucket(std::move(KV.first),
>> std::move(KV.second),
>> TheBucket);
>> - return std::make_pair(iterator(TheBucket, getBucketsEnd(), *this, true),
>> - true);
>> + return std::make_pair(iterator(TheBucket, getBucketsEnd(), true), true);
>> }
>>
>> /// insert - Range insertion of pairs.
>> @@ -437,8 +431,6 @@ private:
>> }
>>
>> BucketT *InsertIntoBucketImpl(const KeyT &Key, BucketT *TheBucket) {
>> - incrementEpoch();
>> -
>> // If the load of the hash table is more than 3/4, or if fewer than 1/8 of
>> // the buckets are empty (meaning that many are filled with tombstones),
>> // grow the table.
>> @@ -995,10 +987,9 @@ private:
>>
>> template <typename KeyT, typename ValueT, typename KeyInfoT, typename Bucket,
>> bool IsConst>
>> -class DenseMapIterator : DebugEpochBase::HandleBase {
>> +class DenseMapIterator {
>> typedef DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, true> ConstIterator;
>> friend class DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, true>;
>> - friend class DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, false>;
>>
>> public:
>> typedef ptrdiff_t difference_type;
>> @@ -1012,10 +1003,8 @@ private:
>> public:
>> DenseMapIterator() : Ptr(nullptr), End(nullptr) {}
>>
>> - DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase &Epoch,
>> - bool NoAdvance = false)
>> - : DebugEpochBase::HandleBase(&Epoch), Ptr(Pos), End(E) {
>> - assert(isHandleInSync() && "invalid construction!");
>> + DenseMapIterator(pointer Pos, pointer E, bool NoAdvance = false)
>> + : Ptr(Pos), End(E) {
>> if (!NoAdvance) AdvancePastEmptyBuckets();
>> }
>>
>> @@ -1026,40 +1015,28 @@ public:
>> typename = typename std::enable_if<!IsConstSrc && IsConst>::type>
>> DenseMapIterator(
>> const DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, IsConstSrc> &I)
>> - : DebugEpochBase::HandleBase(I), Ptr(I.Ptr), End(I.End) {}
>> + : Ptr(I.Ptr), End(I.End) {}
>>
>> reference operator*() const {
>> - assert(isHandleInSync() && "invalid iterator access!");
>> return *Ptr;
>> }
>> pointer operator->() const {
>> - assert(isHandleInSync() && "invalid iterator access!");
>> return Ptr;
>> }
>>
>> bool operator==(const ConstIterator &RHS) const {
>> - assert((!Ptr || isHandleInSync()) && "handle not in sync!");
>> - assert((!RHS.Ptr || RHS.isHandleInSync()) && "handle not in sync!");
>> - assert(getEpochAddress() == RHS.getEpochAddress() &&
>> - "comparing incomparable iterators!");
>> - return Ptr == RHS.Ptr;
>> + return Ptr == RHS.operator->();
>> }
>> bool operator!=(const ConstIterator &RHS) const {
>> - assert((!Ptr || isHandleInSync()) && "handle not in sync!");
>> - assert((!RHS.Ptr || RHS.isHandleInSync()) && "handle not in sync!");
>> - assert(getEpochAddress() == RHS.getEpochAddress() &&
>> - "comparing incomparable iterators!");
>> - return Ptr != RHS.Ptr;
>> + return Ptr != RHS.operator->();
>> }
>>
>> inline DenseMapIterator& operator++() { // Preincrement
>> - assert(isHandleInSync() && "invalid iterator access!");
>> ++Ptr;
>> AdvancePastEmptyBuckets();
>> return *this;
>> }
>> DenseMapIterator operator++(int) { // Postincrement
>> - assert(isHandleInSync() && "invalid iterator access!");
>> DenseMapIterator tmp = *this; ++*this; return tmp;
>> }
>>
>>
>> Removed: llvm/trunk/include/llvm/ADT/EpochTracker.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/EpochTracker.h?rev=231213&view=auto
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ADT/EpochTracker.h (original)
>> +++ llvm/trunk/include/llvm/ADT/EpochTracker.h (removed)
>> @@ -1,95 +0,0 @@
>> -//===- llvm/ADT/EpochTracker.h - ADT epoch tracking --------------*- C++ -*-==//
>> -//
>> -// The LLVM Compiler Infrastructure
>> -//
>> -// This file is distributed under the University of Illinois Open Source
>> -// License. See LICENSE.TXT for details.
>> -//
>> -//===----------------------------------------------------------------------===//
>> -//
>> -// This file defines the DebugEpochBase and DebugEpochBase::HandleBase classes.
>> -// These can be used to write iterators that are fail-fast when LLVM is built
>> -// with asserts enabled.
>> -//
>> -//===----------------------------------------------------------------------===//
>> -
>> -#ifndef LLVM_ADT_EPOCH_TRACKER_H
>> -#define LLVM_ADT_EPOCH_TRACKER_H
>> -
>> -#include <cstdint>
>> -
>> -namespace llvm {
>> -
>> -#ifdef NDEBUG
>> -
>> -class DebugEpochBase {
>> -public:
>> - void incrementEpoch() {}
>> -
>> - class HandleBase {
>> - public:
>> - HandleBase() {}
>> - explicit HandleBase(const DebugEpochBase *) {}
>> - bool isHandleInSync() { return true; }
>> - };
>> -};
>> -
>> -#else
>> -
>> -/// \brief A base class for data structure classes wishing to make iterators
>> -/// ("handles") pointing into themselves fail-fast. When building without
>> -/// asserts, this class is empty and does nothing.
>> -///
>> -/// DebugEpochBase does not by itself track handles pointing into itself. The
>> -/// expectation is that routines touching the handles will poll on
>> -/// isHandleInSync at appropriate points to assert that the handle they're using
>> -/// is still valid.
>> -///
>> -class DebugEpochBase {
>> - uint64_t Epoch;
>> -
>> -public:
>> - DebugEpochBase() : Epoch(0) {}
>> -
>> - /// \brief Calling incrementEpoch invalidates all handles pointing into the
>> - /// calling instance.
>> - void incrementEpoch() { ++Epoch; }
>> -
>> - /// \brief The destructor calls incrementEpoch to make use-after-free bugs
>> - /// more likely to crash deterministically.
>> - ~DebugEpochBase() { incrementEpoch(); }
>> -
>> - /// \brief A base class for iterator classes ("handles") that wish to poll for
>> - /// iterator invalidating modifications in the underlying data structure.
>> - /// When LLVM is built without asserts, this class is empty and does nothing.
>> - ///
>> - /// HandleBase does not track the parent data structure by itself. It expects
>> - /// the routines modifying the data structure to call incrementEpoch when they
>> - /// make an iterator-invalidating modification.
>> - ///
>> - class HandleBase {
>> - const uint64_t *EpochAddress;
>> - uint64_t EpochAtCreation;
>> -
>> - public:
>> - HandleBase() : EpochAddress(nullptr), EpochAtCreation(UINT64_MAX) {}
>> -
>> - explicit HandleBase(const DebugEpochBase *Parent)
>> - : EpochAddress(&Parent->Epoch), EpochAtCreation(Parent->Epoch) {}
>> -
>> - /// \brief Returns true if the DebugEpochBase this Handle is linked to has
>> - /// not called incrementEpoch on itself since the creation of this
>> - /// HandleBase instance.
>> - bool isHandleInSync() const { return *EpochAddress == EpochAtCreation; }
>> -
>> - /// \brief Returns a pointer to the epoch word stored in the data structure
>> - /// this handle points into.
>> - const uint64_t *getEpochAddress() const { return EpochAddress; }
>> - };
>> -};
>> -
>> -#endif
>> -
>> -} // namespace llvm
>> -
>> -#endif
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list