[PATCH] D58385: [tools] Rewrite tests for symbol remapping to FileCheck

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a subscriber: mgrang.
lebedev.ri added a comment.

In D58385#1410560 <https://reviews.llvm.org/D58385#1410560>, @Hahnfeld wrote:

> In D58385#1410546 <https://reviews.llvm.org/D58385#1410546>, @lebedev.ri wrote:
>
> > In D58385#1410534 <https://reviews.llvm.org/D58385#1410534>, @Hahnfeld wrote:
> >
> > > In D58385#1410520 <https://reviews.llvm.org/D58385#1410520>, @lebedev.ri wrote:
> > >
> > > > In D58385#1410516 <https://reviews.llvm.org/D58385#1410516>, @Hahnfeld wrote:
> > > >
> > > > > Ping, that's now the only test that fails the `reverse-iteration` bot...
> > > >
> > > >
> > > > This seems to only shuffle the tests around.
> > > >  Does this now pass in reverse-iteration config?
> > >
> > >
> > > Yes, I've tested this locally. The idea for `instr-remap.test` is to invoke `FileCheck` three times, checking each function separately.
> >
> >
> > So then i think i'm missing the point..
> >
> > If it currently passes without reverse iteration, and does not pass with reverse iteration, 
> >  then that means there is some nondeterminism, correct?
>
>
> From my understanding there is no predefined ordering for the output of functions. So with `reverse-iteration` they are shuffled, compared to what the test expects.


I strongly suspect this is the actual problem.

Side-stepping it in tests is just hiding yet another bug that was found thanks to `reverse-iteration`
(IIRC there was at least one such 'let's just silence the reported issue, who cares that it is nondeterminism'
commit previously, although i failed to find it)

CC @mgrang.

> However, their properties are the same in both configuration, they are just output in a different order which is why the approach with multiple `FileCheck` invocations works.



> Another approach would be to sort the output and make it independent from `reverse-iteration`. See D58631 <https://reviews.llvm.org/D58631> for how this was done for ThinLTO. I'm fine with whatever unbreaks the tests, but I'm not familiar with the code so "fixing" the tests and getting review on this was easier for me.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D58385