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.

Sign up for GitHub

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

Jump to bottom

[Wasm-GC] Generate correct LLInt code for structs containing reference types #10342

Merged
merged 1 commit into from Mar 21, 2023
Merged

[Wasm-GC] Generate correct LLInt code for structs containing reference types #10342

merged 1 commit into from Mar 21, 2023

Conversation

catamorphism
Copy link
Contributor

@catamorphism catamorphism commented Feb 19, 2023 •
edited by webkit-early-warning-system

400ec97

[Wasm-GC] Generate correct LLInt code for structs containing reference types
https://bugs.webkit.org/show_bug.cgi?id=252538

Reviewed by Justin Michaud.

The LLInt generated code for `addStructNew` only worked if the struct
initializers weren't on the stack, as it overwrote live stack slots.
Fixed it to not overwrite live data.

* JSTests/wasm/gc/bug252538.js: Added.
(module):
(testStructOfInts):
(testStructDeclaration):
* Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:
(JSC::Wasm::LLIntGenerator::addStructNew):
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h:
(JSC::Wasm::structNew):

Canonical link: https://commits.webkit.org/261902@main

8635254

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch-sim ✅ 🛠 jsc-mips
✅ 🧪 jsc-mips-tests

@catamorphism catamorphism requested a review from a team as a code owner February 19, 2023 05:18
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash dcec6d4)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@catamorphism catamorphism added the WebAssembly For bugs in JavaScript WebAssembly label Feb 22, 2023
@catamorphism catamorphism mentioned this pull request Feb 27, 2023
Merged
justinmichaud
justinmichaud reviewed Feb 28, 2023
View reviewed changes
Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp Outdated
@@ -2000,17 +2000,41 @@ auto LLIntGenerator::addStructNew(uint32_t index, Vector<ExpressionType>& args,
{
result = push();

// We have to shift all the arguments up the stack by one slot, because we just
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a new dummy argument to avoid this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that adding an extra argument helps. Suppose we enter addStructNew() with 3 arguments and the stack depth (before pushing the arguments) is 18. If these arguments are non-constants, they will already have been assigned to locations 19, 20, and 21. The dummy argument would either have to be 22 or 18, but there's no way to use 18 since that's deeper on the stack. If we use 22, the stack consistency checker will complain, or at least I couldn't find a way to do that without getting assertion failures.

I realized I could change it to just re-use the first argument for the return value (and special-case the 0-argument case). The previous version avoided overwriting any of the arguments, but I don't think that was necessary. The new version doesn't seem as clean, but all the tests pass.

justinmichaud
justinmichaud requested changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@justinmichaud justinmichaud left a comment

Choose a reason for hiding this comment

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

Pinged you on slack

@catamorphism catamorphism self-assigned this Feb 28, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 48dcb94)

Misc iOS, tvOS & watchOS macOS Linux Windows
loading 🧪 style ⏳ 🛠 ios ⏳ 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 wincairo
⏳ 🧪 bindings ⏳ 🛠 ios-sim ⏳ 🛠 mac-AS-debug ⏳ 🛠 gtk
⏳ 🧪 webkitperl ⏳ 🧪 ios-wk2 ⏳ 🧪 api-mac ⏳ 🧪 gtk-wk2
⏳ 🧪 webkitpy ⏳ 🧪 api-ios ⏳ 🧪 mac-wk1 ⏳ 🧪 api-gtk
loading 🛠 🧪 jsc ⏳ 🛠 tv ⏳ 🧪 mac-wk2 ⏳ 🛠 jsc-armv7
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv-sim ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🧪 jsc-armv7-tests
loading 🧪 services ⏳ 🛠 watch ⏳ 🧪 mac-wk2-stress ⏳ 🛠 jsc-mips
⏳ 🛠 watch-sim ⏳ 🧪 jsc-mips-tests

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 48dcb94)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ❌ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch-sim ✅ 🛠 jsc-mips
✅ 🧪 jsc-mips-tests

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 8fc1e60)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ❌ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch-sim ✅ 🛠 jsc-mips
✅ 🧪 jsc-mips-tests

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 15, 2023
justinmichaud
justinmichaud approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@justinmichaud justinmichaud left a comment

Choose a reason for hiding this comment

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

r=me

@catamorphism catamorphism removed the merging-blocked Applied to prevent a change from being merged label Mar 17, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 8635254)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🛠 tv ✅ 🧪 mac-wk2 ⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge ✅ 🛠 watch-sim ✅ 🛠 jsc-mips
✅ 🧪 jsc-mips-tests

@catamorphism catamorphism added the request-merge-queue Request a pull request to be added to merge-queue once ready label Mar 20, 2023
@takikawa takikawa added merge-queue Applied to send a pull request to merge-queue and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Mar 21, 2023
@catamorphism @takikawa
[Wasm-GC] Generate correct LLInt code for structs containing referenc…
400ec97 
…e types

https://bugs.webkit.org/show_bug.cgi?id=252538

Reviewed by Justin Michaud.

The LLInt generated code for `addStructNew` only worked if the struct
initializers weren't on the stack, as it overwrote live stack slots.
Fixed it to not overwrite live data.

* JSTests/wasm/gc/bug252538.js: Added.
(module):
(testStructOfInts):
(testStructDeclaration):
* Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:
(JSC::Wasm::LLIntGenerator::addStructNew):
* Source/JavaScriptCore/wasm/WasmOperationsInlines.h:
(JSC::Wasm::structNew):

Canonical link: https://commits.webkit.org/261902@main
@webkit-commit-queue webkit-commit-queue merged commit 400ec97 into WebKit:main Mar 21, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 261902@main (400ec97): https://commits.webkit.org/261902@main

Reviewed commits have been landed. Closing PR #10342 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 21, 2023
@catamorphism catamorphism deleted the bug252538 branch March 27, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmichaud justinmichaud justinmichaud approved these changes

Assignees

@catamorphism catamorphism

Labels
WebAssembly For bugs in JavaScript WebAssembly
Projects
None yet
Milestone
No milestone
6 participants
@catamorphism @webkit-early-warning-system @webkit-commit-queue @justinmichaud @takikawa @webkit-ews-buildbot