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

winch(trampolines): Save SP, FP and return address #6400

Merged

Conversation

saulecabrera
Copy link
Member

This change is a follow-up to #6358

This change implements the necessary stores of SP, FP and return address for fast stack walking.

This change is a follow-up to bytecodealliance#6358

This change implements the necessary stores of SP, FP and return address
for fast stack walking.
@saulecabrera saulecabrera requested review from a team as code owners May 17, 2023 18:04
@saulecabrera saulecabrera requested review from jameysharp and alexcrichton and removed request for a team May 17, 2023 18:04
@saulecabrera saulecabrera enabled auto-merge May 17, 2023 18:10
@saulecabrera saulecabrera added this pull request to the merge queue May 17, 2023
@github-actions github-actions bot added the winch Winch issues or pull requests label May 17, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @saulecabrera

This issue or pull request has been labeled: "winch"

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

  • saulecabrera: winch

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

Learn more.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 17, 2023
@alexcrichton alexcrichton added this pull request to the merge queue May 17, 2023
@alexcrichton
Copy link
Member

I think that failure may be a bug with github's merge queue...

@saulecabrera
Copy link
Member Author

Oh that makes sense; I was taking a look to see if it was a failure due my changes, but all jobs were marked as cancelled.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 17, 2023
@alexcrichton
Copy link
Member

It's a bit odd but I think that one is legitimate. I think MSVC failed there with 127, which may mean that there's something Windows-specific about the failure.

@saulecabrera
Copy link
Member Author

Yep, I see it; I'll take a look!

@saulecabrera
Copy link
Member Author

I spent some time debugging this issue and it's indeed legitimate. The root cause of the failure in MSVC is the lack of unwind info, which as far as I understand is required in Windows; I can reproduce the exact same failure by disabling the unwind info generation in Cranelift. I'm working on a separate PR to add support for emitting unwind info in Winch.

@alexcrichton
Copy link
Member

Ah that sounds familiar! If you want to add #[cfg_attr(windows, ignore)] now for those tests with a comment I think that'd be sufficient.

@saulecabrera
Copy link
Member Author

saulecabrera commented May 22, 2023

Sounds good, yeah, I'll ignore them to land this PR and put them back once I introduce support for emitting the unwind information.

Temporarily ignoring Winch's trap test on Windows while
support for unwind information is added.
@saulecabrera saulecabrera force-pushed the winch-save-sp-fp-ret-addr branch from 75d9eae to 8d75bdc Compare May 22, 2023 18:45
@saulecabrera saulecabrera added this pull request to the merge queue May 22, 2023
Merged via the queue into bytecodealliance:main with commit b25fe20 May 22, 2023
@saulecabrera saulecabrera deleted the winch-save-sp-fp-ret-addr branch May 22, 2023 19:53
salewski pushed a commit to salewski/wasmtime that referenced this pull request Jun 30, 2023
…6400)

This change is a follow-up to bytecodealliance#6358

This change implements the necessary stores of SP, FP and return address
for fast stack walking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants