| Summary: | [GTK][JSCOnly] Enable WebAssembly on Linux environment | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | achristensen, annulen, buildbot, cgarcia, clopez, fpizlo, ggaren, jfbastien, keith_miller, mcatanzaro, ossy, rniwa, saam | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 162693 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Yusuke Suzuki
2016-10-26 14:09:39 PDT
note: https://trac.webkit.org/changeset/207913 should be reverted once we enable wasm on non Darwin platforms. Can we enable it in Linux now? After https://bugs.webkit.org/show_bug.cgi?id=162693 is landed, I'll start enabling it on at least JSCOnly port by implementing some platform-specific part. (In reply to comment #3) > After https://bugs.webkit.org/show_bug.cgi?id=162693 is landed, > I'll start enabling it on at least JSCOnly port by implementing some > platform-specific part. OK, but please don't forget that FTL isn't supported at all on ARMv7 and x86. And it isn't enabled yet on AArch64 Linux. Created attachment 303446 [details]
Patch
Created attachment 303447 [details]
Patch
Comment on attachment 303447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303447&action=review cmake related changes looks good to me. But unfortunately I don't have any time to check the other part of the patch. > Tools/Scripts/webkitperl/FeatureList.pm:405 > + define => "ENABLE_WEBASSEMBLY", default => (isX86_64() && (isGtk() || isEfl() || isJSCOnly())) , value => \$webAssemblySupport }, EFL port was gone, no need for isEfl() here. > ChangeLog:24 > + duplicated changelog entry Comment on attachment 303447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303447&action=review >> Tools/Scripts/webkitperl/FeatureList.pm:405 >> + define => "ENABLE_WEBASSEMBLY", default => (isX86_64() && (isGtk() || isEfl() || isJSCOnly())) , value => \$webAssemblySupport }, > > EFL port was gone, no need for isEfl() here. Dropped. >> ChangeLog:24 >> + > > duplicated changelog entry Oops, fixed. Created attachment 303448 [details]
Patch
Comment on attachment 303448 [details] Patch Attachment 303448 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3245894 New failing tests: media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html Created attachment 303453 [details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to comment #10) > Comment on attachment 303448 [details] > Patch > > Attachment 303448 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.webkit.org/results/3245894 > > New failing tests: > media/modern-media-controls/tracks-support/tracks-support-show-panel-after- > dragging-controls.html I don't think it is related. Comment on attachment 303448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303448&action=review > Source/cmake/OptionsGTK.cmake:164 > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PRIVATE ${ENABLE_FTL_DEFAULT}) > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBASSEMBLY PRIVATE ${ENABLE_FTL_DEFAULT}) Let's move these from OptionsGTK.cmake/OptionsJSCOnly.cmake to WebKitFeatures.cmake instead. > Source/cmake/OptionsJSCOnly.cmake:9 > +if (WTF_CPU_X86_64) > + set(ENABLE_FTL_DEFAULT ON) > +else () > + set(ENABLE_FTL_DEFAULT OFF) > +endif () Ditto. > Source/cmake/OptionsJSCOnly.cmake:19 > -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ON) > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${ENABLE_FTL_DEFAULT}) > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBASSEMBLY PUBLIC ${ENABLE_FTL_DEFAULT}) Ditto. > Source/cmake/OptionsMac.cmake:111 > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PRIVATE ON) > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBASSEMBLY PRIVATE ON) Then these can be removed too. > Tools/Scripts/webkitperl/FeatureList.pm:228 > { option => "ftl-jit", desc => "Toggle FTLJIT support", > - define => "ENABLE_FTL_JIT", default => (isX86_64() && (isGtk() || isEfl())) , value => \$ftlJITSupport }, > + define => "ENABLE_FTL_JIT", default => (isX86_64() && (isGtk() || isJSCOnly())) , value => \$ftlJITSupport }, I would say this should be 1 (enabled always), but... it is not currently enabled on Mac? What...? > Tools/Scripts/webkitperl/FeatureList.pm:405 > { option => "webassembly", desc => "Toggle WebAssembly support", > - define => "ENABLE_WEBASSEMBLY", default => 0, value => \$webAssemblySupport }, > + define => "ENABLE_WEBASSEMBLY", default => (isX86_64() && (isGtk() || isJSCOnly())) , value => \$webAssemblySupport }, Ditto. I thought these were both enabled on Mac already. Comment on attachment 303448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303448&action=review > Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:165 > + ret = sigaction(SIGSEGV, &action, &oldSigSegvHandler); What determines SIGBUS vs SIGSEGV on Linux? I experienced similar errors leading to SIGSEGV or SIGBUS on Linux dependiding on CPU arch (In reply to comment #13) > Comment on attachment 303448 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303448&action=review ... > > Tools/Scripts/webkitperl/FeatureList.pm:228 > > { option => "ftl-jit", desc => "Toggle FTLJIT support", > > - define => "ENABLE_FTL_JIT", default => (isX86_64() && (isGtk() || isEfl())) , value => \$ftlJITSupport }, > > + define => "ENABLE_FTL_JIT", default => (isX86_64() && (isGtk() || isJSCOnly())) , value => \$ftlJITSupport }, > > I would say this should be 1 (enabled always), but... it is not currently > enabled on Mac? What...? It is already enabled on Mac. But with build-webkit script, you can't disable a feature which is enabled in xcodeproj, you can only enable a disabled feature. :) > > Tools/Scripts/webkitperl/FeatureList.pm:405 > > { option => "webassembly", desc => "Toggle WebAssembly support", > > - define => "ENABLE_WEBASSEMBLY", default => 0, value => \$webAssemblySupport }, > > + define => "ENABLE_WEBASSEMBLY", default => (isX86_64() && (isGtk() || isJSCOnly())) , value => \$webAssemblySupport }, > > Ditto. I thought these were both enabled on Mac already. Ditto. (In reply to comment #14) > What determines SIGBUS vs SIGSEGV on Linux? Looks like SIGBUS is mainly caused by alignment errors: http://cquestion.blogspot.com/2008/03/sigbus-vs-sigsegv.html Alignment errors in userspace are usually fixed by kernel (In reply to comment #18) > Alignment errors in userspace are usually fixed by kernel SIGBUS v.s. SIGSEGV depends on the platforms. Basically, in Linux, SIGBUS happens if we perform illegally aligned memory access (so, it rarely happens in x86). Or, file mmap access to the region that is not corresponding to the actual file region. But according to some document, this raises SIGSEGV in SPARC Linux. (I'm not sure b/c I don't have SPARC machine.) The situation is different in Darwin. Darwin raises SIGBUS when we perform accesses to the incorrect memory region. So, I think installing the both handlers is the safe way for Linux. (In reply to comment #19) > (In reply to comment #18) > > Alignment errors in userspace are usually fixed by kernel > > SIGBUS v.s. SIGSEGV depends on the platforms. > Basically, in Linux, SIGBUS happens if we perform illegally aligned memory > access (so, it rarely happens in x86). > Or, file mmap access to the region that is not corresponding to the actual > file region. > But according to some document, this raises SIGSEGV in SPARC Linux. (I'm not > sure b/c I don't have SPARC machine.) > The situation is different in Darwin. Darwin raises SIGBUS when we perform > accesses to the incorrect memory region. > > So, I think installing the both handlers is the safe way for Linux. I agree! I remember doing these tricks on Linux. It's indeed somewhat random whether you get SIGBUS or SIGSEGV. I believe that registering handlers for both is the right way to be sure that it all works. Likely anyone else doing these tricks would also be registering both, so it's not like this adds a lot of risk. And if they did only register one, then it's almost certainly also the one we would have registered if we only registered one. So, I don't see the downside of this approach. Comment on attachment 303448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303448&action=review Lgtm, but I want to leave it to a Linux reviewer to give it to r+. >> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:165 >> + ret = sigaction(SIGSEGV, &action, &oldSigSegvHandler); > > What determines SIGBUS vs SIGSEGV on Linux? I would be ok with us registering both handlers on all POSIX platforms. Created attachment 303492 [details]
Patch
(In reply to comment #21) > Comment on attachment 303448 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303448&action=review > > Lgtm, but I want to leave it to a Linux reviewer to give it to r+. > > >> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:165 > >> + ret = sigaction(SIGSEGV, &action, &oldSigSegvHandler); > > > > What determines SIGBUS vs SIGSEGV on Linux? > > I would be ok with us registering both handlers on all POSIX platforms. OK, I'll do that :) Created attachment 303493 [details]
Patch
Comment on attachment 303448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303448&action=review >> Source/cmake/OptionsGTK.cmake:164 >> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBASSEMBLY PRIVATE ${ENABLE_FTL_DEFAULT}) > > Let's move these from OptionsGTK.cmake/OptionsJSCOnly.cmake to WebKitFeatures.cmake instead. Moved. >> Source/cmake/OptionsJSCOnly.cmake:9 >> +endif () > > Ditto. Moved. >> Source/cmake/OptionsJSCOnly.cmake:19 >> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBASSEMBLY PUBLIC ${ENABLE_FTL_DEFAULT}) > > Ditto. Moved. >> Source/cmake/OptionsMac.cmake:111 >> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBASSEMBLY PRIVATE ON) > > Then these can be removed too. OK, removed. >>> Tools/Scripts/webkitperl/FeatureList.pm:228 >>> + define => "ENABLE_FTL_JIT", default => (isX86_64() && (isGtk() || isJSCOnly())) , value => \$ftlJITSupport }, >> >> I would say this should be 1 (enabled always), but... it is not currently enabled on Mac? What...? > > It is already enabled on Mac. But with build-webkit script, > you can't disable a feature which is enabled in xcodeproj, > you can only enable a disabled feature. :) OK, in this patch, I leave it as is. >>> Tools/Scripts/webkitperl/FeatureList.pm:405 >>> + define => "ENABLE_WEBASSEMBLY", default => (isX86_64() && (isGtk() || isJSCOnly())) , value => \$webAssemblySupport }, >> >> Ditto. I thought these were both enabled on Mac already. > > Ditto. Ditto. I guess my main concern is: why on Earth are we trying to handle SIGSEGV/SIGBUS? Haven't we already hit undefined behavior by that point? Is the goal is to do something graceful when the user's web assembly is broken? What happens to normal WebKit crashes...? (In reply to comment #26) > I guess my main concern is: why on Earth are we trying to handle > SIGSEGV/SIGBUS? Haven't we already hit undefined behavior by that point? Is > the goal is to do something graceful when the user's web assembly is broken? > What happens to normal WebKit crashes...? All WebAssembly memory accesses are guarded. The behavior we want is a bounds check on each access. Surely we don't want the browser engine to crash when they perform an out of bounds memory access. Catching SIGSEGV/SIGBUS allows us to perform an optimization where we mmap enough memory for the largest possible Wasm load/store, and we catch SIGBUS/SIGSEGV to any out of bounds accesses. Note that these are still accesses to memory we've requested from the OS. It'll just be PROT_NONE memory. (In reply to comment #26) > I guess my main concern is: why on Earth are we trying to handle > SIGSEGV/SIGBUS? Haven't we already hit undefined behavior by that point? Is > the goal is to do something graceful when the user's web assembly is broken? > What happens to normal WebKit crashes...? This is necessary for Wasm faster load. I'll explain the background of that. Wasm can dereference the pointers. But if the pointer points invalid memory region, Wasm runtime should throw WasmError(OutOfBoundsMemoryAccess). To impement this semantics, previously, we leveraged the bound check: when loading the value from the pointer, we perform bound checks to validate that the given pointer is in range of the valid Wasm Memory region. It is not efficient even if we can drop some part of duplicate checks in optimization phases. So, we recently introduce faster load. In that optimization, we leverage the fact that Wasm pointer is 32bit. We first reserve 32bit region by using mmap and make some part of region usable. This is OK because we have bunch of virtual memory space in 64bit environment! If our wasm memory access is always performed with some offset address that indicates the start of that memory region, we can ensure that all the wasm access can be within this 32bit region. So, if Wasm load is done onto the incorrect memory region, it causes SIGBUS / SIGSEGV since we mark these area as non readable / non writable. In that case, the signal handler is invoked. In this handler, we need to check that the given memory access is done in the Wasm code to distinguish from the usual SIGBUS/SIGSEGV bugs. If we find that load is done in the Wasm code, we change the return address of the signal handler, and this trampoline will throw WasmError(OutOfBoundsMemoryAccess). In this way, we can drop bound checks and still handle invalid memory accesses. Comment on attachment 303493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303493&action=review > Source/cmake/WebKitFeatures.cmake:69 > + if (WTF_CPU_X86_64) I think it should be if (WTF_CPU_X86_64 OR WTF_CPU_ARM64), B3 works fine on AArch64 for a long time Comment on attachment 303493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303493&action=review >> Source/cmake/WebKitFeatures.cmake:69 >> + if (WTF_CPU_X86_64) > > I think it should be if (WTF_CPU_X86_64 OR WTF_CPU_ARM64), B3 works fine on AArch64 for a long time OK, let's change it. (In reply to comment #4) > (In reply to comment #3) > > After https://bugs.webkit.org/show_bug.cgi?id=162693 is landed, > > I'll start enabling it on at least JSCOnly port by implementing some > > platform-specific part. > > OK, but please don't forget that FTL isn't supported at all > on ARMv7 and x86. And it isn't enabled yet on AArch64 Linux. Hm, it seems that AArch64 FTL is not enabled right now. So, I think leaving it as is is better. If we decide to enable AArch64 FTL & Wasm, I think we should do that in a separate bug. (In reply to comment #28) > (In reply to comment #26) > > I guess my main concern is: why on Earth are we trying to handle > > SIGSEGV/SIGBUS? Haven't we already hit undefined behavior by that point? Is > > the goal is to do something graceful when the user's web assembly is broken? > > What happens to normal WebKit crashes...? > > This is necessary for Wasm faster load. I'll explain the background of that. > > Wasm can dereference the pointers. > But if the pointer points invalid memory region, Wasm runtime should throw > WasmError(OutOfBoundsMemoryAccess). > > To impement this semantics, previously, we leveraged the bound check: when > loading the value from the pointer, we perform bound checks to validate that > the given pointer is in range of the valid Wasm Memory region. > It is not efficient even if we can drop some part of duplicate checks in > optimization phases. > > So, we recently introduce faster load. In that optimization, we leverage the > fact that Wasm pointer is 32bit. > We first reserve 32bit region by using mmap and make some part of region > usable. > This is OK because we have bunch of virtual memory space in 64bit > environment! > If our wasm memory access is always performed with some offset address that > indicates the start of that memory region, we can ensure that all the wasm > access can be within this 32bit region. > > So, if Wasm load is done onto the incorrect memory region, it causes SIGBUS > / SIGSEGV since we mark these area as non readable / non writable. > In that case, the signal handler is invoked. > In this handler, we need to check that the given memory access is done in > the Wasm code to distinguish from the usual SIGBUS/SIGSEGV bugs. > If we find that load is done in the Wasm code, we change the return address > of the signal handler, and this trampoline will throw > WasmError(OutOfBoundsMemoryAccess). > > In this way, we can drop bound checks and still handle invalid memory > accesses. Note that usual SIGSEGV/SIGBUS bugs are handled as usual. If the handler finds that this signal occurs outside of Wasm, the handler resets sigaction configuration to the default one and return. So, then, the same memory operation is done and the same SIGSEGV/SIGBUS will occur. And they should be handled in a default manner. And we also note that this Wasm faster load is enabled only if we call JSC::Wasm::enableFastMemory() is called. If this function is not called, Wasm keeps using bound checks. Since this JSC::Wasm::enableFastMemory() function is only called in the managed process (Like, I think, Safari WebProcess / Epiphany WebProcess. It should not be called inside WebKitGTK+ library), installing signal handlers is OK: it means that the user of JSC::Wasm::enableFastMemory() should know that it installs signal handlers. These Safari WebProcess / Epiphany WebProcess should take care that SIGSEGV / SIGBUS handlers are already used. Comment on attachment 303493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303493&action=review OK, thanks for the explanation. Let's try it. > Source/JavaScriptCore/ChangeLog:3 > + [GTK][EFL][JSCOnly] Enable WebAssembly on Linux environment You'll need to remove EFL from the title here. > Source/cmake/WebKitFeatures.cmake:73 > + if (WTF_CPU_X86_64) > + set(ENABLE_FTL_DEFAULT ON) > + else () > + set(ENABLE_FTL_DEFAULT OFF) > + endif () Can you simplify this to set(ENABLE_FTL_DEFAULT ${WTF_CPU_X86_64}), or does that not work? > ChangeLog:3 > + [GTK][EFL][JSCOnly] Enable WebAssembly on Linux environment Ditto. I'd be impressed if there are no race conditions caused by the signal handler, but you're not adding it in this patch, and I haven't reviewed it. And I don't want to. Signal handlers hurt my head! Comment on attachment 303493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303493&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:3 >> + [GTK][EFL][JSCOnly] Enable WebAssembly on Linux environment > > You'll need to remove EFL from the title here. OK, I'll drop EFL. >> Source/cmake/WebKitFeatures.cmake:73 >> + endif () > > Can you simplify this to set(ENABLE_FTL_DEFAULT ${WTF_CPU_X86_64}), or does that not work? I'll try it. >> ChangeLog:3 >> + [GTK][EFL][JSCOnly] Enable WebAssembly on Linux environment > > Ditto. Fixed. Committed r213450: <http://trac.webkit.org/changeset/213450> Committed r213451: <http://trac.webkit.org/changeset/213451> I think this had the accidental side effect of enabling FTL on 64-bit Windows. We should revert that. This was fixed in https://trac.webkit.org/changeset/213707 |