[llvm] r231214 - Revert "[ADT] fail-fast iterators for DenseMap"
Zachary Turner
zturner at google.com
Wed Mar 4 09:43:52 PST 2015
How necessary to this change is the ABI incompatibility between the cases
where NDEBUG is and isn't defined? Is it possible to make them ABI
compatible while still keeping the meat of the change intact?
It doesn't really seem like a "nice" thing to do to require that people who
use LLVM as a library use the same compilation flags as LLVM.
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150304/ebc42317/attachment.html>
More information about the llvm-commits
mailing list