[llvm] r231214 - Revert "[ADT] fail-fast iterators for DenseMap"
Duncan P. N. Exon Smith
dexonsmith at apple.com
Wed Mar 4 09:30:53 PST 2015
>
> On 2015-Mar-04, at 02:23, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
>
> As an example, ClangASTType.h will include clang headers without
> #defining NDEBUG while ClangExternalASTSourceCommon.h will include
> clang headers *after* #defining NDEBUG.
>
I've looked up rdar://8691220 for you, which is referenced in the
comments around LLDB_DEFINED_NDEBUG_FOR_CLANG.
This was apparently fixed by a series of CFE commits ending with
r120708 back in 2011, but the workaround was never removed.
Nevertheless you probably need to propagate the workaround further,
or come up with a different approach -- what I've learned is that
lldb likes to build in debug mode and link against a clang built in
release mode. (Another option is to convince the lldb folks not to
mix configurations.)
> 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.
It looks like this logic was added in r143036, but I can't find
anything that actually sets it (even in the build systems). Seems
like dead code to me too.
> 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