[llvm] r231214 - Revert "[ADT] fail-fast iterators for DenseMap"
Rick Foos
rfoos at codeaurora.org
Wed Mar 4 11:27:31 PST 2015
I see a lot of _DEBUG, DEBUG, and NDEBUG used, and passed in on the
compile line of some tests.
Nothing ties them to an LLVM Debug/Release build.
I can't think of the right answer to force them to be consistent:
overriding the compiler line would involve an everywhere include file;
or overriding the LLVM build type, not what the user intended...Maybe an
Ugly Warning Message?
--- From Ted
When LLDB quits, it prints this out:
MI: Program exited OK
Which seems to be from here:
#if _DEBUG
CMICmnStreamStdout::Instance().Write( MIRSRC( IDE_MI_APP_EXIT_OK ) ); //
Both stdout and Log
#else
CMICmnLog::WriteLog( MIRSRC( IDE_MI_APP_EXIT_OK ) ); // Just to the Log
#endif // _DEBUG
It looks like _DEBUG is defined. I’m not sure where or why.
On 03/04/2015 11:43 AM, Zachary Turner wrote:
> 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 <mailto:dexonsmith at apple.com>> wrote:
>
> >
> > On 2015-Mar-04, at 02:23, Sanjoy Das
> <sanjoy at playingwithpointers.com
> <mailto: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
> <mailto: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
> <mailto: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 <mailto: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 <mailto:llvm-commits at cs.uiuc.edu>
> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
--
Rick Foos
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150304/2d651479/attachment.html>
More information about the llvm-commits
mailing list