[AArch64] Incorrect RETURN_ADDRESS in tests

[rovka]

Bugzilla Link	36313
Resolution	FIXED
Resolved on	Feb 15, 2018 03:00
Version	unspecified
OS	Linux
Blocks	#35152
Attachments	Full failure report
CC	@aemerson,@dsandersllvm,@hahnjo,@jprotze,@qcolombet

Extended Description

I'm getting these failures on AArch64 in 6.0.0-rc2:

Failing Tests (6):
libomp :: ompt/misc/control_tool.c
libomp :: ompt/synchronization/flush.c
libomp :: ompt/synchronization/master.c
libomp :: ompt/synchronization/taskwait.c
libomp :: ompt/synchronization/test_lock.c
libomp :: ompt/synchronization/test_nest_lock_parallel.c

They all look something like this:
******************** TEST 'libomp :: ompt/synchronization/test_nest_lock_parallel.c' FAILED ********************
Script:

/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/./bin/clang -fopenmp -I /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test -I /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/projects/openmp/runtime/src -L /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/lib -I /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test/ompt /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test/ompt/synchronization/test_nest_lock_parallel.c -o /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/projects/openmp/runtime/test/ompt/synchronization/Output/test_nest_lock_parallel.c.tmp -lm -latomic && /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/projects/openmp/runtime/test/ompt/synchronization/Output/test_nest_lock_parallel.c.tmp | sort --numeric-sort --stable | /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/./bin/FileCheck /home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test/ompt/synchronization/test_nest_lock_parallel.c

Exit Code: 1

Command Output (stdout):

$ "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/./bin/clang" "-fopenmp" "-I" "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test" "-I" "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/projects/openmp/runtime/src" "-L" "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/lib" "-I" "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test/ompt" "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test/ompt/synchronization/test_nest_lock_parallel.c" "-o" "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/projects/openmp/runtime/test/ompt/synchronization/Output/test_nest_lock_parallel.c.tmp" "-lm" "-latomic"
$ "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/projects/openmp/runtime/test/ompt/synchronization/Output/test_nest_lock_parallel.c.tmp"
$ "sort" "--numeric-sort" "--stable"
$ "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/Phase3/Release/llvmCore-6.0.0-rc2.obj/./bin/FileCheck" "/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test/ompt/synchronization/test_nest_lock_parallel.c"

command stderr:

/home/tcwg-buildslave/workspace/tcwg-llvm-release/tcwg-apm_64-build/rc2/llvm.src/projects/openmp/runtime/test/ompt/synchronization/test_nest_lock_parallel.c:47:17: error: expected string not found in input
// CHECK-NEXT: {{^}}[[MASTER_ID]]: current_address={{.*}}[[RETURN_ADDRESS]]
^
:20:1: note: scanning from here
281474976710657: current_address=0x403658 or 0x403654
^
:20:1: note: with variable "MASTER_ID" equal to "281474976710657"
281474976710657: current_address=0x403658 or 0x403654
^
:20:1: note: with variable "RETURN_ADDRESS" equal to "0x403650"
281474976710657: current_address=0x403658 or 0x403654
^
:20:13: note: possible intended match here
281474976710657: current_address=0x403658 or 0x403654
^

error: command failed with exit status: 1

[rovka]

assigned to @zmodem

[hahnjo]

+Paul who added support for OMPT on AArch64

The check for the return address is known to be a bit fragile, relies on the instructions emitted by the compiler and needs changes for every architecture. Paul did them for AArch64 but it looks like Clang 6.0 inserts another instruction (off by 4 bytes). In particular this might be related to GlobalIsel?

Anyway, can you get me an objdump of the related code region? There should be

• a call to a _kmpc* function
• possibly a store instruction to save the return value
• a NOP
  If Clang now emits another instruction we can just add another possible address.

[jprotze]

https://reviews.llvm.org/D43115 should address these issues. I agree with Jonas, that we would like to look into the assembly to understand what is going on there. We would appreciate to get an object dump for the respective code region.

[rovka]

Thanks! I'll post the assembly as soon as I can.

[rovka]

Assembly for the synchronization/flush.c test
I've attached the disassembly for the flush.c test (since it superficially seemed smaller than the others). Hope that helps, please let me know if you need anything else. Sorry about the delay.

[hahnjo]

Created attachment 19848 [details]
Assembly for the synchronization/flush.c test

I've attached the disassembly for the flush.c test (since it superficially
seemed smaller than the others). Hope that helps, please let me know if you
need anything else. Sorry about the delay.

That's the interesting part:
403344: 97fff4f3 bl 400710 __kmpc_flush@plt
-> call runtime function

403348: d503201f nop
-> this nop is the return instruction and inserted by the test via inline assembly

40334c: 14000001 b 403350 <.omp_outlined.+0x30>
-> ... an unconditional branch to the next instruction

403350: f0000088 adrp x8, 416000 <GLOBAL_OFFSET_TABLE+0x28>
403354: f9408d08 ldr x8, [x8,#280]
403358: d63f0100 blr x8
40335c: f9400001 ldr x1, [x0]
-> ompt_get_thread_data()

403360: 90000008 adrp x8, 403000 <on_ompt_callback_task_schedule+0x6c>
403364: 910d4108 add x8, x8, #​0x350
-> address of the label: 0x403350

403368: f1001102 subs x2, x8, #​0x4
40336c: f1002103 subs x3, x8, #​0x8
-> two possible values: 4 and 8 bytes before the label

403370: 90000000 adrp x0, 403000 <on_ompt_callback_task_schedule+0x6c>
403374: 913dfc00 add x0, x0, #​0xf7f
-> formatting string

403378: 97fff50e bl 4007b0 printf@plt
-> printf()

So I think this test would be fine if it actually checks for the second address (done in the linked patch). However, this only works if the runtime call returns no value. If it does have a return value and there is an additional store instruction, I'd assume that the branch is there as well which kills the assumptions.

I'm not sure where the branch instruction comes from, but if it's really related to GlobalIsel we'll end up having the same problem with other architectures if they do the switch...

[hahnjo]

I'm not sure where the branch instruction comes from, but if it's really
related to GlobalIsel we'll end up having the same problem with other
architectures if they do the switch...

It is, small example:
$ cat label.c
void test() {
asm("nop");
label:
printf("%p\n", &&label);
}

$ clang -target aarch64 -S label.c -o -
[...]
//APP
nop
//NO_APP
b .LBB0_1
.Ltmp0: // Block address taken
.LBB0_1:
[...]

$ clang -target aarch64 -fno-experimental-isel -S label.c -o -
[...]
//APP
nop
//NO_APP
.Ltmp0: // Block address taken
// %bb.1:
[...]

And I can get the same on x86:
$ clang -target x86_64 -S label.c -o -
[...]
#APP
nop
#NO_APP
.Ltmp0: # Block address taken

%bb.1:

[...]

$ clang -target x86_64 -fexperimental-isel -S label.c -o -
[...]
#APP
nop
#NO_APP
jmp .LBB0_4
.Ltmp0: # Block address taken
.LBB0_4:
[...]

So if we don't "fix" GlobalIsel in the compiler we need to adapt the return addresses of all architectures. Diana, do you think it's worth messing with GlobalIsel?

[rovka]

I think this is worth investigating deeper.

If I use -mllvm -print-after-all on your example, I get this:

*** IR Dump After InstructionSelect ***:

Machine code for function test: IsSSA, TracksLiveness, FailedISel, Legalized, RegBankSelected, Selected

%bb.1: derived from LLVM BB %0
%2:_ = G_GLOBAL_VALUE @.str;
%1:_ = COPY %2;
INLINEASM $nop [sideeffect] [attdialect]
Successors according to CFG: %bb.2(?%)

%bb.2: derived from LLVM BB %1, ADDRESS TAKEN
Predecessors according to CFG: %bb.1
ADJCALLSTACKDOWN 0, 0, implicit-def %sp, implicit %sp
%x0 = COPY %1;
%x1 = COPY %3:;
BL @​printf, <regmask %fp %lr %b8 %b9 %b10 %b11 %b12 %b13 %b14 %b15 %d8 %d9 %d10 %d11 %d12 %d13 %d14 %d15 %h8 %h9 %h10 %h11 %h12 %h13 %h14 %h15 %s8 %s9 %s10 %s11 %s12 %s13 %s14 and 63 more...>, implicit-def %lr, implicit %sp, implicit %x0, implicit %x1, implicit-def %w0
%0: = COPY %w0;
ADJCALLSTACKUP 0, 0, implicit-def %sp, implicit %sp
RET_ReallyLR

End machine code for function test.

*** IR Dump After AArch64 Instruction Selection ***:

Machine code for function test: IsSSA, TracksLiveness

%bb.3: derived from LLVM BB %0
INLINEASM $nop [sideeffect] [attdialect], !​2
B %bb.4
Successors according to CFG: %bb.4(?%)

%bb.4: derived from LLVM BB %1, ADDRESS TAKEN
Predecessors according to CFG: %bb.3
ADJCALLSTACKDOWN 0, 0, implicit-def dead %sp, implicit %sp
%0:gpr64 = MOVaddr target-flags(aarch64-page) @.str, target-flags(aarch64-pageoff, aarch64-nc) @.str; G#64 :%0
%1:gpr64 = MOVaddrBA target-flags(aarch64-page) blockaddress(@test, %ir-block.1), target-flags(aarch64-pageoff, aarch64-nc) blockaddress(@test, %ir-block.1); G#64 :%1
%x0 = COPY %0; G#64 :%0
%x1 = COPY %1; G#64 :%1
BL @​printf, <regmask %fp %lr %b8 %b9 %b10 %b11 %b12 %b13 %b14 %b15 %d8 %d9 %d10 %d11 %d12 %d13 %d14 %d15 %h8 %h9 %h10 %h11 %h12 %h13 %h14 %h15 %s8 %s9 %s10 %s11 %s12 %s13 %s14 and 63 more...>, implicit-def dead %lr, implicit %sp, implicit %x0, implicit %x1, implicit-def %sp, implicit-def %w0
ADJCALLSTACKUP 0, 0, implicit-def dead %sp, implicit %sp
%2:gpr32all = COPY %w0; G#32 all:%2
RET_ReallyLR

End machine code for function test.

So it seems we're running both GlobalISel and DAGISel, and it's actually the DAGISel that introduces the branch... I'm not sure why we fallback here.

[hahnjo]

It gets even better: I tried running the full OpenMP test suite with -fexperimental-isel on x86 and there are two jumps in ompt/synchronization/master.c

403ae6: e8 15 cc ff ff callq 400700 __kmpc_end_master@plt
403aeb: e9 00 00 00 00 jmpq 403af0 <.omp_outlined.+0xb0>
403af0: 90 nop
403af1: e9 00 00 00 00 jmpq 403af6 <.omp_outlined.+0xb6>

-> this is the position of the label
403af6: 48 8b 05 4b 36 20 00 mov 0x20364b(%rip),%rax # 607148 <ompt_get_thread_data>
403afd: ff d0 callq *%rax
403aff: 48 8b 30 mov (%rax),%rsi
403b02: b9 eb 47 40 00 mov $0x4047eb,%ecx
403b07: 89 cf mov %ecx,%edi
403b09: 48 8d 15 e5 ff ff ff lea -0x1b(%rip),%rdx # 403af5 <.omp_outlined.+0xb5>
-> one NOP = 1 byte offset

403b10: 48 8d 0d db ff ff ff lea -0x25(%rip),%rcx # 403af2 <.omp_outlined.+0xb2>
-> NOP + MOV (for non-void functions)

403b17: 4c 8d 05 d2 ff ff ff lea -0x2e(%rip),%r8 # 403af0 <.omp_outlined.+0xb0>
-> NOP + JMPQ, trying to account for GlobalIsel

403b1e: 4c 8d 0d c8 ff ff ff lea -0x38(%rip),%r9 # 403aed <.omp_outlined.+0xad>
-> NOP + MOV + JMPQ

What would actually be needed here is JMPQ + NOP + JMPQ = 11 bytes :-(

[hahnjo]

So it seems we're running both GlobalISel and DAGISel, and it's actually the
DAGISel that introduces the branch... I'm not sure why we fallback here.

label.c:1:1: remark: unable to translate constant: i8* [-Rpass-missed=gisel-irtranslator]

Next question: Why does the fallback insert these branches and jumps?

[dsandersllvm]

In the output Diana provided in comment #​7, I see the FailedISel attribute is present after the InstructionSelect pass. Going by the IR Translator message that Jonas mentioned in comment #​9, it was most likely added by the IR Translator in which case most of the GlobalISel passes won't have done anything to that function. I don't see anything taking the address of the block so the IR Translator most likely failed on that.

The strange thing is that the FailedISel attribute causes the function to be reset before DAGISel runs on it so you should see the same behaviour between DAGISel-only and GlobalISel that fell back to DAGISel. Could there be some state that the ResetMachineFunction pass isn't resetting?

[hahnjo]

I don't see anything taking the address of the
block so the IR Translator most likely failed on that.

That's probably "&&label" which is a GNU extension: It should return the address of the next instruction right behind the label...

[dsandersllvm]

I don't see anything taking the address of the
block so the IR Translator most likely failed on that.

That's probably "&&label" which is a GNU extension: It should return the
address of the next instruction right behind the label...

I mean I don't see it in the MIR. The DAGISel pass has blockaddress() operands referring to %ir-block.1 but GlobalISel's pass doesn't.

[qcolombet]

The strange thing is that the FailedISel attribute causes the function to be
reset before DAGISel runs on it so you should see the same behaviour between
DAGISel-only and GlobalISel that fell back to DAGISel. Could there be some
state that the ResetMachineFunction pass isn't resetting?

I agree with Daniel, when GISel falls back we should get the same output as DAGISel-only. If that's not the case, that's a bug.

[hahnjo]

The strange thing is that the FailedISel attribute causes the function to be
reset before DAGISel runs on it so you should see the same behaviour between
DAGISel-only and GlobalISel that fell back to DAGISel. Could there be some
state that the ResetMachineFunction pass isn't resetting?

I agree with Daniel, when GISel falls back we should get the same output as
DAGISel-only. If that's not the case, that's a bug.

Okay, do you want to move this bug to GlobalISel and update the information accordingly?