Created attachment 21994 [details] Reproducer When compiling the attached reproducer, Clang emits a vtable thunk (called from the GetStats method) which looks like this: A::`vcall'{0}': 00007FF7124C10FC sub sp,sp,#0x40 00007FF7124C1100 stp x1,x2,[sp,#8] 00007FF7124C1104 stp x3,x4,[sp,#0x18] 00007FF7124C1108 stp x5,x6,[sp,#0x28] 00007FF7124C110C str x7,[sp,#0x38] 00007FF7124C1110 ldr x8,[x0] 00007FF7124C1114 ldr x8,[x8] 00007FF7124C1118 add sp,sp,#0x40 00007FF7124C111C br x8 This doesn't work when the function expects to return something via x8. Attempting to do so will result in a crash. For contrast, this is what MSVC produces: 00007FF6C59F1080 ldr xip0,[x0] 00007FF6C59F1084 ldr xip0,[xip0] 00007FF6C59F1088 br xip0 Full arguments are: "clang-cl.exe" "-cc1" "-triple" "aarch64-pc-windows-msvc19.16.27030" "-emit-obj" "-mincremental-linker-compatible" "-disable-free" "-main-file-name" "Clang-Reproducer-x8.cpp" "-mrelocation-model" "static" "-mthread-model" "posix" "-relaxed-aliasing" "-fmath-errno" "-masm-verbose" "-mconstructor-aliases" "-munwind-tables" "-target-cpu" "generic" "-target-feature" "+neon" "-target-abi" "aapcs" "-fallow-half-arguments-and-returns" "-D_MT" "-flto-visibility-public-std" "--dependent-lib=libcmt" "--dependent-lib=oldnames" "-stack-protector" "2" "-fdiagnostics-format" "msvc" "-gcodeview" "-debug-info-kind=limited" "-momit-leaf-frame-pointer" "-ffunction-sections" "-O2" "-fdeprecated-macro" "-ferror-limit" "19" "-fmessage-length" "120" "-fno-use-cxa-atexit" "-fms-extensions" "-fms-compatibility" "-fms-compatibility-version=19.16.27030" "-std=c++14" "-fdelayed-template-parsing" "-fobjc-runtime=gcc" "-fdiagnostics-show-option" "-fcolor-diagnostics" "-vectorize-loops" "-vectorize-slp" "-x" "c++" "Clang-Reproducer-x8.cpp" "-faddrsig"
X86 has the same issue around AL and varargs functions, so I applied the same fix. The generated code shifts to use X9. I'll send the patch for review. $ git diff diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp index 4b027e93633..ac656959bcb 100644 --- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp +++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp @@ -3207,6 +3207,12 @@ SDValue AArch64TargetLowering::LowerFormalArguments( FuncInfo->getForwardedMustTailRegParms(); CCInfo.analyzeMustTailForwardedRegisters(Forwards, RegParmTypes, CC_AArch64_AAPCS); + + // Conservatively forward X8, since it might be used for aggregate return. + if (!CCInfo.isAllocated(AArch64::X8)) { + unsigned X8VReg = MF.addLiveIn(AArch64::X8, &AArch64::GPR64RegClass); + Forwards.push_back(ForwardedRegister(X8VReg, AArch64::X8, MVT::i64)); + } } } rnk@rnk5-w MINGW64 /c/src/llvm-project/build (master) $ diff -u t-before.s t-after.s --- t-before.s 2019-05-23 13:34:44.447222500 -0700 +++ t-after.s 2019-05-23 13:35:43.614976800 -0700 @@ -196,10 +196,10 @@ stp x3, x4, [sp, #24] stp x5, x6, [sp, #40] str x7, [sp, #56] - ldr x8, [x0] - ldr x8, [x8] + ldr x9, [x0] + ldr x9, [x9] add sp, sp, #64 ; =64 - br x8 + br x9 .Lfunc_end5: .section .xdata,"dr",associative,"??_9A@@$BA@AA" .seh_handlerdata @@ -371,7 +371,7 @@ .hword 0 .hword 0 .hword 0 - .asciz "clang version 9.0.0 (git@github.com:llvm/llvm-project.git e539eae91de2a91d42ecfce451ba517ed0ebb108)" ; Null-terminated compiler version string + .asciz "clang version 9.0.0 (git@github.com:llvm/llvm-project.git a0fd5218c2747bf89645d3d732d166ca580a572d)" ; Null-terminated compiler version string .p2align 2 .Ltmp19:
Does fix the problem with the reproducer, I'm just rebuilding the compiler + Chromium to double-check that it fixes the original issue...
The fix from Reid would work. Tail call with vararg should be conservative for input parameters and assume sret is possible, so mark x8 as forwarded register makes sense in such case.
Just built and played with a browser built with that patch applied (no longer crashing :D). Reid, could you add me to the CC when you upload?
(In reply to Richard Townsend from comment #4) > Just built and played with a browser built with that patch applied (no > longer crashing :D). > > Reid, could you add me to the CC when you upload? Sorry, I spent a while trying to find your Phab username and eventually just mashed send on the review. It was at https://reviews.llvm.org/D62344, I cc'd you on it just before landing it. It became r361585. By the way, I was told that arm64 debug builds of chromium don't work at all. And we probably need to do something about CodeView emission. I'd be surprised if debug info works just out of the box. If you want to test those configurations and file more bugs, that'd be helpful.
Debug builds currently work for me: building Clang with the official Google scripts and r361562 + your patch produces a debug mode binary that doesn't crash, and there are no compiler crashes when building in debug mode like there were a month or so ago. You don't get any local variable values whilst debugging (at least not in Visual Studio 2017), how important is that for you?
(In reply to Richard Townsend from comment #6) > Debug builds currently work for me: building Clang with the official Google > scripts and r361562 + your patch produces a debug mode binary that doesn't > crash, and there are no compiler crashes when building in debug mode like > there were a month or so ago. Great, thanks. > You don't get any local variable values whilst > debugging (at least not in Visual Studio 2017), how important is that for > you? Well, I imagine that users will start asking for it sooner than later, and I'd rather get started on it before it becomes an issue. I think we can probably make headway simply by comparing clang's symbol records to MSVC's when cross-compiling for ARM64.
Excellent, shall we monitor that work in a new ticket?
(In reply to Richard Townsend from comment #8) > Excellent, shall we monitor that work in a new ticket? I filed https://bugs.llvm.org/show_bug.cgi?id=42048 for it.