[X86] Fix musttail miscompilation when arguments are passed on the stack#199691
Conversation
|
Hello @mmasztalerczuk 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-backend-x86 Author: Mariusz Masztalerczuk (mmasztalerczuk) ChangesAfter commit 782bf6a, a musttail call with matching CC was always treated as a sibcall, which skips the stores of outgoing stack arguments. Any non-forwarded stack argument was silently dropped. Only treat musttail as a sibcall when every argument is in a register; otherwise fall back to full tail-call lowering. Fix #199224 Full diff: https://github.com/llvm/llvm-project/pull/199691.diff 2 Files Affected:
diff --git a/llvm/lib/Target/X86/X86ISelLoweringCall.cpp b/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
index 65d77769b3c45..0d570173c0e6a 100644
--- a/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
+++ b/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
@@ -2124,7 +2124,18 @@ X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
// function prototype.
CallingConv::ID CallerCC = MF.getFunction().getCallingConv();
isTailCall = (CallConv == CallerCC);
- IsSibcall = IsMustTail;
+ // Treat musttail as a sibcall only when all arguments are in registers;
+ // otherwise full tail-call lowering must run to populate the outgoing
+ // stack slots.
+ if (isTailCall && IsMustTail) {
+ IsSibcall = true;
+ for (const CCValAssign &VA : ArgLocs) {
+ if (!VA.isRegLoc()) {
+ IsSibcall = false;
+ break;
+ }
+ }
+ }
} else if (isTailCall) {
// Check if this tail call is a "sibling" call, which is loosely defined to
// be a tail call that doesn't require heroics like moving the return
diff --git a/llvm/test/CodeGen/X86/musttail-tailcc.ll b/llvm/test/CodeGen/X86/musttail-tailcc.ll
index f1ffbcb1142c5..b366416b7aec5 100644
--- a/llvm/test/CodeGen/X86/musttail-tailcc.ll
+++ b/llvm/test/CodeGen/X86/musttail-tailcc.ll
@@ -55,6 +55,15 @@ define dso_local tailcc void @void_test(i32, i32, i32, i32) {
;
; X86-LABEL: void_test:
; X86: # %bb.0: # %entry
+; X86-NEXT: pushl %esi
+; X86-NEXT: .cfi_def_cfa_offset 8
+; X86-NEXT: .cfi_offset %esi, -8
+; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movl {{[0-9]+}}(%esp), %esi
+; X86-NEXT: movl %esi, {{[0-9]+}}(%esp)
+; X86-NEXT: movl %eax, {{[0-9]+}}(%esp)
+; X86-NEXT: popl %esi
+; X86-NEXT: .cfi_def_cfa_offset 4
; X86-NEXT: jmp void_test # TAILCALL
entry:
musttail call tailcc void @void_test( i32 %0, i32 %1, i32 %2, i32 %3)
@@ -68,8 +77,59 @@ define dso_local tailcc i1 @i1test(i32, i32, i32, i32) {
;
; X86-LABEL: i1test:
; X86: # %bb.0: # %entry
+; X86-NEXT: pushl %esi
+; X86-NEXT: .cfi_def_cfa_offset 8
+; X86-NEXT: .cfi_offset %esi, -8
+; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movl {{[0-9]+}}(%esp), %esi
+; X86-NEXT: movl %esi, {{[0-9]+}}(%esp)
+; X86-NEXT: movl %eax, {{[0-9]+}}(%esp)
+; X86-NEXT: popl %esi
+; X86-NEXT: .cfi_def_cfa_offset 4
; X86-NEXT: jmp i1test # TAILCALL
entry:
%4 = musttail call tailcc i1 @i1test( i32 %0, i32 %1, i32 %2, i32 %3)
ret i1 %4
}
+
+; Regression test: musttail tailcc with non-forwarded stack args.
+declare tailcc void @f1_64(i64, i64, i64, i64, i64, i64, i64)
+
+define tailcc void @stack_arg_const_64(i64, i64, i64, i64, i64, i64, i64) {
+; X86-LABEL: stack_arg_const_64:
+; X86: # %bb.0:
+; X86-NEXT: movl $4, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $0, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $8, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $0, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $15, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $0, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $16, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $0, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $23, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $0, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $42, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $0, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $1, %ecx
+; X86-NEXT: xorl %edx, %edx
+; X86-NEXT: jmp f1_64@PLT # TAILCALL
+ musttail call tailcc void @f1_64(i64 1, i64 4, i64 8, i64 15, i64 16, i64 23, i64 42)
+ ret void
+}
+
+declare tailcc void @f1_32(i32, i32, i32, i32, i32, i32, i32)
+
+define tailcc void @stack_arg_const_32(i32, i32, i32, i32, i32, i32, i32) {
+; X86-LABEL: stack_arg_const_32:
+; X86: # %bb.0:
+; X86-NEXT: movl $8, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $15, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $16, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $23, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $42, {{[0-9]+}}(%esp)
+; X86-NEXT: movl $1, %ecx
+; X86-NEXT: movl $4, %edx
+; X86-NEXT: jmp f1_32@PLT # TAILCALL
+ musttail call tailcc void @f1_32(i32 1, i32 4, i32 8, i32 15, i32 16, i32 23, i32 42)
+ ret void
+}
|
| // Treat musttail as a sibcall only when all arguments are in registers; | ||
| // otherwise full tail-call lowering must run to populate the outgoing | ||
| // stack slots. | ||
| if (isTailCall && IsMustTail) { |
There was a problem hiding this comment.
It's better to use llvm::all_of here to make it clear.
refer: https://llvm.org/doxygen/STLExtras_8h.html
There was a problem hiding this comment.
I made changes.
it could be also simplify it to
IsSibcall = isTailCall && IsMustTail &&
llvm::all_of(ArgLocs, [](const CCValAssign &VA) {
return VA.isRegLoc();
});
but I'm not sure if that doesn't make it a bit less readable.
After commit 782bf6a, a musttail call with matching CC was always treated as a sibcall, which skips the stores of outgoing stack arguments. Any non-forwarded stack argument was silently dropped. Only treat musttail as a sibcall when every argument is in a register; otherwise fall back to full tail-call lowering.
f9b1df1 to
ac0ceab
Compare
| ; Regression test: musttail tailcc with non-forwarded stack args. | ||
| declare tailcc void @f1_64(i64, i64, i64, i64, i64, i64, i64) |
There was a problem hiding this comment.
So, what about this is actually specific to tailcc? Can you copy these tests over to llvm/test/CodeGen/X86/sibcall.ll, remove the use of tailcc and see what that generates. It looks like we don't actually test with more than 6 arguments there.
Currently it emits https://clang.godbolt.org/z/G6MTThzbG which looks right to me?
There was a problem hiding this comment.
You're right, bug is specific to calling conventions with ShouldGuaranteeTCO, for ccc musttail we go through isEligibleForSiblingCallOpt which returns false for constants, so the stack arguments are stored correctly.
There was a problem hiding this comment.
I added musttail test with 7 i64 / 7 i32 stack args (using default CC) to sibcall.ll to cover the >6 args case you noted :)
ac0ceab to
a027f67
Compare
folkertdev
left a comment
There was a problem hiding this comment.
LGTM
maybe wait a couple of days to see if other reviewers have thoughts
| // otherwise full tail-call lowering must run to populate the outgoing | ||
| // stack slots. | ||
| if (isTailCall && IsMustTail) | ||
| IsSibcall = llvm::all_of( |
There was a problem hiding this comment.
This is hand-rolling the eligibility check (isEligibleForSiblingCallOpt). We shouldn't do that, we should just call it. Why did we add this if block to bypass the eligibility check in the first place? It's not present in PR #168956 . I'll dig to find out why, but leaving feedback for now.
rnk
left a comment
There was a problem hiding this comment.
Can you pull this change? I think these conditionals are simpler:
main...rnk:llvm-project:fix-tailcc-musttail
I don't want to open a PR and take the authorship credit, I'd rather you keep it, I just find it easier to express the feedback in code.
Split the guarantee-TCO check from the sibling-call eligibility check in LowerCall, so the predicate reads isTailCall = IsSibcall || IsMustTail || ShouldGuaranteeTCO Guaranteed-TCO callers that are not sibcall-eligible (notably musttail+tailcc with stack arguments) now go through full tail-call lowering via the explicit ShouldGuaranteeTCO disjunct rather than the previous all-register check on musttail. Update assembly checks in hipe-cc64.ll and swifttailcc-store-ret-address-aliasing-stack-slot.ll for the resulting codegen. Co-authored-by: Reid Kleckner <rkleckner@nvidia.com>
thanks, this is way nicer. splitting the TCO check from the sibcall one and ending with IsSibcall || IsMustTail || ShouldGuaranteeTCO is much clearer than what i had. also nice catch on the swifttailcc return-address thing, mine wouldn't have hit that. pushed as a followup with you in Co-authored-by |
rnk
left a comment
There was a problem hiding this comment.
Thanks! I'll push the merge button.
|
@mmasztalerczuk Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…ack (llvm#199691) After commit 782bf6a, a musttail call with matching CC was always treated as a sibcall, which skips the stores of outgoing stack arguments. Any non-forwarded stack argument was silently dropped. Only treat musttail as a sibcall when every argument is in a register; otherwise fall back to full tail-call lowering. Fix llvm#199224 --------- Co-authored-by: Reid Kleckner <rkleckner@nvidia.com>
…ack (llvm#199691) After commit 782bf6a, a musttail call with matching CC was always treated as a sibcall, which skips the stores of outgoing stack arguments. Any non-forwarded stack argument was silently dropped. Only treat musttail as a sibcall when every argument is in a register; otherwise fall back to full tail-call lowering. Fix llvm#199224 --------- Co-authored-by: Reid Kleckner <rkleckner@nvidia.com>
After commit 782bf6a, a musttail call with matching CC was always treated as a sibcall, which skips the stores of outgoing stack arguments. Any non-forwarded stack argument was silently dropped.
Only treat musttail as a sibcall when every argument is in a register; otherwise fall back to full tail-call lowering.
Fix #199224