[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D141625#4066681 <https://reviews.llvm.org/D141625#4066681>, @steven_wu wrote:

> In D141625#4066466 <https://reviews.llvm.org/D141625#4066466>, @dblaikie wrote:
>
>> In D141625#4066362 <https://reviews.llvm.org/D141625#4066362>, @steven_wu wrote:
>>
>>> @dblaikie Do you have any objection for committing the patch as it is? I don't think reverse iteration test is a proper way to test for this specific bug.
>>
>> I think reverse iteration is the right way to test this specific bug (& any bug where behavior may be unintentionally nondeterministic due to non-deterministically ordered containers) - it makes the testing more reliable rather than depending on implementation details of such containers that aren't guaranteed (that being the point/problem).
>>
>> But it's not the worst/most unwieldy test for this sort of thing & if we don't have bots using it... hmm, actually I looked more closely & maybe we do have reverse iteration builders: https://github.com/llvm/llvm-zorg/search?q=reverse-iteration (though I'm having trouble navigating the builder UI to see whether there are up-to-date builds for this configuration)
>>
>> Could you help me understand your perspective regarding using reverse iteration to test this specific bug? (are there some bugs you find reverse iteration suitable for? what's different about this one from them?)
>
> With reverse iteration, you can make sure that the we didn't iterate over the non-deterministic container by checking the ordering of the decls in the module, which is fine.

I'm not sure I follow - does building LLVM with reverse iteration not break the integration/diff test you've written? I'd expect it would, and that it would fail a smaller test with only a couple of decls?

> But no one really runs that tests regularly.
> Without the reverse iteration, the ordering check will always succeed because the anonymous decls will be numbered and ordered in the ascending order as iteration. It is not easy to decode which decl is which by analyze the output of llvm-bcanalyzer. Current test (which is not big at all) will put (80 - 32) entries into hash table of smallptrset, thus it kind of triggers the bug near 100% of the time. I would think the current test is more valuable than a test only fail in reverse iteration.

Given stability issues are pretty rare/unstable to reproduce, I think a test that only fails in one of reverse or forward iteration mode is acceptable/a suitable way to test stability problems & I'm OK with/would personally prefer the test be reduced to only that, rather than having to add enough to cause the hashing to tip over into instability - that's why reverse iteration was added, to catch these sort of issues, including through intentional tests of features that would expose problems when combined with reverse iteration.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141625/new/

https://reviews.llvm.org/D141625