Page MenuHomePhabricator

[tools] Rewrite tests for symbol remapping to FileCheck
AbandonedPublic

Authored by Hahnfeld on Feb 19 2019, 7:28 AM.

Details

Reviewers
rsmith
davidxl
wmi
Summary

These were added in D51470 and have been failing with reverse-iteration
since. Rewrite the tests with FileCheck so it's easier to test check
the functions independently.

Diff Detail

Event Timeline

Hahnfeld created this revision.Feb 19 2019, 7:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 7:28 AM

Ping, that's now the only test that fails the reverse-iteration bot...

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?

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.

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?
So why is silencing this determinism via modifying the tests is the correct thing to do?

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

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

Another approach would be to sort the output and make it independent from reverse-iteration. See 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.

I've uploaded D58787 which replaces a SmallDenseMap with std::map and fixes the test.

Hahnfeld abandoned this revision.Mar 1 2019, 1:20 AM