Bug 171537 - WebAssembly: running out of executable memory should throw OoM
Summary: WebAssembly: running out of executable memory should throw OoM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
: 171454 (view as bug list)
Depends on:
Blocks: 159775 173908
  Show dependency treegraph
 
Reported: 2017-05-01 23:05 PDT by JF Bastien
Modified: 2017-06-27 23:47 PDT (History)
9 users (show)

See Also:


Attachments
patch (14.96 KB, patch)
2017-06-23 23:12 PDT, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
patch (32.01 KB, patch)
2017-06-27 16:56 PDT, JF Bastien
sbarati: review+
Details | Formatted Diff | Diff
patch (33.74 KB, patch)
2017-06-27 23:02 PDT, JF Bastien
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 2017-05-01 23:05:28 PDT
We'll OoO when some parts of compile fails, it would be nice if X memory fail were in that category too. This is unique to wasm because we compile some code up-front and cannot function in the llint if X memory is unavailable, whereas JS does this incrementally and can fall back.
Comment 1 JF Bastien 2017-06-23 23:01:25 PDT
*** Bug 171454 has been marked as a duplicate of this bug. ***
Comment 2 JF Bastien 2017-06-23 23:01:59 PDT
While we're here, if We try to tier up a function but don't have X memory available for it then it should just stay at O1. Slow is better than dead. That's what we do for JS (stay in llint).
Comment 3 JF Bastien 2017-06-23 23:12:01 PDT
Created attachment 313772 [details]
patch

I'm wondering if this is worth testing, and how. I'm open to suggestions!

I've been running a huge program with: JSC_webAssemblyBBQOptimizationLevel=1 JSC_useConcurrentGC=0 JSC_useConcurrentJIT=0 JSC_jitMemoryReservationSize=5000 to hit its import limit, and then higher for BBQ functions. Maybe I should force llint by disabling DFG and FTL? Also, I'd like to make sure we don't leak code that's associated with the Module and which was compiled before the failure! Not all the failure points are tested, because that's kind of dynamic (wasm->wasm entry points are tricky to hit). Tiering is also tricky to hit OoM with.
Comment 4 Radar WebKit Bug Importer 2017-06-23 23:12:38 PDT
<rdar://problem/32963338>
Comment 5 Mark Lam 2017-06-24 10:31:36 PDT
Comment on attachment 313772 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:299
> -    if (m_state == State::Compiled) {
> +    if (!failed() && m_state == State::Compiled) {

Why not just return early if already failed?  The only possible reason for not returning early is if you want to move the state to State::Complete.  However, this disagrees with the allocation failure cases below where you return early without setting the state to State::Complete.  So, which is behaving wrongly here?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:65
> +                m_errorMessage = "Out of executable memory";

Use ASCIILiteral here:
    m_errorMessage = ASCIILiteral("Out of executable memory");

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:101
> +    bool runnable() { return !m_errorMessage; }

nit: I suggest:
    bool runnable() const { ...

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:109
> +        CString cString = m_errorMessage.ascii();
> +        return String(cString.data());

Why marshal thru CString?  Why not just return m_errorMessage?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:116
>          m_codeBlock.set(vm, this, codeBlock);
> +        if (UNLIKELY(!codeBlock->runnable())) {

Why do this check after setting m_codeBlock?  Why not do it before?
Comment 6 Saam Barati 2017-06-24 15:57:33 PDT
Comment on attachment 313772 [details]
patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63
> +            switch (binding.error()) {

Nit: I’m a fan of adding default: ASSERT_NOT_REACHED

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:105
> +    String errorMessage()

This comment is wrong. This is a JSCell. No races to worry about

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:116
>> +        if (UNLIKELY(!codeBlock->runnable())) {
> 
> Why do this check after setting m_codeBlock?  Why not do it before?

+1
Comment 7 Saam Barati 2017-06-24 16:59:04 PDT
(In reply to JF Bastien from comment #3)
> Created attachment 313772 [details]
> patch
> 
> I'm wondering if this is worth testing, and how. I'm open to suggestions!
Testing ARM64 shouldn’t be that hard. I’d just craft an artificial function that’s close to 1 MB in size. Compile in a loop and keep array of them to keep them all alive. 
> 
> I've been running a huge program with: JSC_webAssemblyBBQOptimizationLevel=1
> JSC_useConcurrentGC=0 JSC_useConcurrentJIT=0
> JSC_jitMemoryReservationSize=5000 to hit its import limit, and then higher
> for BBQ functions. Maybe I should force llint by disabling DFG and FTL?
> Also, I'd like to make sure we don't leak code that's associated with the
> Module and which was compiled before the failure! Not all the failure points
> are tested, because that's kind of dynamic (wasm->wasm entry points are
> tricky to hit). Tiering is also tricky to hit OoM with.
Comment 8 Saam Barati 2017-06-24 17:01:28 PDT
Comment on attachment 313772 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:93
> +        Base::fail(holdLock(m_lock), makeString("Out of executable memory while tiering up function at index ", String::number(m_functionIndex)));

What will happen once we fail here? Will we shut down the compilation and never try again? I think this would be pretty easy to test on ARM64
Comment 9 Saam Barati 2017-06-24 17:02:36 PDT
(In reply to Saam Barati from comment #8)
> Comment on attachment 313772 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313772&action=review
> 
> > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:93
> > +        Base::fail(holdLock(m_lock), makeString("Out of executable memory while tiering up function at index ", String::number(m_functionIndex)));
> 
> What will happen once we fail here? Will we shut down the compilation and
> never try again? I think this would be pretty easy to test on ARM64

Actually, there is an option to set the executable allocator pool size. You could probably write a test using that on x86 too. You just need to make sure that option actually does what it’s supposed to
Comment 10 JF Bastien 2017-06-27 16:56:03 PDT
Created attachment 313960 [details]
patch

(In reply to Mark Lam from comment #5)
> Comment on attachment 313772 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313772&action=review
> 
> > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:299
> > -    if (m_state == State::Compiled) {
> > +    if (!failed() && m_state == State::Compiled) {
> 
> Why not just return early if already failed?  The only possible reason for
> not returning early is if you want to move the state to State::Complete. 
> However, this disagrees with the allocation failure cases below where you
> return early without setting the state to State::Complete.  So, which is
> behaving wrongly here?

Because I also want to run completion tasks, as done below, which can call back into this function.


(In reply to Saam Barati from comment #6)
> Comment on attachment 313772 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313772&action=review
> 
> > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63
> > +            switch (binding.error()) {
> 
> Nit: I’m a fan of adding default: ASSERT_NOT_REACHED

That's a bad idea because right now it's a compile error if one of the cases isn't covered. With default it because a runtime assert.
Comment 11 Darin Adler 2017-06-27 17:40:55 PDT
Comment on attachment 313772 [details]
patch

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

>>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63
>>> +            switch (binding.error()) {
>> 
>> Nit: I’m a fan of adding default: ASSERT_NOT_REACHED
> 
> That's a bad idea because right now it's a compile error if one of the cases isn't covered. With default it because a runtime assert.

My preferred trick is to return at the end of the case rather than just breaking and then we can ASSERT_NOT_REACHED after the switch. That way we get both a compile error if a case isn’t covered and a runtime assert if an invalid value somehow gets into the enumeration.
Comment 12 Saam Barati 2017-06-27 17:59:11 PDT
Comment on attachment 313772 [details]
patch

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

>>>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63
>>>> +            switch (binding.error()) {
>>> 
>>> Nit: I’m a fan of adding default: ASSERT_NOT_REACHED
>> 
>> That's a bad idea because right now it's a compile error if one of the cases isn't covered. With default it because a runtime assert.
> 
> My preferred trick is to return at the end of the case rather than just breaking and then we can ASSERT_NOT_REACHED after the switch. That way we get both a compile error if a case isn’t covered and a runtime assert if an invalid value somehow gets into the enumeration.

This makes sense JF, I wasn't thinking about the compilation error. I'm also a fan of Darin's suggestion, and it'd be trivial to apply to this code.
Comment 13 Saam Barati 2017-06-27 18:05:42 PDT
Comment on attachment 313960 [details]
patch

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

r=me

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:320
> +    return exec->vm().canUseJIT() && Options::useBaselineJIT();

nice

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:179
> +        if (UNLIKELY(!binding)) {

Do your tests trigger this?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:310
> +        LinkBuffer linkBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail);
> +        if (UNLIKELY(linkBuffer.didFailToAllocate()))
> +            return makeUnexpected(BindingFailure::OutOfMemory);

Do your tests stress this?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:606
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail);
> +    if (UNLIKELY(patchBuffer.didFailToAllocate()))

ditto

> Source/JavaScriptCore/wasm/WasmBinding.cpp:664
> +    LinkBuffer patchBuffer(jit, GLOBAL_THUNK_ID, JITCompilationCanFail);
> +    if (UNLIKELY(patchBuffer.didFailToAllocate()))
> +        return makeUnexpected(BindingFailure::OutOfMemory);
> +

ditto

> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:95
> +    LinkBuffer linkBuffer(*context.wasmEntrypointJIT, nullptr, JITCompilationCanFail);
> +    if (UNLIKELY(linkBuffer.didFailToAllocate())) {
> +        Base::fail(holdLock(m_lock), makeString("Out of executable memory while tiering up function at index ", String::number(m_functionIndex)));
> +        return;
> +    }

Do any of your tests trigger this?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:62
> +        if (UNLIKELY(!binding)) {

Do your tests trigger this?

> Tools/Scripts/run-jsc-stress-tests:1261
> +    run("default-wasm", "--useConcurrentGC=0" , "--useConcurrentJIT=0", "--jitMemoryReservationSize=15000", "--useBaselineJIT=0", "--useDFGJIT=0", "--useFTLJIT=0", "-m")

I see a potential bug where we don't honor this option. It won't matter in this case since your region is smaller than what's used, but since you're relying on this option, you might as well fix it. There is a direct use of fixedExecutableMemoryPoolSize instead of asking the allocator its reservation size. Look in JITStubRoutine.h. It should be trivial to fix just by loading it from the allocator instead.
Comment 14 JF Bastien 2017-06-27 23:02:58 PDT
Created attachment 313995 [details]
patch

Address all comments.


On tests: yes, I believe I probabilistically hit all the checks. What I mean is that I disable all non-wasm JITting to raise the probability that wasm will hit a limit, then I remove concurrent things so it's still more likely. I then create functions of random size so that even if I hit one limit on one run it'll maybe hit another limit on another run (say, if our codegen changes). I skew the "imports" and "exports" test so they have more of those compared to regular wasm code. Finally, the execution test tries to cause tier up as well.

You can see this by running the tests with verbose = true in each file, and JSC_logExecutableAllocation=1 on the command-line.

Non-obvious, but locally it seems to do what I want!


On the potential bug: Saam and I chatted on IRC and he submitted https://bugs.webkit.org/show_bug.cgi?id=173906 to handle the issue.
Comment 15 JF Bastien 2017-06-27 23:04:50 PDT
> On tests: yes, I believe I probabilistically hit all the checks. What I mean
> is that I disable all non-wasm JITting to raise the probability that wasm
> will hit a limit, then I remove concurrent things so it's still more likely.
> I then create functions of random size so that even if I hit one limit on
> one run it'll maybe hit another limit on another run (say, if our codegen
> changes). I skew the "imports" and "exports" test so they have more of those
> compared to regular wasm code. Finally, the execution test tries to cause
> tier up as well.
> 
> You can see this by running the tests with verbose = true in each file, and
> JSC_logExecutableAllocation=1 on the command-line.
> 
> Non-obvious, but locally it seems to do what I want!

Clarifying "probability": by that I mean that I loop until *something* hits the executable limit. What that thing is I don't check as long as it's the right kind of WebAssembly exception. I can't check because I don't know for a fact that the current codegen will *ever* hit that limit! But with the randomness I throw in it's likely to hit the limits we want.
Comment 16 Saam Barati 2017-06-27 23:08:58 PDT
My only question is: have you verified locally all paths are hit at some point? It’s ok if this is tied to a particular platform.
Comment 17 JF Bastien 2017-06-27 23:41:38 PDT
(In reply to Saam Barati from comment #16)
> My only question is: have you verified locally all paths are hit at some
> point? It’s ok if this is tied to a particular platform.

I believe I have, yes. My answer was trying to illustrate how the tests are written to stay relatively current even if some of our implementation changes.
Comment 18 WebKit Commit Bot 2017-06-27 23:42:16 PDT
Comment on attachment 313995 [details]
patch

Clearing flags on attachment: 313995

Committed r218868: <http://trac.webkit.org/changeset/218868>
Comment 19 WebKit Commit Bot 2017-06-27 23:42:18 PDT
All reviewed patches have been landed.  Closing bug.