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