Skip to content

[wasm] Disallow tail calls when passing structs on the stack #124443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anematode
Copy link
Contributor

The following code is incorrectly compiled with -mtail-call:

struct passed_by_value { int a; int b; };
int f(struct passed_by_value val);
int g(struct passed_by_value in) {
    return f(in);
}

Output:

g:
        global.get      __stack_pointer
        i32.const       16
        i32.sub 
        local.tee       1
        global.set      __stack_pointer
        local.get       1
        local.get       0
        i64.load        0:p2align=2
        i64.store       8
        local.get       1
        i32.const       16
        i32.add 
        global.set      __stack_pointer
        local.get       1
        i32.const       8
        i32.add 
        return_call     f
        end_function

Stack space is reserved for the struct arguments, but the stack pointer is moved back before the return_call. So I added a check to prevent TCO from happening if we're passing things on the stack.

@llvmbot
Copy link
Member

llvmbot commented Jan 26, 2025

@llvm/pr-subscribers-backend-webassembly

Author: Timothy Herchen (anematode)

Changes

The following code is incorrectly compiled with -mtail-call:

struct passed_by_value { int a; int b; };
int f(struct passed_by_value val);
int g(struct passed_by_value in) {
    return f(in);
}

Output:

g:
        global.get      __stack_pointer
        i32.const       16
        i32.sub 
        local.tee       1
        global.set      __stack_pointer
        local.get       1
        local.get       0
        i64.load        0:p2align=2
        i64.store       8
        local.get       1
        i32.const       16
        i32.add 
        global.set      __stack_pointer
        local.get       1
        i32.const       8
        i32.add 
        return_call     f
        end_function

Stack space is reserved for the struct arguments, but the stack pointer is moved back before the return_call. So I added a check to prevent TCO from happening if we're passing things on the stack.


Full diff: https://github.com/llvm/llvm-project/pull/124443.diff

1 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp (+9)
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
index 02db1b142a22b5..ec745ebabb4f63 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
@@ -1211,6 +1211,15 @@ WebAssemblyTargetLowering::LowerCall(CallLoweringInfo &CLI,
         }
       }
     }
+
+    // If outgoing arguments are passed via the stack, we cannot tail call
+    for (const ISD::OutputArg &Out : CLI.Outs) {
+      if (Out.Flags.isByVal() && Out.Flags.getByValSize() != 0) {
+        NoTail(
+            "WebAssembly does not support tail calling with stack arguments");
+        break;
+      }
+    }
   }
 
   SmallVectorImpl<ISD::InputArg> &Ins = CLI.Ins;

@anematode
Copy link
Contributor Author

This test fails:

declare i32 @quux(ptr byval(i32))
define i32 @mismatched_byval(ptr %x) {
; CHECK-LABEL: mismatched_byval:
; CHECK:         .functype mismatched_byval (i32) -> (i32)
; CHECK-NEXT:  # %bb.0:
; CHECK-NEXT:    global.get $push1=, __stack_pointer
; CHECK-NEXT:    i32.const $push2=, 16
; CHECK-NEXT:    i32.sub $push8=, $pop1, $pop2
; CHECK-NEXT:    local.tee $push7=, $1=, $pop8
; CHECK-NEXT:    global.set __stack_pointer, $pop7
; CHECK-NEXT:    i32.load $push0=, 0($0)
; CHECK-NEXT:    i32.store 12($1), $pop0
; CHECK-NEXT:    i32.const $push3=, 16
; CHECK-NEXT:    i32.add $push4=, $1, $pop3
; CHECK-NEXT:    global.set __stack_pointer, $pop4
; CHECK-NEXT:    i32.const $push5=, 12
; CHECK-NEXT:    i32.add $push6=, $1, $pop5
; CHECK-NEXT:    return_call quux, $pop6
  %v = tail call i32 @quux(ptr byval(i32) %x)
  ret i32 %v
}

but it's not clear to me that the test is compiled correctly in the first place. Isn't this calling quux with a pointer to (sp - 4)?

@sunfishcode
Copy link
Member

@tlively Would you be able to investigate this?

tlively added a commit to tlively/llvm-project that referenced this pull request Jan 31, 2025
WebAssembly disallows tail calls with stack-allocated arguments because
they tail calls leave no way to fix up the stack pointer after the call.
WebAssembly also passes `byval` arguments on the stack, but we were not
considering them when deciding whether a tail call should be allowed.
Properly disallow tail calls in the presence of byval arguments and fix
the existing test that should have caught this.

Fixes llvm#124443.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants