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

[wasm] Fix for getelema1 crash in specific scenarios #87246

Merged
merged 4 commits into from
Jun 8, 2023

Conversation

kg
Copy link
Member

@kg kg commented Jun 8, 2023

There was a latent bug in getelema1 that for some reason triggers only in BenchmarkDotNet, this PR should fix it. This is what I get for using 'const' like eslint constantly reminds me to.

I also improved on the zero page reserved checks so that we permanently disable use of the zero page if we ever see nonzero data there, and if nonzero data appears there after startup we log an error (if this happened your app is probably going to crash.)

kg added 2 commits June 7, 2023 19:55
If we ever see garbage in the zero page disable it permanently
Log an error if garbage appears in the zero page after startup
@kg kg added arch-wasm WebAssembly architecture area-Codegen-Jiterpreter-mono labels Jun 8, 2023
@ghost
Copy link

ghost commented Jun 8, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

There was a latent bug in getelema1 that for some reason triggers only in BenchmarkDotNet, this PR should fix it. This is what I get for using 'const' like eslint constantly reminds me to.

I also improved on the zero page reserved checks so that we permanently disable use of the zero page if we ever see nonzero data there, and if nonzero data appears there after startup we log an error (if this happened your app is probably going to crash.)

Author: kg
Assignees: -
Labels:

arch-wasm, area-Codegen-Jiterpreter-mono

Milestone: -

@ghost ghost assigned kg Jun 8, 2023
@kg
Copy link
Member Author

kg commented Jun 8, 2023

This is apparently still not enough to fix the two BDN benchmarks that broke.

Fix count not being initialized for cpblk in some cases
@kg
Copy link
Member Author

kg commented Jun 8, 2023

There was a second bug (in CPBLK) hidden until recently where under certain circumstances the count local used to control the number of bytes copied wasn't getting initialized, and due to local reuse in traces it could have an arbitrarily large value in it. This was hard to find at first so I ended up just refactoring how traces use locals, renaming some of the scratch locals to have more semantically meaningful names and adding more of them to make it less likely that uses of them will collide. I also changed the memmove and memcpy unrollers to use dedicated scratch locals so it's safe to call them without worrying about them trampling on your locals.

As a result the "math_..." locals should mostly be used for math operations now, and for things like index computations and memory accesses "dest_ptr", "src_ptr", "index" and "count" should be getting used instead.

I probably need to add a system for allocating locals on demand... I don't know if adding these additional locals will make trace stack frames bigger or cause traces to get slower.

@kg
Copy link
Member Author

kg commented Jun 8, 2023

Determined that the old leave-on-stack behavior in the two places I changed was actually correct, so I changed it back. I misunderstood how WebAssembly blocks interact with the stack.

@kg
Copy link
Member Author

kg commented Jun 8, 2023

Previous commit passed on linux (didn't fail this time, just never ran), and the windows lanes passed in both AOT and interp modes, so merging.

@kg kg merged commit 1d5ee1c into dotnet:main Jun 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants