Bug 146133 – Crash in com.apple.WebKit.WebContent at com.apple.JavaScriptCore: JSC::FTL::fixFunctionBasedOnStackMaps + 17225

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

[Michael Saboff]

The FTL inline cache code uses constants sizes to allocate space for inline cache code.  Those constants are empirically determined, yet we seems to hit cases where the constants are exceeded.  when that happens we don't have any fallback, instead we RELEASE_ASSERT.

Instead, if the code we want to generate is larger than is available, we should generate the code out of line and jump to it from the inline area.

<rdar://problem/20772159>

[Michael Saboff]

Created attachment 255157 [details]
Work in progress that forces the out of line case all the time.  Posted for EWS testing.

[WebKit Commit Bot]

Attachment 255157 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLJITFinalizer.h:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLJITFinalizer.h:46:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:167:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:168:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:199:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:114:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:115:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 7 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 255157 [details]
Work in progress that forces the out of line case all the time.  Posted for EWS testing.

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

> Source/JavaScriptCore/assembler/LinkBuffer.h:112
> +        if (macroAssembler.m_assembler.buffer().codeSize() > size || size > 10) {

I assume the "10" here is just for testing?

> Source/JavaScriptCore/ftl/FTLCompile.cpp:168
> +static void generateICIfPossibleOutOfLineIfNot(State& state, VM& vm, CodeBlock* codeBlock, CCallHelpers& code, char* startOfInlineCode, size_t sizeOfInlineCode, const char* codeDescription, const std::function<void(LinkBuffer&, CCallHelpers&, bool wasCompiledInline)>& callback)
> +{
> +    bool compiledInline = true;
> +
> +    std::unique_ptr<LinkBuffer> codeLinkBuffer;
> +    size_t actualCodeSize = code.m_assembler.buffer().codeSize();
> +
> +    if ((actualCodeSize > sizeOfInlineCode) || (sizeOfInlineCode > 10)) {
> +        // If there isn't enough space in the provided inline code area, allocate out of line
> +        // executable memory to link the provided code.  Place a jump at the beginning of the
> +        // inline area and jump to the out of line code.  Similarly return by appending a jump

This function seems redundant with the LinkBuffer thing.  Will we have both or just one of them?

> Source/JavaScriptCore/ftl/FTLCompile.cpp:231
> +        auto postLink = [&](LinkBuffer& linkBuffer, CCallHelpers&, bool)
> +        {

We usually say lambdas this way:

auto postLink = [&] (LinkBuffer& linkBuffer, CCallHelpers&, bool) {

That is, space between "]" and "(" and we put the "{" on the same line.  I don't know how this style arose, but I saw that WK2 code was doing it so that's how I started writing my lambdas.  Now I guess it has taken over.  But stylebot doesn't know yet.

[Michael Saboff]

(In reply to comment #3)
> Comment on attachment 255157 [details]
> Work in progress that forces the out of line case all the time.  Posted for
> EWS testing.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255157&action=review
> 
> > Source/JavaScriptCore/assembler/LinkBuffer.h:112
> > +        if (macroAssembler.m_assembler.buffer().codeSize() > size || size > 10) {
> 
> I assume the "10" here is just for testing?

Yes.

> > Source/JavaScriptCore/ftl/FTLCompile.cpp:168
> > +static void generateICIfPossibleOutOfLineIfNot(State& state, VM& vm, CodeBlock* codeBlock, CCallHelpers& code, char* startOfInlineCode, size_t sizeOfInlineCode, const char* codeDescription, const std::function<void(LinkBuffer&, CCallHelpers&, bool wasCompiledInline)>& callback)
> > +{
> > +    bool compiledInline = true;
> > +
> > +    std::unique_ptr<LinkBuffer> codeLinkBuffer;
> > +    size_t actualCodeSize = code.m_assembler.buffer().codeSize();
> > +
> > +    if ((actualCodeSize > sizeOfInlineCode) || (sizeOfInlineCode > 10)) {
> > +        // If there isn't enough space in the provided inline code area, allocate out of line
> > +        // executable memory to link the provided code.  Place a jump at the beginning of the
> > +        // inline area and jump to the out of line code.  Similarly return by appending a jump
> 
> This function seems redundant with the LinkBuffer thing.  Will we have both
> or just one of them?

The new LinkBuffer constructor was the first try at a fix.  It will be removed.

> > Source/JavaScriptCore/ftl/FTLCompile.cpp:231
> > +        auto postLink = [&](LinkBuffer& linkBuffer, CCallHelpers&, bool)
> > +        {
> 
> We usually say lambdas this way:
> 
> auto postLink = [&] (LinkBuffer& linkBuffer, CCallHelpers&, bool) {
> 
> That is, space between "]" and "(" and we put the "{" on the same line.  I
> don't know how this style arose, but I saw that WK2 code was doing it so
> that's how I started writing my lambdas.  Now I guess it has taken over. 
> But stylebot doesn't know yet.

I updated the lambdas accordingly.  I inlined the three smaller ones.

[Michael Saboff]

Created attachment 255202 [details]
Patch

[WebKit Commit Bot]

Attachment 255202 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:272:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 255202 [details]
Patch

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

r=me with some edits below.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:170
> +        // Fill the middle with nop's

Needs a ".".

> Source/JavaScriptCore/ftl/FTLCompile.cpp:175
> +        codeLinkBuffer = std::make_unique<LinkBuffer>(vm, code, codeBlock, JITCompilationCanFail);

We shouldn't use JITCompilationCanFail because we don't have a recovery path, so it's a security bug to allow compilation to fail without crashing. Let's use JITCompilationMustSucceed.

Long-term, we should write the recover path -- but we didn't have it before, so I won't ask you to add it in this patch.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:178
> +        MacroAssembler callToOutOfLineJit;

Let's call this callToOutOfLineCode. We're not calling to a JIT, and "Jit" is the wrong style.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:183
> +        // Fill the remainder of the inline space with nops

Let's say why we're doing this (for the disassembler), and put a "." at the end.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:195
> +        codeLinkBuffer = std::make_unique<LinkBuffer>(vm, code, startOfInlineCode, sizeOfInlineCode);
> +
> +        // Fill the remainder of the inline space with nops
> +        MacroAssembler::AssemblerType_T::fillNops(bitwise_cast<char*>(startOfInlineCode) + actualCodeSize, sizeOfInlineCode - actualCodeSize);
> +    }
> +
> +    callback(*codeLinkBuffer.get(), code, compiledInline);

I think this would read more clearly if you put the common case first followed by early return. That way, we don't need to track a boolean to say what we did, and we don't need to malloc the link buffer in the common case.

[Michael Saboff]

Created attachment 255211 [details]
Patch for landing

Made changes suggested by Geoff.

Fixed a debug build failure.

[WebKit Commit Bot]

Attachment 255211 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:272:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 255217 [details]
Updated patch to fix build

Turns out LinkBuffer::isValid() is used in other places.

[WebKit Commit Bot]

Attachment 255217 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLCompile.cpp:272:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Committed r185772: <http://trac.webkit.org/changeset/185772>