[PATCH] D39245: [ADT] Shuffle containers before sorting to uncover non-deterministic behavior

[Mandeep Singh Grang via Phabricator via llvm-commits]

mgrang added a comment.

In https://reviews.llvm.org/D39245#907960, @dexonsmith wrote:

> In https://reviews.llvm.org/D39245#907527, @davide wrote:
>
> > In https://reviews.llvm.org/D39245#907426, @mgrang wrote:
> >
> > > In https://reviews.llvm.org/D39245#907221, @chandlerc wrote:
> > >
> > > > I don't really like the ever growing set of configuration macros here.
> > > >
> > > > I would much prefer we have one ABI-breaking check macro and just use that for everything here...
> > >
> > >
> > > Thanks @chandlerc. I am fine with enabling this based on LLVM_ABI_BREAKING_CHECKS. However, this macro is ON by default for +asserts builds. This means all existing +asserts bots would start failing as soon as this patch gets merged since there are about 15 unit test failures. Do we want to gate patches if they show up sort order failures uncovered by shuffling?
> > >  My idea was to control this via a separate macro so that we can isolate shuffle+sort failures into a separate bot which may make it easier to fix without gating the regular flow. Thoughts?
> >
> >
> > I agree with Chandler that we should avoid the proliferation of macros and use ABI_BREAKING_CHECKS instead.
>
>
> I'm actually a bit confused by this.  How is this ABI-breaking?  If (1) I'm right, and it isn't ABI-breaking, and (2) we're not going to have a custom macro, then I think this should just be behind NDEBUG.
>
> ABI_BREAKING_CHECKS is meant to be for NDEBUG-like checks that actually break the ABI.  One could, in theory, build Clang with assertions, build LLVM without assertions, and still a functional compiler by overriding one of the defaults for ABI_BREAKING_CHECKS.  I don't see how that applies to these checks.




In https://reviews.llvm.org/D39245#908229, @dexonsmith wrote:

> In https://reviews.llvm.org/D39245#908059, @davide wrote:
>
> > That said, my point that we shouldn't have yet another macro stands, I guess, independently from the bucket we decide to throw these checks in.
>
>
> Yup, no quarrel there.


I guess +asserts and -asserts builds should be comparable in terms of ninja check failures. I would prefer not to add these under ABI_BREAKING. How about we add these either under EXPENSIVE_CHECKS or rename LLVM_REVERSE_ITERATION as LLVM_NON_DETERMINISM_CHECKS and add these under that?

Btw .. I ran shuffle+sort with all targets enabled and see 39 ninja check failures (previously I had built only ARM/AArch64 targets).


Repository:
  rL LLVM

https://reviews.llvm.org/D39245