Skip to content

Set temporaryCallFrame in WebAssembly.asm when !USE(BUILTIN_FRAME_ADDRESS) #46839

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Set temporaryCallFrame in WebAssembly.asm when !USE(BUILTIN_FRAME_ADDRESS) #46839

wants to merge 1 commit into from

Conversation

aoikonomopoulos
Copy link
Contributor

@aoikonomopoulos aoikonomopoulos commented Jun 17, 2025 •
edited by webkit-early-warning-system
Loading

d140d36

Set temporaryCallFrame in WebAssembly.asm when !USE(BUILTIN_FRAME_ADDRESS)
https://bugs.webkit.org/show_bug.cgi?id=294597

Reviewed by NOBODY (OOPS!).

When !USE(BUILTIN_FRAME_ADDRESS), DECLARE_WASM_CALL_FRAME() expects a
non-null temporaryCallFrame().

ae10311

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@aoikonomopoulos aoikonomopoulos requested a review from a team as a code owner June 17, 2025 08:30
@aoikonomopoulos aoikonomopoulos self-assigned this Jun 17, 2025
@aoikonomopoulos aoikonomopoulos added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jun 17, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 17, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ❌ 🛠 wpe ⏳ 🛠 win
❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ❌ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ❌ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🛠 gtk
❌ 🛠 🧪 jsc-arm64 ❌ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
❌ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
⏳ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
❌ 🛠 tv ❌ 🛠 mac-safer-cpp ❌ 🛠 jsc-armv7
❌ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 17, 2025
@aoikonomopoulos aoikonomopoulos marked this pull request as draft June 17, 2025 08:39
@aoikonomopoulos aoikonomopoulos force-pushed the eng/Set-temporaryCallFrame-in-WebAssembly-asm-when-USE-BUILTIN_FRAME_ADDRESS branch from d140d36 to d273218 Compare June 17, 2025 08:42
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 17, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ❌ 🛠 wpe ❌ 🛠 win
❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ❌ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
❌ 🛠 🧪 jsc ❌ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🛠 gtk
❌ 🛠 🧪 jsc-arm64 ❌ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
❌ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
⏳ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
❌ 🛠 tv ❌ 🛠 mac-safer-cpp ❌ 🛠 jsc-armv7
❌ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
❌ 🛠 watch
❌ 🛠 watch-sim

@aoikonomopoulos
Set temporaryCallFrame in WebAssembly.asm when !USE(BUILTIN_FRAME_ADD…
ae10311 
…RESS)

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

Reviewed by NOBODY (OOPS!).

When !USE(BUILTIN_FRAME_ADDRESS), DECLARE_WASM_CALL_FRAME() expects a
non-null temporaryCallFrame().
@aoikonomopoulos aoikonomopoulos force-pushed the eng/Set-temporaryCallFrame-in-WebAssembly-asm-when-USE-BUILTIN_FRAME_ADDRESS branch from d273218 to ae10311 Compare June 17, 2025 11:19
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 17, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@aoikonomopoulos aoikonomopoulos marked this pull request as ready for review June 17, 2025 11:56
@aoikonomopoulos aoikonomopoulos removed the merging-blocked Applied to prevent a change from being merged label Jun 17, 2025
Constellation
Constellation reviewed Jun 20, 2025
View reviewed changes
Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h
Comment on lines +173 to +177
#if USE(BUILTIN_FRAME_ADDRESS)
#define OFFLINE_ASM_USE_BUILTIN_FRAME_ADDRESS 1
#else
#define OFFLINE_ASM_USE_BUILTIN_FRAME_ADDRESS 0
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This addition makes LLIntAssembly.h bloat significantly (since LLInt generates each combination).

Source/JavaScriptCore/llint/WebAssembly.asm
@@ -1159,7 +1158,7 @@ end
loadp (constexpr (JSWebAssemblyInstance::offsetOfVM()))[wasmInstance], a0
copyCalleeSavesToVMEntryFrameCalleeSavesBuffer(a0, a1)

if ASSERT_ENABLED
if (not USE_BUILTIN_FRAME_ADDRESS) or ASSERT_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should just make ARMv7 work with BUILTIN_FRAME_ADDRESS. It is the last JIT architecture which is not supporting this BUILTIN_FRAME_ADDRES.

Copy link
Member

Choose a reason for hiding this comment

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

In IPInt side, I think we should just put this frame without ASSERT_ENABLED. We would like to support arbitrary arch in IPInt, and we may support things without USE_BUILTIN_FRAME_ADDRESS. But anyway, I think definitely all JIT arch should support USE_BUILTIN_FRAME_ADDRESS

Copy link
Member

Choose a reason for hiding this comment

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

But for this particular case, I think we should just enable USE_BUILTIN_FRAME_ADDRESS for ARMv7. That's the simplest solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. However it looks like we get broken code when doing a Release build with GCC on ARMv7. I can see it doing

sub     sp, #180        @ 0xb4
add     r7, sp, #24

and then dereferencing that to get the caller frame address. clang correctly sets up r7 before allocating stack space. Looking for a workaround.

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 see any way to work around this, I think this needs an upstream GCC fix. How about we gate this on ARMv7 in WebAssembly.asm for now?

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.e. to avoid the blow up in LLIntAssembly.h.

@justinmichaud
Copy link
Contributor

I agree with @aoikonomopoulos; I think the best path forward here is 1) a patch for GCC, and 2) to use ARMv7 as the condition for now, to avoid blowing up the header. Then, we can bump up the minimum supported GCC version and remove this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Constellation Constellation Constellation left review comments

Assignees

@aoikonomopoulos aoikonomopoulos

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aoikonomopoulos @webkit-early-warning-system @justinmichaud @Constellation @webkit-ews-buildbot