[tools] Rewrite tests for symbol remapping to FileCheck

[Hahnfeld]

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.

[Hahnfeld]

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

[lebedev.ri]

In 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?

[Hahnfeld]

In D58385#1410520, @lebedev.ri wrote:

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

[lebedev.ri]

In D58385#1410534, @Hahnfeld wrote:

In D58385#1410520, @lebedev.ri wrote:

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

[Hahnfeld]

In D58385#1410546, @lebedev.ri wrote:

In D58385#1410534, @Hahnfeld wrote:

In D58385#1410520, @lebedev.ri wrote:

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

[lebedev.ri]

In D58385#1410560, @Hahnfeld wrote:

In D58385#1410546, @lebedev.ri wrote:

In D58385#1410534, @Hahnfeld wrote:

In D58385#1410520, @lebedev.ri wrote:

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

[Hahnfeld]

In D58385#1410560, @Hahnfeld wrote:

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.

llvm/test/tools/llvm-cxxmap/Inputs/expected

-_ZN3foo4quux1fEv _ZN3foo6detail3qux1fEv
-_ZN1N1fENS_1X1YE _ZN1N1fEN1M1X1YE

llvm/test/tools/llvm-cxxmap/remap.test

-RUN: llvm-cxxmap %S/Inputs/before.sym %S/Inputs/after.sym -r %S/Inputs/remap.map -o %t.output -Wambiguous -Wincomplete 2>&1 | FileCheck %s --allow-empty
-RUN: diff %S/Inputs/expected %t.output
+RUN: llvm-cxxmap %S/Inputs/before.sym %S/Inputs/after.sym \
+RUN:             -r %S/Inputs/remap.map -o %t.output -Wambiguous -Wincomplete
+RUN:     2>&1 | FileCheck --check-prefix=CXXMAP %s --allow-empty
+RUN: cat %t.output | FileCheck --check-prefix=OUTPUT %s
 
-CHECK-NOT: warning
-CHECK-NOT: error
+CXXMAP-NOT: warning
+CXXMAP-NOT: error
+
+OUTPUT: _ZN3foo4quux1fEv _ZN3foo6detail3qux1fEv
+OUTPUT: _ZN1N1fENS_1X1YE _ZN1N1fEN1M1X1YE

llvm/test/tools/llvm-profdata/Inputs/instr-remap.expected

-# IR level Instrumentation Flag
-:ir
-bar
-# Func Hash:
-1234
-# Num Counters:
-2
-# Counter Values:
-31
-42
-
-bar
-# Func Hash:
-5678
-# Num Counters:
-2
-# Counter Values:
-500
-600
-
-baz
-# Func Hash:
-5678
-# Num Counters:
-2
-# Counter Values:
-7
-8
-

llvm/test/tools/llvm-profdata/Inputs/sample-remap.expected

-main:184019:0
- 4: 534
- 4.2: 534
- 5: 1075
- 5.1: 1075
- 6: 2080
- 7: 534
- 9: 2064 _Z3bazi:1471 _Z3fooi:631
- 10: inline2:2000
-  1: 2000
- 10: inline42:1000
-  1: 1000
-_Z3bazi:40602:2437
- 1: 2437
-_Z3fooi:7711:610
- 1: 610

llvm/test/tools/llvm-profdata/instr-remap.test

-; RUN: llvm-profdata merge -text %S/Inputs/instr-remap.proftext -r %S/Inputs/instr-remap.remap -o %t.output
-; RUN: diff %S/Inputs/instr-remap.expected %t.output
+; RUN: llvm-profdata merge -text %S/Inputs/instr-remap.proftext \
+; RUN:               -r %S/Inputs/instr-remap.remap -o %t.output
+; RUN: cat %t.output | FileCheck --check-prefixes=CHECK,BAR1 %s
+; RUN: cat %t.output | FileCheck --check-prefixes=CHECK,BAR2 %s
+; RUN: cat %t.output | FileCheck --check-prefixes=CHECK,BAZ %s
+
+; CHECK: # IR level Instrumentation Flag
+; CHECK: :ir
+; BAR1: bar
+; BAR1: # Func Hash:
+; BAR1: 1234
+; BAR1: # Num Counters:
+; BAR1: 2
+; BAR1: # Counter Values:
+; BAR1: 31
+; BAR1: 42
+
+; BAR2: bar
+; BAR2: # Func Hash:
+; BAR2: 5678
+; BAR2: # Num Counters:
+; BAR2: 2
+; BAR2: # Counter Values:
+; BAR2: 500
+; BAR2: 600
+
+; BAZ: baz
+; BAZ: # Func Hash:
+; BAZ: 5678
+; BAZ: # Num Counters:
+; BAZ: 2
+; BAZ: # Counter Values:
+; BAZ: 7
+; BAZ: 8

llvm/test/tools/llvm-profdata/sample-remap.test

-; RUN: llvm-profdata merge -sample -text %S/Inputs/sample-remap.proftext -r %S/Inputs/sample-remap.remap -o %t.output
-; RUN: diff %S/Inputs/sample-remap.expected %t.output
+; RUN: llvm-profdata merge -sample -text %S/Inputs/sample-remap.proftext \
+; RUN:               -r %S/Inputs/sample-remap.remap -o %t.output
+; RUN: cat %t.output | FileCheck %s
+
+; CHECK: main:184019:0
+; CHECK:  4: 534
+; CHECK:  4.2: 534
+; CHECK:  5: 1075
+; CHECK:  5.1: 1075
+; CHECK:  6: 2080
+; CHECK:  7: 534
+; CHECK:  9: 2064 _Z3bazi:1471 _Z3fooi:631
+; CHECK:  10: inline2:2000
+; CHECK:   1: 2000
+; CHECK:  10: inline42:1000
+; CHECK:   1: 1000
+; CHECK: _Z3bazi:40602:2437
+; CHECK:  1: 2437
+; CHECK: _Z3fooi:7711:610
+; CHECK:  1: 610