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] Use correct offsets when generating code for struct gets and sets in B3 and Air #10490

Merged
merged 1 commit into from Mar 21, 2023
Merged

[Wasm-GC] Use correct offsets when generating code for struct gets and sets in B3 and Air #10490

merged 1 commit into from Mar 21, 2023

Conversation

catamorphism
Copy link
Contributor

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

2e2ee48

[Wasm-GC] Use correct offsets when generating code for struct gets and sets in B3 and Air
https://bugs.webkit.org/show_bug.cgi?id=252719

Reviewed by Tadeu Zagallo and Justin Michaud.

The generated code for struct get and set operations was using the wrong offsets and overwriting
the header for the struct object's `m_payload.storage` field. Triggering the bug requires
a function call where the callee returns a struct and the caller performs a `struct.get` on the
result, and the callee is interpreted while the caller is compiled (or vice versa).

* JSTests/wasm/gc/bug252719.js: Added.
(module):
(testIntFields):
* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::ExpressionType>::addStructGet):
(JSC::Wasm::ExpressionType>::addStructSet):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::emitStructSet):
(JSC::Wasm::B3IRGenerator::addStructGet):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp:
(JSC::Wasm::StructType::StructType):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.h:
(JSC::Wasm::StructType::offsetOfField const):
(JSC::Wasm::StructType::offsetOfField):
(JSC::Wasm::StructType::getFieldOffset const): Deleted.
(JSC::Wasm::StructType::getFieldOffset): Deleted.
* Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.cpp:
(JSC::JSWebAssemblyStruct::fieldPointer const):
* Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.h:

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

848946b

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 22, 2023 04:02
@catamorphism catamorphism self-assigned this Feb 22, 2023
@catamorphism catamorphism added the WebAssembly For bugs in JavaScript WebAssembly label Feb 22, 2023
@webkit-early-warning-system
Copy link
Collaborator

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

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

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 9521d87)

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 request-merge-queue Request a pull request to be added to merge-queue once ready label Feb 24, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 3633d03)

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

justinmichaud
justinmichaud requested changes Feb 28, 2023
View reviewed changes
Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h Outdated
self().emitLoad(structBase, JSWebAssemblyStruct::offsetOfPayload(), payload);

uint32_t fieldOffset = fixupPointerPlusOffset(payload, *structType.getFieldOffset(fieldIndex));
// Add offset(data) to get structBase.m_payload.m_storage.data()
uint32_t fieldOffset = JSWebAssemblyStruct::offsetOfPayloadData() + fixupPointerPlusOffset(payload, *structType.getFieldOffset(fieldIndex));
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be inside the fixup check in case it overflows

Copy link
Contributor

Choose a reason for hiding this comment

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

getFieldOffset should be offsetOfField, and should include the wasm struct offset of payload data.

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 made this change, but as a result, I had to change JSWebAssemblyStruct::fieldPointer() to explicitly subtract the offset of the payload data, since StructType::offsetOfField() now returns an offset relative to m_payload.m_storage, but fieldPointer() uses m_payload.data() as the base. I tried replacing m_payload.data() with m_payload.storage(), but couldn't get the casts to work while preserving the const return value of the method. It's kind of ugly, but if you have any suggestions, let me know.

Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h Outdated
self().emitLoad(structBase, JSWebAssemblyStruct::offsetOfPayload(), payload);
// Add offset(data) to get structBase.m_payload.m_storage.data()
uint32_t fieldOffset = JSWebAssemblyStruct::offsetOfPayloadData() + fixupPointerPlusOffset(payload, *structType.getFieldOffset(fieldIndex));
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@catamorphism catamorphism removed the request-merge-queue Request a pull request to be added to merge-queue once ready label Feb 28, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 0b39d7c)

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 3cfc57e)

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 16, 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

Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.cpp Outdated
@@ -69,7 +69,8 @@ JSWebAssemblyStruct* JSWebAssemblyStruct::tryCreate(JSGlobalObject* globalObject

const uint8_t* JSWebAssemblyStruct::fieldPointer(uint32_t fieldIndex) const
{
return m_payload.data() + *structType()->getFieldOffset(fieldIndex);
// offsetOfField() returns an offset relative to `m_payload`; subtract offsetOfData()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just make a separate getter for this

@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 848946b)

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
loading 🛠 🧪 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 17, 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 20, 2023
@catamorphism @takikawa
[Wasm-GC] Use correct offsets when generating code for struct gets an…
2e2ee48 
…d sets in B3 and Air

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

Reviewed by Tadeu Zagallo and Justin Michaud.

The generated code for struct get and set operations was using the wrong offsets and overwriting
the header for the struct object's `m_payload.storage` field. Triggering the bug requires
a function call where the callee returns a struct and the caller performs a `struct.get` on the
result, and the callee is interpreted while the caller is compiled (or vice versa).

* JSTests/wasm/gc/bug252719.js: Added.
(module):
(testIntFields):
* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::ExpressionType>::addStructGet):
(JSC::Wasm::ExpressionType>::addStructSet):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::emitStructSet):
(JSC::Wasm::B3IRGenerator::addStructGet):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp:
(JSC::Wasm::StructType::StructType):
* Source/JavaScriptCore/wasm/WasmTypeDefinition.h:
(JSC::Wasm::StructType::offsetOfField const):
(JSC::Wasm::StructType::offsetOfField):
(JSC::Wasm::StructType::getFieldOffset const): Deleted.
(JSC::Wasm::StructType::getFieldOffset): Deleted.
* Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.cpp:
(JSC::JSWebAssemblyStruct::fieldPointer const):
* Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.h:

Canonical link: https://commits.webkit.org/261899@main
@webkit-commit-queue webkit-commit-queue merged commit 2e2ee48 into WebKit:main Mar 21, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 261899@main (2e2ee48): https://commits.webkit.org/261899@main

Reviewed commits have been landed. Closing PR #10490 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 bug252719 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

@tadeuzagallo tadeuzagallo tadeuzagallo approved these changes

Assignees

@catamorphism catamorphism

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