Skip to content
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

Append lld --stack-first on wasm targets #5501

Merged
merged 1 commit into from
Jul 27, 2020
Merged

Append lld --stack-first on wasm targets #5501

merged 1 commit into from
Jul 27, 2020

Conversation

zigazeljko
Copy link
Contributor

Closes #4496

@fengb
Copy link
Contributor

fengb commented Jun 1, 2020

Comparison:

; Before
(global (;stack_start;) (mut i32) (i32.const 66576))
(global (;quuz;) i32 (i32.const 1028))

; After
(global (;stack_start;) (mut i32) (i32.const 65536))
(global (;quuz;) i32 (i32.const 65540))

Looks good! 👍

@fengb
Copy link
Contributor

fengb commented Jun 1, 2020

I think the CI failure is exposing an existing bug.

    0: failed to invoke `_start`
    1: wasm trap: out of bounds memory access, source location: @856d24

This looks like a stack overflow. The current output "pads the stack" with globals — we accidentally consume too much stack space but it's simply stealing/corrupting memory from globals.

@kubkon
Copy link
Member

kubkon commented Jun 2, 2020

I think the CI failure is exposing an existing bug.

    0: failed to invoke `_start`
    1: wasm trap: out of bounds memory access, source location: @856d24

This looks like a stack overflow. The current output "pads the stack" with globals — we accidentally consume too much stack space but it's simply stealing/corrupting memory from globals.

Interesting. I've noticed the same buggy behaviour when troubleshooting the disabled tests in std.packed_int_array, and it definitely looks like a stack overflow. For the record, this bug can easily be replicated with increasing the quota with @setEvalBranchQuota and inlining some loop in comptime.

Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

I'm not sure what others reckon but IMHO skipping the failing tests in Wasm is OK for now until we figure out what the exact cause is and how to rectify it (a subject of a subsequent PR I guess?). @fengb thoughts?

@fengb
Copy link
Contributor

fengb commented Jun 2, 2020

The medium term solution is probably adding a manual stack size override: #3735 (comment). In the meantime, I think skipping it is okay.

@kubkon
Copy link
Member

kubkon commented Jun 4, 2020

FYI, the CI failure should go away after we merge in #5529 which increases the default stack size to 1MB.

@zigazeljko
Copy link
Contributor Author

Since #5529 got merged, can this get merged too?

@kubkon
Copy link
Member

kubkon commented Jul 4, 2020

SGTM! Could you rebase to the current upstream and see if the tests now pass though?

@zigazeljko
Copy link
Contributor Author

@kubkon Done. Looks like tests are now passing.

@andrewrk andrewrk merged commit bf273b7 into ziglang:master Jul 27, 2020
@zigazeljko zigazeljko deleted the patch-1 branch July 27, 2020 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasm targets should append lld --stack-first
5 participants