Bug 144954 – Small refactoring before ES6 Arrow function implementation

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

[GSkachkov]

It is necessary make small refactoring of parser/Parser.cpp before arrow function implementation.

[GSkachkov]

Created attachment 253037 [details]
Patch

[GSkachkov]

Comment on attachment 253037 [details]
Patch

Small refactoring. Landing of this page allow to move to the base implemention of the arrow function.

[Filip Pizlo]

Comment on attachment 253037 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:2
> +        Small refactoring of the parcer.cpp file. It is necessary to increase readability of the patch

Add a blank line here.

> Source/JavaScriptCore/parser/Parser.cpp:1333
> -
> +    

Revert.

[WebKit Commit Bot]

Comment on attachment 253037 [details]
Patch

Rejecting attachment 253037 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 253037, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/4708680951922688

[GSkachkov]

Created attachment 253058 [details]
Patch

[GSkachkov]

Created attachment 253059 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 253059 [details]
Patch

Clearing flags on attachment: 253059

Committed r184313: <http://trac.webkit.org/changeset/184313>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Looks like this patch introduced a crash on 32-bit builds:
https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/3967/steps/webkit-32bit-jsc-test/logs/stdio
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/10038/steps/webkit-32bit-jsc-test/logs/stdio

jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: PASS ({get a([x]){}}) threw exception SyntaxError: Unexpected token '['. getter functions must have no parameters..
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: ASSERTION FAILED: !hasError()
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: /Volumes/Data/slave/yosemite-32bitJSC-debug/build/Source/JavaScriptCore/parser/Parser.h(823) : JSC::Parser::SavePoint JSC::Parser<JSC::Lexer<unsigned char> >::createSavePoint() [LexerType = JSC::Lexer<unsigned char>]
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 1   0xb1619d WTFCrash
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 2   0x8f7bf2 JSC::Parser<JSC::Lexer<unsigned char> >::createSavePoint()
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 3   0x9853b1 JSC::SyntaxChecker::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::SyntaxChecker>(JSC::SyntaxChecker&, JSC::SourceElementsMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 4   0x984cad JSC::ASTBuilder::FunctionBody JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionBody<JSC::ASTBuilder>(JSC::ASTBuilder&, int, int, int, JSC::ConstructorKind)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 5   0x979b8f bool JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionInfo<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::FunctionRequirements, JSC::FunctionParseMode, bool, JSC::ConstructorKind, JSC::SuperBinding, int, JSC::ParserFunctionInfo<JSC::ASTBuilder>&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 6   0x982077 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseGetterSetter<JSC::ASTBuilder>(JSC::ASTBuilder&, bool, JSC::PropertyNode::Type, unsigned int, JSC::ConstructorKind, JSC::SuperBinding)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 7   0x980834 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 8   0x97d562 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseObjectLiteral<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 9   0x976d8f JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parsePrimaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 10  0x975400 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 11  0x9740fe JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 12  0x9731eb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 13  0x9726df JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 14  0x971acb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 15  0x97109d JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 16  0x976df0 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parsePrimaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 17  0x975400 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 18  0x9740fe JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 19  0x9731eb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 20  0x9726df JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 21  0x971acb JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 22  0x97109d JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 23  0x970da7 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionStatement<JSC::ASTBuilder>(JSC::ASTBuilder&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 24  0x965fe9 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 25  0x8f47a8 JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SourceElementsMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 26  0x8f3f0e JSC::Parser<JSC::Lexer<unsigned char> >::parseInner()
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 27  0x1dce9c std::__1::unique_ptr<JSC::EvalNode, std::__1::default_delete<JSC::EvalNode> > JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::EvalNode>(JSC::ParserError&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 28  0x1dbdda std::__1::unique_ptr<JSC::EvalNode, std::__1::default_delete<JSC::EvalNode> > JSC::parse<JSC::EvalNode>(JSC::VM*, JSC::SourceCode const&, JSC::FunctionParameters*, JSC::Identifier const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::JSParserCodeType, JSC::ParserError&, JSC::JSTextPosition*, JSC::ConstructorKind, JSC::ThisTDZMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 29  0x1dab4c JSC::UnlinkedEvalCodeBlock* JSC::CodeCache::getGlobalCodeBlock<JSC::UnlinkedEvalCodeBlock, JSC::EvalExecutable>(JSC::VM&, JSC::EvalExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::ThisTDZMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 30  0x1d918f JSC::CodeCache::getEvalCodeBlock(JSC::VM&, JSC::EvalExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::ThisTDZMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: 31  0x76d821 JSC::JSGlobalObject::createEvalCodeBlock(JSC::ExecState*, JSC::EvalExecutable*, JSC::ThisTDZMode)
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: test_script_18968: line 2:  4296 Segmentation fault: 11  "$@" ../../../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --enableFunctionDotArguments\=true resources/standalone-pre.js object-literal-syntax.js resources/standalone-post.js
jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout: ERROR: Unexpected exit code: 139
FAIL: jsc-layout-tests.yaml/js/script-tests/object-literal-syntax.js.layout

[Ryosuke Niwa]

Actually, it's nothing to do with 32-bit-ness. It's an assertion failure.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144974

[Ryosuke Niwa]

More crash traces:
https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r184313%20(4304)/results.html
https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK1%20(Tests)/r184313%20(4641)/results.html

[Saam Barati]

Comment on attachment 253059 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:1314
>  {

It's also weird that we we're returning bools and ints from this function.
I think there is probably some case that isn't being thought through.

> Source/JavaScriptCore/parser/Parser.cpp:1363
> +    int parametersStart = parseFunctionParamters(context, requirements, mode, nameIsInContainingScope, functionScope, info);

You might need to propagate errors here.

[GSkachkov]

Created attachment 253119 [details]
Patch

[GSkachkov]

Comment on attachment 253059 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:1314
>>  {
> 
> It's also weird that we we're returning bools and ints from this function.
> I think there is probably some case that isn't being thought through.

I'm quite new in webkit, so I tried to follow exist examples. Could you please suggest what I should change in this function?

>> Source/JavaScriptCore/parser/Parser.cpp:1363
>> +    int parametersStart = parseFunctionParamters(context, requirements, mode, nameIsInContainingScope, functionScope, info);
> 
> You might need to propagate errors here.

Done in new patch

[GSkachkov]

(In reply to comment #10)
> Actually, it's nothing to do with 32-bit-ness. It's an assertion failure.

I see, my fault. I always run tests in release mode, because in debug mode it is run so slowly on my Laptop.  
Will check in debug mode also before load patch.

[WebKit Commit Bot]

Comment on attachment 253119 [details]
Patch

Clearing flags on attachment: 253119

Committed r184349: <http://trac.webkit.org/changeset/184349>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.