Bug 165546 - WebAssembly: perform stack checks
Summary: WebAssembly: perform stack checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 165118 172188
Blocks: 161709 165856
  Show dependency treegraph
 
Reported: 2016-12-07 13:44 PST by JF Bastien
Modified: 2017-05-18 12:38 PDT (History)
14 users (show)

See Also:


Attachments
WIP (16.11 KB, patch)
2017-05-15 19:02 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (23.73 KB, patch)
2017-05-15 19:50 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (23.72 KB, patch)
2017-05-15 19:51 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (19.49 KB, patch)
2017-05-15 20:19 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (29.33 KB, patch)
2017-05-16 17:58 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.01 MB, application/zip)
2017-05-17 04:11 PDT, Build Bot
no flags Details
patch (50.64 KB, patch)
2017-05-17 19:57 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (50.91 KB, patch)
2017-05-17 20:00 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (51.00 KB, patch)
2017-05-17 20:01 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
patch for landing (60.67 KB, patch)
2017-05-18 12:35 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-12-07 13:44:46 PST
In WasmBindings.cpp.
Comment 1 JF Bastien 2016-12-07 13:45:12 PST
We need to test that stack exhaustion can be caught as well.
Comment 2 Radar WebKit Bug Importer 2016-12-20 14:28:23 PST
<rdar://problem/29760307>
Comment 3 JF Bastien 2017-02-23 14:17:53 PST
It looks like wasmCallingConvention::setupFrameInPrologue also needs a stack check. We could use vm().addressOfSoftStackLimit() but then that prevents us from making the code PIC as in bug #168264 (making it patchable means workers get their own code copy).
Comment 4 JF Bastien 2017-02-23 14:19:33 PST
I think we'll need to write a fuzz test for this, because we want to test exhaustion for different types of frames, and a hard-coded test won't be able to hit all possible locations reliably. We should randomly generate functions which will have different stack size, and probabilistically the tests will hit different check points. A trap would mean we've missed one.
Comment 5 JF Bastien 2017-02-23 14:22:57 PST
A TLS load for every function is not cool though. Maybe we can have some clever stack limit bit pattern?
Comment 6 JF Bastien 2017-02-23 15:48:15 PST
Or not, Fit says TLS is cheap.
Comment 7 Saam Barati 2017-05-15 15:37:14 PDT
Working on this now
Comment 8 Saam Barati 2017-05-15 19:02:51 PDT
Created attachment 310205 [details]
WIP
Comment 9 Saam Barati 2017-05-15 19:50:59 PDT
Created attachment 310210 [details]
WIP

Still need to do stack checks for the JS call ICs. I'm thinking that I'll just make the wasm caller of the JS call IC stub know if it makes any JS calls, and if it does, it can calculate the maximum number of arguments for all JS calls inside of it, and then we can incorporate that into the stack check. This will prevent each JS call stub from having to do its own stack check
Comment 10 Saam Barati 2017-05-15 19:51:38 PDT
Created attachment 310211 [details]
WIP
Comment 11 Saam Barati 2017-05-15 20:19:07 PDT
Created attachment 310214 [details]
WIP

close to being done. I need to write some tests.
Comment 12 Build Bot 2017-05-15 20:21:43 PDT
Attachment 310214 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:427:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Saam Barati 2017-05-16 17:58:45 PDT
Created attachment 310335 [details]
WIP

Waiting on:
https://bugs.webkit.org/show_bug.cgi?id=172188
Comment 14 Build Bot 2017-05-16 18:00:30 PDT
Attachment 310335 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:634:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:637:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:430:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Build Bot 2017-05-16 18:40:40 PDT
Comment on attachment 310335 [details]
WIP

Attachment 310335 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3755013

New failing tests:
wasm.yaml/wasm/function-tests/factorial.js.wasm-eager-jettison
wasm.yaml/wasm/function-tests/factorial.js.default-wasm
wasm.yaml/wasm/function-tests/factorial.js.wasm-no-cjit
wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-cjit
wasm.yaml/wasm/function-tests/float-sub.js.wasm-no-call-ic
wasm.yaml/wasm/js-api/wasm-to-wasm.js.wasm-no-cjit
wasm.yaml/wasm/js-api/wasm-to-wasm.js.default-wasm
wasm.yaml/wasm/js-api/wasm-to-wasm.js.wasm-no-call-ic
wasm.yaml/wasm/js-api/wasm-to-wasm.js.wasm-eager-jettison
wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager-jettison
wasm.yaml/wasm/function-tests/float-sub.js.wasm-eager-jettison
wasm.yaml/wasm/function-tests/stack-overflow.js.default-wasm
wasm.yaml/wasm/function-tests/factorial.js.wasm-no-call-ic
wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-call-ic
wasm.yaml/wasm/function-tests/float-sub.js.wasm-no-cjit
wasm.yaml/wasm/function-tests/float-sub.js.default-wasm
Comment 16 Build Bot 2017-05-17 04:11:08 PDT
Comment on attachment 310335 [details]
WIP

Attachment 310335 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3761709

New failing tests:
imported/w3c/web-platform-tests/css/selectors4/focus-display-none-001.html
imported/w3c/web-platform-tests/css/selectors4/focus-within-display-none-001.html
Comment 17 Build Bot 2017-05-17 04:11:09 PDT
Created attachment 310378 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Saam Barati 2017-05-17 19:57:00 PDT
Created attachment 310474 [details]
patch
Comment 19 Build Bot 2017-05-17 19:58:14 PDT
Attachment 310474 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:431:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Saam Barati 2017-05-17 20:00:33 PDT
Created attachment 310475 [details]
patch
Comment 21 Saam Barati 2017-05-17 20:01:36 PDT
Created attachment 310477 [details]
patch
Comment 22 Build Bot 2017-05-17 20:03:19 PDT
Attachment 310477 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:431:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 JF Bastien 2017-05-17 20:28:52 PDT
Comment on attachment 310477 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review

Some features call C++ code, like grow_memory. Should these also be marked as m_makesCalls?

> JSTests/wasm/function-tests/factorial.js:36
> +assert.throws(() => fac(10000000000000), RangeError, "Maximum call stack size exceeded.");

10,000,000,000,000 way exceeds i32. Can you use 1e9 or something smaller?

> JSTests/wasm/function-tests/stack-overflow.js:39
> +    // FIXME: asssert some stack size

Bug #

> JSTests/wasm/function-tests/stack-overflow.js:216
> +    test(42);

So there are 3 places we want to check for stack overflow:

* Same instance wasm -> wasm direct call.
* wasm -> wasm indirect call (same or different instance)
* Call of any import (JS or wasm) where the stub is the thing that overflows

That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though!

> Source/JavaScriptCore/ChangeLog:11
> +        runtime option to configure this.

TLS or wasm context (which can sometimes be TLS)?

> Source/JavaScriptCore/ChangeLog:25
> +        Surprisingly, this patch was neutral on WasmBench and TitzerBench.

🎉

> Source/JavaScriptCore/runtime/Options.h:459
> +    v(bool, useFastTLSForWasmStackChecks, true, Normal, "If true (and fast TLS is enabled), we will store stack bounds in fast TLS. If false, we will store it on the context.") \

Yeah I thought it would always be a slot on the wasm context instead. Is there a perf difference for either being false?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:411
> +                // 2. Try to speed things up by skipping stack checks.

Does this work for leaf JS -> wasm?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:415
> +                // stack that such a stub would use.

Nice!

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:419
> +            RELEASE_ASSERT(checkSize >= 0);

Signed overflow is UB, so this check is technically dead. Should be a checked add instead (and the unsigned -> signed coercion could also fail... Not UB, impl-def instead).
Comment 24 Keith Miller 2017-05-17 20:35:46 PDT
Comment on attachment 310477 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review

>> Source/JavaScriptCore/ChangeLog:11
>> +        This patch adds stack checks to wasm. It implements it by storing the stack
>> +        bounds on the Context or in TLS. By default, we use TLS. However, there is a
>> +        runtime option to configure this.
> 
> TLS or wasm context (which can sometimes be TLS)?

Is there an observable difference in performance between these? If not, I'm not sure it's worth the use of our few fast TLS slots.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:148
> +    wasmContext->setCachedStackLimit(bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max()));

Why is this correct? What happens if prevWasmContext == wasmContext? Wouldn't this blow away the previous cached stack bounds?

If so, can you add a test?
Comment 25 Saam Barati 2017-05-17 22:22:06 PDT
Comment on attachment 310477 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:148
>> +    wasmContext->setCachedStackLimit(bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max()));
> 
> Why is this correct? What happens if prevWasmContext == wasmContext? Wouldn't this blow away the previous cached stack bounds?
> 
> If so, can you add a test?

This is definitely not correct for the reason you outlined. I added it while testing just to ensure that removing some checks did the right thing. I’ll remove this and add a check that would have made this eagerly stack overflow
Comment 26 Saam Barati 2017-05-17 23:28:12 PDT
Comment on attachment 310477 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review

>>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:148
>>> +    wasmContext->setCachedStackLimit(bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max()));
>> 
>> Why is this correct? What happens if prevWasmContext == wasmContext? Wouldn't this blow away the previous cached stack bounds?
>> 
>> If so, can you add a test?
> 
> This is definitely not correct for the reason you outlined. I added it while testing just to ensure that removing some checks did the right thing. I’ll remove this and add a check that would have made this eagerly stack overflow

Oh sorry this is correct because it happens before the storeContext call below it! That said, this is quite tricky and perhaps I should remove it.
My thinking when adding this is it’ll make every stack check fail, so if we ever forget to update the bounds, this buys us some safety.
Comment 27 Saam Barati 2017-05-18 01:28:36 PDT
Comment on attachment 310477 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1062
> +        m_maxNumJSCallArguments = std::max(m_maxNumJSCallArguments, static_cast<uint32_t>(args.size()));

I need to do this for callIndirect as well. I at first convinced myself that this wasn't needed since callIndirect is doing a wasm call, and its stack bound is part of our frame size, however, we might be calling a WebAssemblyWrapperFunction, which is as if we called JS, so we double the stack size. So I need to also do this same math for callIndirect arguments.
Comment 28 Saam Barati 2017-05-18 01:42:10 PDT
Comment on attachment 310477 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review

>> JSTests/wasm/function-tests/factorial.js:36
>> +assert.throws(() => fac(10000000000000), RangeError, "Maximum call stack size exceeded.");
> 
> 10,000,000,000,000 way exceeds i32. Can you use 1e9 or something smaller?

Sure

>> JSTests/wasm/function-tests/stack-overflow.js:39
>> +    // FIXME: asssert some stack size
> 
> Bug #

This should be removed, I added the code to check for some base stack size.

>> JSTests/wasm/function-tests/stack-overflow.js:216
>> +    test(42);
> 
> So there are 3 places we want to check for stack overflow:
> 
> * Same instance wasm -> wasm direct call.
> * wasm -> wasm indirect call (same or different instance)
> * Call of any import (JS or wasm) where the stub is the thing that overflows
> 
> That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though!

My patch tests these. Note that a stub does not do the bounds check, it's the job of anything calling the stub to take into account the stack space the stub will use. You can see the math to do that with the m_maxNumJSCallArguments field.

>>> Source/JavaScriptCore/ChangeLog:11
>>> +        runtime option to configure this.
>> 
>> TLS or wasm context (which can sometimes be TLS)?
> 
> Is there an observable difference in performance between these? If not, I'm not sure it's worth the use of our few fast TLS slots.

I'll re-run this and get back to y'all.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:411
>> +                // 2. Try to speed things up by skipping stack checks.
> 
> Does this work for leaf JS -> wasm?

leaf wasm functions never check their stack bounds unless their frame size is larger than ~1024 bytes. So this would be the same here. Note that the entrance to Wasm does a stack check for the frame size needed by the JS entry frame and the first Wasm frame. Because of that, leafs are safe to not have stack checks when called directly from JS since the C++ checks roughly where the stack would be (and it's allowed to be approximate because of our red zone).

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:419
>> +            RELEASE_ASSERT(checkSize >= 0);
> 
> Signed overflow is UB, so this check is technically dead. Should be a checked add instead (and the unsigned -> signed coercion could also fail... Not UB, impl-def instead).

I'll move to checked.
Comment 29 JF Bastien 2017-05-18 10:03:27 PDT
> >> JSTests/wasm/function-tests/stack-overflow.js:216
> >> +    test(42);
> > 
> > So there are 3 places we want to check for stack overflow:
> > 
> > * Same instance wasm -> wasm direct call.
> > * wasm -> wasm indirect call (same or different instance)
> > * Call of any import (JS or wasm) where the stub is the thing that overflows
> > 
> > That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though!
> 
> My patch tests these. Note that a stub does not do the bounds check, it's
> the job of anything calling the stub to take into account the stack space
> the stub will use. You can see the math to do that with the
> m_maxNumJSCallArguments field.

That's testing what your current implementation does, assuming no bugs. When possible I'd rather test what any implementation could do, assuming we could have bugs. Granted it's a tricky one here, but I wouldn't just assume that the stubs never need stack space.

So I'm OK with what you have now, but if you think it's not too difficult I'd rather have a more thorough test, either here or as a follow-up.
Comment 30 Saam Barati 2017-05-18 10:26:58 PDT
(In reply to JF Bastien from comment #29)
> > >> JSTests/wasm/function-tests/stack-overflow.js:216
> > >> +    test(42);
> > > 
> > > So there are 3 places we want to check for stack overflow:
> > > 
> > > * Same instance wasm -> wasm direct call.
> > > * wasm -> wasm indirect call (same or different instance)
> > > * Call of any import (JS or wasm) where the stub is the thing that overflows
> > > 
> > > That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though!
> > 
> > My patch tests these. Note that a stub does not do the bounds check, it's
> > the job of anything calling the stub to take into account the stack space
> > the stub will use. You can see the math to do that with the
> > m_maxNumJSCallArguments field.
> 
> That's testing what your current implementation does, assuming no bugs. When
> possible I'd rather test what any implementation could do, assuming we could
> have bugs. Granted it's a tricky one here, but I wouldn't just assume that
> the stubs never need stack space.
My patch does not assume this, it assumes the opposite. It assumes it uses numArgs*8 stack space.

> 
> So I'm OK with what you have now, but if you think it's not too difficult
> I'd rather have a more thorough test, either here or as a follow-up.
Comment 31 JF Bastien 2017-05-18 10:41:06 PDT
(In reply to Saam Barati from comment #30)
> (In reply to JF Bastien from comment #29)
> > > >> JSTests/wasm/function-tests/stack-overflow.js:216
> > > >> +    test(42);
> > > > 
> > > > So there are 3 places we want to check for stack overflow:
> > > > 
> > > > * Same instance wasm -> wasm direct call.
> > > > * wasm -> wasm indirect call (same or different instance)
> > > > * Call of any import (JS or wasm) where the stub is the thing that overflows
> > > > 
> > > > That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though!
> > > 
> > > My patch tests these. Note that a stub does not do the bounds check, it's
> > > the job of anything calling the stub to take into account the stack space
> > > the stub will use. You can see the math to do that with the
> > > m_maxNumJSCallArguments field.
> > 
> > That's testing what your current implementation does, assuming no bugs. When
> > possible I'd rather test what any implementation could do, assuming we could
> > have bugs. Granted it's a tricky one here, but I wouldn't just assume that
> > the stubs never need stack space.
> My patch does not assume this, it assumes the opposite. It assumes it uses
> numArgs*8 stack space.

This test does? I misunderstood then.
Comment 32 Saam Barati 2017-05-18 12:35:00 PDT
Created attachment 310533 [details]
patch for landing
Comment 33 Saam Barati 2017-05-18 12:38:31 PDT
landed in:
https://trac.webkit.org/changeset/217060/webkit