[llvm] r231214 - Revert "[ADT] fail-fast iterators for DenseMap"
Sanjoy Das
sanjoy at playingwithpointers.com
Wed Mar 4 00:21:18 PST 2015
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