[llvm] r231214 - Revert "[ADT] fail-fast iterators for DenseMap"
Sanjoy Das
sanjoy at playingwithpointers.com
Fri Mar 6 12:39:13 PST 2015
Another way of approaching the problem: keep the two words in
DenseMapIterators even in non-asserts mode, and have the asserts mode
only turn on the assertions. I'd speculate that not many places
within LLVM/Clang/LLDB keep around iterators long enough for this to
impact RAM usage from the size bloat within the iterators.
The extra word within a DenseMap is more problematic; but in 64 bit
mode, I think we can pack a 32-bit epoch right after NumBuckets for
free. In 32 bit mode, if we really wanted to, we could use NumEntries
as an approximation to "epoch".
What do you think?
On Wed, Mar 4, 2015 at 9:30 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>>
>> 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