wasmtime / issue #6635 Cranelift: Implement tail calls fo... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #6635 Cranelift: Implement tail calls fo...

Wasmtime GitHub notifications bot (Jun 23 2023 at 16:47):

github-actions[bot] commented on issue #6635:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

cfallin: isle

fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jun 25 2023 at 21:13):

afonso360 commented on issue #6635:

:wave: Hey, I'm not sure if this is ready or not, but I implemented return call's on fuzzgen (#6641) and it found something.

It is also possible that this is related to #6640, but it feels quite different. Here we get a segfault, and we don't need the stack probe.

<details>
<summary>Testcase</summary>

test run
set preserve_frame_pointers=true
target x86_64

function %b(i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail {
    ss0 = explicit_slot 126
    ss1 = explicit_slot 126
    ss2 = explicit_slot 13

block0(v0: i8, v1: i8, v2: i8, v3: i8, v4: i8, v5: i8, v6: i8, v7: i8, v8: i8, v9: i8, v10: i8, v11: i8, v12: i8, v13: i8, v14: i8, v15: i8):
    return v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v9, v9, v9, v9, v9, v9
}


function %a(i8 uext, i8 uext, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail {
    sig0 = (i8 uext, i8 uext, i8 uext, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail
    fn0 = %b sig0

block0(v0: i8, v1: i8, v2: i8, v3: i8, v4: i8, v5: i8, v6: i8, v7: i8, v8: i8, v9: i8, v10: i8, v11: i8, v12: i8, v13: i8, v14: i8, v15: i8):
    return_call fn0(v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0)
}

; run: %a(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) == [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

</details>

Wasmtime GitHub notifications bot (Jun 25 2023 at 21:13):

afonso360 edited a comment on issue #6635:

:wave: Hey, I'm not sure if this is ready or not, but I implemented return call's on fuzzgen (#6641) and it found something.

It is possible that this is related to #6640, but it feels quite different. Here we get a segfault, and we don't need the stack probe.

<details>
<summary>Testcase</summary>

test run
set preserve_frame_pointers=true
target x86_64

function %b(i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail {
    ss0 = explicit_slot 126
    ss1 = explicit_slot 126
    ss2 = explicit_slot 13

block0(v0: i8, v1: i8, v2: i8, v3: i8, v4: i8, v5: i8, v6: i8, v7: i8, v8: i8, v9: i8, v10: i8, v11: i8, v12: i8, v13: i8, v14: i8, v15: i8):
    return v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v9, v9, v9, v9, v9, v9
}


function %a(i8 uext, i8 uext, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail {
    sig0 = (i8 uext, i8 uext, i8 uext, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail
    fn0 = %b sig0

block0(v0: i8, v1: i8, v2: i8, v3: i8, v4: i8, v5: i8, v6: i8, v7: i8, v8: i8, v9: i8, v10: i8, v11: i8, v12: i8, v13: i8, v14: i8, v15: i8):
    return_call fn0(v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0)
}

; run: %a(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) == [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

</details>

Wasmtime GitHub notifications bot (Jun 26 2023 at 18:04):

fitzgen commented on issue #6635:

@afonso360 still haven't dug in fully yet, but it is interesting that the second function claims the first function's signature is something other than what it the function is declared as: the second function says the first three arguments are uext but the first function doesn't declare that. That shouldn't matter here I don't think but seems like something that fuzzgen shouldn't generate.

Wasmtime GitHub notifications bot (Jun 26 2023 at 18:09):

afonso360 commented on issue #6635:

Oh boy, I removed uext and sext when I was minimizing the test case, but it looks like I accidentally left those in. Please ignore them! Fuzzgen should be generating those correctly, at least I haven't ever caught it not doing so.

I've updated the example above to remove them, and verified that it still reproduces the original issue.

Wasmtime GitHub notifications bot (Jun 26 2023 at 18:10):

afonso360 edited a comment on issue #6635:

:wave: Hey, I'm not sure if this is ready or not, but I implemented return call's on fuzzgen (#6641) and it found something.

It is possible that this is related to #6640, but it feels quite different. Here we get a segfault, and we don't need the stack probe.

<details>
<summary>Testcase</summary>

test run
set preserve_frame_pointers=true
target x86_64

function %b(i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail {
    ss0 = explicit_slot 126
    ss1 = explicit_slot 126
    ss2 = explicit_slot 13

block0(v0: i8, v1: i8, v2: i8, v3: i8, v4: i8, v5: i8, v6: i8, v7: i8, v8: i8, v9: i8, v10: i8, v11: i8, v12: i8, v13: i8, v14: i8, v15: i8):
    return v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v9, v9, v9, v9, v9, v9
}


function %a(i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail {
    sig0 = (i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 tail
    fn0 = %b sig0

block0(v0: i8, v1: i8, v2: i8, v3: i8, v4: i8, v5: i8, v6: i8, v7: i8, v8: i8, v9: i8, v10: i8, v11: i8, v12: i8, v13: i8, v14: i8, v15: i8):
    return_call fn0(v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0)
}

; run: %a(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) == [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

</details>

Wasmtime GitHub notifications bot (Jun 26 2023 at 21:04):

fitzgen commented on issue #6635:

Fixed that fuzz bug, thanks again for reporting it @afonso360!

Turns out that my codegen-the-arguments sequence was accidentally skipping "hidden" extra parameters like return pointers. Fixed!

Wasmtime GitHub notifications bot (Jun 27 2023 at 16:40):

fitzgen commented on issue #6635:

In addition to comments above I think it would be nice to see the "extra ret-addr movement" go away for the overwhelmingly common case (no stack args) either in this PR or in an immediate followup -- it seems to me that it shouldn't be too much work (tens of lines of code? Option<Reg> for ret_addr, and check if old_size == new_size?) and it's an important (in)efficiency to clean up soon when using tail-calls as a sort of fine-grained control flow mechanism (BB jumps between funclets etc).

Will do this in an immediate follow up.

Thanks for the review!

Last updated: Dec 23 2024 at 12:05 UTC