Bug 164032 – [GTK][JSCOnly] Enable WebAssembly on Linux environment

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Once it is completed!

[Csaba Osztrogonác]

note: https://trac.webkit.org/changeset/207913 should be reverted once we enable wasm on non Darwin platforms.

[Yusuke Suzuki]

Can we enable it in Linux now?

[Yusuke Suzuki]

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.

[Csaba Osztrogonác]

(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.

[Yusuke Suzuki]

Created attachment 303446 [details]
Patch

[Yusuke Suzuki]

Created attachment 303447 [details]
Patch

[Csaba Osztrogonác]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 303448 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Yusuke Suzuki]

(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.

[Michael Catanzaro]

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.

[Saam Barati]

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?

[Konstantin Tokarev]

I experienced similar errors leading to SIGSEGV or SIGBUS on Linux dependiding on CPU arch

[Csaba Osztrogonác]

(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.

[Michael Catanzaro]

(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

[Konstantin Tokarev]

Alignment errors in userspace are usually fixed by kernel

[Yusuke Suzuki]

(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.

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Yusuke Suzuki]

Created attachment 303492 [details]
Patch

[Yusuke Suzuki]

(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 :)

[Yusuke Suzuki]

Created attachment 303493 [details]
Patch

[Yusuke Suzuki]

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.

[Michael Catanzaro]

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...?

[Saam Barati]

(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.

[Yusuke Suzuki]

(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.

[Konstantin Tokarev]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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!

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r213450: <http://trac.webkit.org/changeset/213450>

[Yusuke Suzuki]

Committed r213451: <http://trac.webkit.org/changeset/213451>

[Alex Christensen]

I think this had the accidental side effect of enabling FTL on 64-bit Windows.  We should revert that.

[Alex Christensen]

This was fixed in https://trac.webkit.org/changeset/213707