[llvm] r231214 - Revert "[ADT] fail-fast iterators for DenseMap"
Sanjoy Das
sanjoy at playingwithpointers.com
Wed Mar 4 02:23:05 PST 2015
As an example, ClangASTType.h will include clang headers without
#defining NDEBUG while ClangExternalASTSourceCommon.h will include
clang headers *after* #defining NDEBUG.
Also, where is LLVM_NDEBUG_OFF set? When building with a
Release+Asserts llvm it should have been #defined, but it does not
seem to be. I could not find anything with a straightforward grep.
Thanks,
-- Sanjoy
On Wed, Mar 4, 2015 at 1:17 AM, Sanjoy Das
<sanjoy at playingwithpointers.com> wrote:
> 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