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

Thu Jan 19 11:19:10 PST 2023

steven_wu added a comment.

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. 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.

I can add extra file check to make sure all decls are in order, so if you run reverse iteration, you will fail before hitting the diff test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625