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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
