[PATCH] [ADT][CMake][AutoConf] fail-fast iterators for DenseMap

[Duncan P. N. Exon Smith]

> On 2015 Mar 16, at 09:03, Zachary Turner <zturner at google.com> wrote:
> 
> If I understand correctly, this broke before because LLDB would redefine _NDEBUG sometimes, but not other times when including clang headers.
> 

I think LLDB's NDEBUG inconsistency is a separate issue, and should
probably be cleaned up/removed (as I pointed out, the original reason
for redefining NDEBUG was cleaned up years ago).

If the LLVM/clang libraries that LLDB links against are built with
FAIL_FAST=false, and LLDB builds with FAIL_FAST=true, then you're
still going to have an ABI problem.

IIUC, with this patch LLDB would need to match FAIL_FAST with the
LLVM/clang libraries it's linking against.

> Now, since this isn't based on _NDEBUG, this is no longer an issue. If so, looks good to me.
> On Mon, Mar 16, 2015 at 8:35 AM David Blaikie <dblaikie at gmail.com> wrote:
> On Sun, Mar 15, 2015 at 10:48 PM, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
> Hi chandlerc, dexonsmith, rnk, zturner,
> 
> This is a re-do of D7931 with the following changes:
> 
>  * the `EpochTracker` class is now conditional on
>    `LLVM_ENABLE_FAIL_FAST_ITERATORS` and not `NDEBUG`.
> 
> It'd be nice to have a more general conditional name so we can add other ABI-breaking debug checks under the same flag. (strawman: LLVM_ABI_BREAKING_DEBUG ?)
>  
> 
>  * **The most significant change**: `LLVM_ENABLE_FAIL_FAST_ITERATORS`
>    can be forced to be set or unset independent of `NDEBUG`.  By
>    default, cmake and autoconf set it (thus enabling fail-fast
>    iterators) for with-asserts builds and unset it for without-asserts
>    builds.
> 
> *fingers crossed that this is sufficient for other users* (that they're willing to manually unset this value for their debug builds if they need ABI compatibility)
>  
> 
> With these changes, projects that require ABI compatibility between
> `defined(NDEBUG)` builds and `!defined(NDEBUG)` builds can get that by
> clamping `LLVM_ENABLE_FAIL_FAST_ITERATORS` to on or off.
> 
> == Minor changes from D7931 ==
> 
>  * `HandleBase::getEpochAddress` returns a `void *` to emphasise that
>    the result is for comparison, not derefernecing.
> 
>  * `HandleBase::getEpochAddress` was missing in the fail-fast-disabled
>    version of `HandleBase`.  This was not a problem earlier since the
>    function is only called from `assert`s and when `assert`s were
>    enabled, we'd always choose the fail-fast-enabled version of
>    `HandleBase`.  Since this is no longer the case (a with-asserts
>    build may be using the fail-fast-disabled version of `HandleBase`),
>    the missing method shows up as an obvious compiler error.
> 
> Original summary for D7931:
> This patch is an attempt at making `DenseMapIterator`s "fail-fast".
> Fail-fast iterators that have been invalidated due to insertion into
> the host `DenseMap` deterministically trip an assert (in debug mode)
> on access, instead of non-deterministically hitting memory corruption
> issues.
> 
> http://reviews.llvm.org/D8351
> 
> Files:
>   CMakeLists.txt
>   Makefile.config.in
>   Makefile.rules
>   autoconf/configure.ac
>   cmake/modules/HandleLLVMOptions.cmake
>   configure
>   docs/CMake.rst
>   include/llvm/ADT/DenseMap.h
>   include/llvm/ADT/EpochTracker.h
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
>