Bug 171265 - test262: Completion values for control flow do not match the spec
Summary: test262: Completion values for control flow do not match the spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on: 173981
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-25 02:19 PDT by Joseph Pecoraro
Modified: 2017-09-27 12:59 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Work in Progress (43.72 KB, patch)
2017-04-25 02:25 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (943.84 KB, application/zip)
2017-04-25 03:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (908.47 KB, application/zip)
2017-04-25 03:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.77 MB, application/zip)
2017-04-25 03:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (11.58 MB, application/zip)
2017-04-25 04:00 PDT, Build Bot
no flags Details
[PATCH] Work in Progress (46.29 KB, patch)
2017-04-25 22:32 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (50.18 KB, patch)
2017-04-26 00:25 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix v2 (68.48 KB, patch)
2017-04-29 01:47 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix v2 (70.85 KB, patch)
2017-04-29 16:21 PDT, Joseph Pecoraro
joepeck: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix v3 (74.81 KB, patch)
2017-05-27 13:04 PDT, Joseph Pecoraro
joepeck: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (21.54 MB, application/zip)
2017-05-27 16:20 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix v3 (78.38 KB, patch)
2017-05-27 18:10 PDT, Joseph Pecoraro
sbarati: review+
Details | Formatted Diff | Diff
[PATCH] Proposed Fix v4 (83.16 KB, patch)
2017-06-19 00:23 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Rebaselined (83.12 KB, patch)
2017-09-05 01:05 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-04-25 02:19:36 PDT
test262: Completion values for control flow do not match the spec

Lots of test262 tests with "cptn" in the name fail.

Example:

    assert( eval("1; if(false);") === undefined )
    assert( eval("1; while(false);") === undefined )
    assert( eval("1; try{}catch(e){};") === undefined )
    ...
    assert( eval("0; try {throw 3} catch (e) {2} finally {1}") === 2 )

Expected:
* Most control flow cases return undefined when no statement executes, otherwise the statements inside them
* Finally is a bit whacky in that its value is not used, the try (or catch) value is used instead

Actual:
* JSC produces 1 for each of the productions above

Notes:
* Chrome seems to match the spec for each of these
* Firefox matches us for a few (finally, empty blocks)
* Not all statements produce values (variable declarations).

Spec:

Read through each of the different statement runtime semantics to see their Completion Values:
https://tc39.github.io/ecma262/#sec-statement-semantics

The Completion Record which is the foundation for each of these: (NormalCompletion, ReturnIfAbrupt, empty value, ...)
https://tc39.github.io/ecma262/#sec-completion-record-specification-type

Control flow tends to use UpdateEmpty. Most start out prepared to return the "undefined" value, and that only gets replaced if statements produce non-empty values:
https://tc39.github.io/ecma262/#sec-updateempty
Comment 1 Joseph Pecoraro 2017-04-25 02:25:45 PDT
Created attachment 308077 [details]
[PATCH] Work in Progress

So this is the work in progress. It passes all of the "cptn" tests I could find in test262.

A few concerns I have to test and think through more:

  * performance
    - emits some extra stores of undefined in global code / eval code, how much of an impact is it
    - technically in a browser only eval code produces a result, program/module are never seen.
    - however in the jsc command line REPL it is most natural for each line to be a program and get this behavior
  * bytecode caching
    - the store byte codes are only emitted outside of functions, does this interact well with our caches?
  * compatibility
    - eval / JSC APIs (JSEvaluateScript, etc) can return different values now. They should be "more correct" but still different.
Comment 2 Build Bot 2017-04-25 03:03:09 PDT
Comment on attachment 308077 [details]
[PATCH] Work in Progress

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

New failing tests:
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-llint
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-baseline
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-dfg-eager-no-cjit-validate-phases
Comment 3 Build Bot 2017-04-25 03:31:50 PDT
Comment on attachment 308077 [details]
[PATCH] Work in Progress

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

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html
Comment 4 Build Bot 2017-04-25 03:31:51 PDT
Created attachment 308083 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 Build Bot 2017-04-25 03:37:23 PDT
Comment on attachment 308077 [details]
[PATCH] Work in Progress

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

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html
Comment 6 Build Bot 2017-04-25 03:37:24 PDT
Created attachment 308084 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-04-25 03:52:51 PDT
Comment on attachment 308077 [details]
[PATCH] Work in Progress

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

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html
Comment 8 Build Bot 2017-04-25 03:52:52 PDT
Created attachment 308086 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-04-25 04:00:54 PDT
Comment on attachment 308077 [details]
[PATCH] Work in Progress

Attachment 308077 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3600766

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html
Comment 10 Build Bot 2017-04-25 04:00:56 PDT
Created attachment 308088 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 11 Joseph Pecoraro 2017-04-25 22:32:10 PDT
Created attachment 308218 [details]
[PATCH] Work in Progress

This should pass all tests as I rebaselined the last. Going to do performance next.
Comment 12 Joseph Pecoraro 2017-04-26 00:10:06 PDT
>   * performance
>     - emits some extra stores of undefined in global code / eval code, how much of an impact is it
>     - technically in a browser only eval code produces a result, program/module are never seen.
>     - however in the jsc command line REPL it is most natural for each line to be a program and get this behavior

run-jsc-benchmarks shows negligible results, which would be expected since most of these run code in functions not global code. I don't have JSBench, that might be better to test instead of the OpenSource benchmarks like Sunspider.

>   * bytecode caching
>     - the store byte codes are only emitted outside of functions, does this interact well with our caches?

The CodeCache takes the CodeType into account already.

>   * compatibility
>     - eval / JSC APIs (JSEvaluateScript, etc) can return different values now. They should be "more correct" but still different.

This is the price of moving forward. I think it should be rare for users to have some code after the last expression and expect that not to impact the result.
Comment 13 Joseph Pecoraro 2017-04-26 00:25:04 PDT
Created attachment 308229 [details]
[PATCH] Proposed Fix

Reviewable!
Comment 14 Saam Barati 2017-04-26 18:51:05 PDT
Comment on attachment 308229 [details]
[PATCH] Proposed Fix

I talked to Joe in person and suggested he tries a different approach of determining statically the last statement in a statement list that produces a value.
Comment 15 Joseph Pecoraro 2017-04-26 18:57:44 PDT
> I talked to Joe in person and suggested he tries a different approach of
> determining statically the last statement in a statement list that produces
> a value.

I'll need to do some reading to be sure this can be determined completely statically. My guess is yes it can.

For instance a block statement may produce empty (if no statement list) or return the completion value of a statement in the list:
https://tc39.github.io/ecma262/#sec-block-runtime-semantics-evaluation

So a StatementNode::hasCompletionValue for BlockNode might then have to walk its statements recursively. But I think the point (especially with control flow statements producing undefined or a value) is that it should be possible to determine statically.
Comment 16 Joseph Pecoraro 2017-04-29 01:36:30 PDT
After writing a bunch of my own tests I found some issues in other browsers. New test262 tests to cover the differences:
https://github.com/tc39/test262/pull/1012
Comment 17 Joseph Pecoraro 2017-04-29 01:47:59 PDT
Created attachment 308662 [details]
[PATCH] Proposed Fix v2

This takes the approach of statically determining (as best we can) the last statement that can produce a completion value.
Comment 18 Joseph Pecoraro 2017-04-29 01:50:08 PDT
Comment on attachment 308662 [details]
[PATCH] Proposed Fix v2

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:879
> +        bool shouldBeConcernedWithCompletionValue() { return m_codeType != FunctionCode; }

This could be `const`.
Comment 19 Build Bot 2017-04-29 02:26:42 PDT
Comment on attachment 308662 [details]
[PATCH] Proposed Fix v2

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

New failing tests:
stress/super-get-by-id.js.dfg-maximal-flush-validate-no-cjit
stress/super-get-by-id.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
stress/super-get-by-id.js.default
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-ftl
stress/super-get-by-id.js.ftl-no-cjit-no-put-stack-validate
stress/super-get-by-id.js.dfg-eager-no-cjit-validate
stress/super-get-by-id.js.ftl-eager-no-cjit
stress/super-get-by-id.js.no-cjit-collect-continuously
stress/super-get-by-id.js.ftl-eager-no-cjit-b3o1
stress/super-get-by-id.js.ftl-no-cjit-no-inline-validate
stress/super-get-by-id.js.no-llint
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-eager-no-cjit
stress/super-get-by-id.js.ftl-no-cjit-validate-sampling-profiler
stress/super-get-by-id.js.dfg-eager
stress/super-get-by-id.js.no-ftl
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-no-cjit
stress/super-get-by-id.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
stress/super-get-by-id.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
stress/super-get-by-id.js.ftl-eager
Comment 20 Joseph Pecoraro 2017-04-29 16:21:19 PDT
Created attachment 308678 [details]
[PATCH] Proposed Fix v2
Comment 21 Saam Barati 2017-04-29 17:04:12 PDT
Comment on attachment 308678 [details]
[PATCH] Proposed Fix v2

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2466
> +        if (statement == lastStatementWithCompletionValue)

If we do this here, why do we need to load undefined in a few of the various places you added it like with/if-else, etc?
Comment 22 Joseph Pecoraro 2017-04-29 20:58:43 PDT
(In reply to Saam Barati from comment #21)
> Comment on attachment 308678 [details]
> [PATCH] Proposed Fix v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308678&action=review
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2466
> > +        if (statement == lastStatementWithCompletionValue)
> 
> If we do this here, why do we need to load undefined in a few of the various
> places you added it like with/if-else, etc?

This is because a with/if/else can include a break statement. In those cases it may break out of its statement list without having set some completion value. I'll investigate this a bit more and provide a clearer answer.
Comment 23 Joseph Pecoraro 2017-04-30 00:47:04 PDT
> I'll investigate this a bit more and provide a clearer answer.

IfStatement:
https://tc39.github.io/ecma262/#sec-if-statement-runtime-semantics-evaluation

> 13.6.7 Runtime Semantics: Evaluation
> 
> 1. Let exprRef be the result of evaluating Expression.
> 2. Let exprValue be ToBoolean(? GetValue(exprRef)).
> 3. If exprValue is false, then
>     a. Return NormalCompletion(undefined).
> 4. Else,
>     a. Let stmtCompletion be the result of evaluating Statement.
> 5. Return Completion(UpdateEmpty(stmtCompletion, undefined)).

BlockStatement:
https://tc39.github.io/ecma262/#sec-block-runtime-semantics-evaluation

> 13.2.13 Runtime Semantics: Evaluation
> 
> Block : { StatementList }
> 
> 1. Let oldEnv be the running execution context's LexicalEnvironment.
> 2. Let blockEnv be NewDeclarativeEnvironment(oldEnv).
> 3. Perform BlockDeclarationInstantiation(StatementList, blockEnv).
> 4. Set the running execution context's LexicalEnvironment to blockEnv.
> 5. Let blockValue be the result of evaluating StatementList.
> 6. Set the running execution context's LexicalEnvironment to oldEnv.
> 7. Return blockValue.
> 
> 
> StatementList : StatementList StatementListItem
> 
> 1. Let sl be the result of evaluating StatementList.
> 2. ReturnIfAbrupt(sl).
> 3. Let s be the result of evaluating StatementListItem.
> 4. Return Completion(UpdateEmpty(s, sl)).
> 
> NOTE 2
> The value of a StatementList is the value of the last value producing item in the StatementList.
> For example, the following calls to the eval function all return the value 1:
> 
>     eval("1;;;;;")
>     eval("1;{}")
>     eval("1;var a;")


BreakStatement:
https://tc39.github.io/ecma262/#sec-break-statement-runtime-semantics-evaluation

> 13.9.3 Runtime Semantics: Evaluation
> 
> BreakStatement : break;
> 
> 1. Return Completion{[[Type]]: break, [[Value]]: empty, [[Target]]: empty}.

---

So take this case from:
test262/test/language/statements/if/cptn-else-true-abrupt-empty.js

    eval('1; do { 2; if (true) { break; } else { }; 3 } while (false)')

The AST roughly breaks down as:

    - Program
      - SourceElements
        - ExpressionStatement - '1;'
        - DoWhileStatement
          - BlockStatement
            - ExpressionStatement - '2'
            - IfStatement
              - Expression - 'true'
              - BlockStatement
                - BreakStatement - 'break'
            - ExpressionStatement - '3'
          - Condition - 'false'

The last statement of the global code to have a completion value is the DoWhileStatement. It should produce undefined or the completion value of its block.

When we generate bytecode for the block we detect the last statement is the Expression Statement '3' so we'll load an undefined before evaluating that expression statement.

However, while executing the block the `3` is not the last statement to execute. The block evaluate the `2`. Then when evaluating the if expression, which should produce a value or `undefined`, we `break`. This break actually takes us out of the do/while. So the do/while's BlockStatement got its completion value from the last statement to execute (the IfStatement).

For this to be correct something needs to produce the `undefined` here. So that is why the load is needed.

---

This boils down to:

A BreakableStatement (loops and switch) may get its completion value from something other than the last statement in its BlockStatement due to the break statement. In those cases, statements which need to produce value or undefined (namely IfElse, With, or TryCatch) need to produce the undefineds that are expected of it.

---

This might still be statically detectable, but it is starting to no longer be clean / straightforward. I think we could do this by only generating the undefined load inside a IfElse/With/Try if it (1) contains a break statement and (2) no statement before the break statement has a completion value. Something like a "hasEarlyBreak".

Which of course means walking into sub-blocks and over statements:

    do {
        1;
        if (true) {
            var emptyCompletionValue;
            {
                { }
                function foo() {};
                debugger;
                break;
            }
        }
        2;
    } while (false);

I'm not sure how deep we want to go. The complexity of this approach might not be warranted.
Comment 24 Joseph Pecoraro 2017-04-30 00:49:37 PDT
Comment on attachment 308678 [details]
[PATCH] Proposed Fix v2

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

> JSTests/stress/completion-value.js:190
> +shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while (false);`), undefined);

A slight modification to this causes it to fail:

    shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while (false);`), undefined); // pass
    shouldBe(eval(`99; do { -2; try { break; } catch (e) { -1 }; -3 } while (false);`), undefined); // fails with -2

We would need the loads of undefined like we did with IfElse/With.
Comment 25 Saam Barati 2017-05-04 01:24:47 PDT
(In reply to Joseph Pecoraro from comment #24)
> Comment on attachment 308678 [details]
> [PATCH] Proposed Fix v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308678&action=review
> 
> > JSTests/stress/completion-value.js:190
> > +shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while (false);`), undefined);
> 
> A slight modification to this causes it to fail:
> 
>     shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while
> (false);`), undefined); // pass
>     shouldBe(eval(`99; do { -2; try { break; } catch (e) { -1 }; -3 } while
> (false);`), undefined); // fails with -2
> 
> We would need the loads of undefined like we did with IfElse/With.
Does your first proposed fix also break?
Comment 26 Joseph Pecoraro 2017-05-27 13:04:02 PDT
Created attachment 311422 [details]
[PATCH] Proposed Fix v3

Extended tests to cover more cases. More checks are done statically to avoid outputting the `undefined` in If/With/Try cases.
Comment 27 Build Bot 2017-05-27 16:20:13 PDT
Comment on attachment 311422 [details]
[PATCH] Proposed Fix v3

Attachment 311422 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3829072

New failing tests:
http/tests/cache/cancel-during-revalidation-succeeded.html
webrtc/peer-connection-audio-mute.html
fast/css/target-fragment-match.html
Comment 28 Build Bot 2017-05-27 16:20:15 PDT
Created attachment 311426 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 29 Joseph Pecoraro 2017-05-27 17:35:50 PDT
Comment on attachment 311422 [details]
[PATCH] Proposed Fix v3

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

> Source/JavaScriptCore/parser/Nodes.cpp:83
> +bool SourceElements::hasEarlyBreak() const

This isn't enough, it needs to allow for continue as well. For example:

    assert.sameValue(
      eval('12; do { 13; if (true) { continue; } 14; } while (false)'), undefined
    );
Comment 30 Joseph Pecoraro 2017-05-27 18:10:09 PDT
Created attachment 311430 [details]
[PATCH] Proposed Fix v3
Comment 31 Saam Barati 2017-05-31 14:33:38 PDT
Comment on attachment 311430 [details]
[PATCH] Proposed Fix v3

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

r=me

> JSTests/mozilla/mozilla-tests.yaml:1035
> +  cmd: defaultRunMozillaTest :failDueToOutdatedOrBadTest, "../shell.js"

my favorite kind of failure.

> JSTests/stress/completion-value.js:20
> +// Statements that produce an (empty) completion value do not affect results:

This test is great. Maybe it's so good we should give it to test262 if they want it?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2593
> +    if (generator.shouldBeConcernedWithCompletionValue()) {

So this is the completion value of the "if" itself? Would it work just to put this logic in SourceElements::emitBytecode?

Perhaps another way to phrase what I'm asking: If we have an earlyBreakOrContinue(), are there places we actually get to skip caring about the return value? Maybe it's worth consolidating this logic in SourceElements::emitBytecode just for cleanliness?

I'm leaving it up to you since you understand the problem here much better than I do.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3345
> +    if (generator.shouldBeConcernedWithCompletionValue() && m_tryBlock->hasEarlyBreakOrContinue())

ditto

> Source/JavaScriptCore/parser/Nodes.cpp:83
> +bool SourceElements::hasEarlyBreakOrContinue() const

nice!
Comment 32 Joseph Pecoraro 2017-06-01 13:55:53 PDT
Comment on attachment 311430 [details]
[PATCH] Proposed Fix v3

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

>> JSTests/stress/completion-value.js:20
>> +// Statements that produce an (empty) completion value do not affect results:
> 
> This test is great. Maybe it's so good we should give it to test262 if they want it?

They have a bunch of the trickier ones, but there are some that they don't have. I added a bunch they were missing and other browsers were failing:
https://github.com/tc39/test262/pull/1012

I can search and individually add others. They have a nice naming convention, but it just means a lot of small tests in a lot of different places. Its work, but kind of nice in the end.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2593
>> +    if (generator.shouldBeConcernedWithCompletionValue()) {
> 
> So this is the completion value of the "if" itself? Would it work just to put this logic in SourceElements::emitBytecode?
> 
> Perhaps another way to phrase what I'm asking: If we have an earlyBreakOrContinue(), are there places we actually get to skip caring about the return value? Maybe it's worth consolidating this logic in SourceElements::emitBytecode just for cleanliness?
> 
> I'm leaving it up to you since you understand the problem here much better than I do.

Yes, this is the completion value of the if statement itself. It'll either produce undefined or the last completion value of its block.

Earlier I had determined we could put this just inside of If/With/Try which should allow us to skip putting it in While/For/Do. The idea being that all of these have a completion value of undefined || value and we aim to only produce the undefined if this is the last statement that can include a completion value. If an If/With/Try is nested inside of a While/For/Do then its completion value can change based on whether or not the if is reached.

I'll try some other cases I just thought of to confirm this a bit more. I've since forgotten relevant parts of the spec. Note to self, test these cases:

    99; do { 1; do { break; 2; } while (false); } while (false);
    99; do { 1; do { break; 2; } while (false); 3; } while (false);
    99; label: do { 1; do { break label; 2; } while (false); } while (false);
    99; label: do { 1; do { break label; 2; } while (false); 3; } while (false);
Comment 33 Saam Barati 2017-06-01 22:51:01 PDT
Comment on attachment 311430 [details]
[PATCH] Proposed Fix v3

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

>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2593
>>> +    if (generator.shouldBeConcernedWithCompletionValue()) {
>> 
>> So this is the completion value of the "if" itself? Would it work just to put this logic in SourceElements::emitBytecode?
>> 
>> Perhaps another way to phrase what I'm asking: If we have an earlyBreakOrContinue(), are there places we actually get to skip caring about the return value? Maybe it's worth consolidating this logic in SourceElements::emitBytecode just for cleanliness?
>> 
>> I'm leaving it up to you since you understand the problem here much better than I do.
> 
> Yes, this is the completion value of the if statement itself. It'll either produce undefined or the last completion value of its block.
> 
> Earlier I had determined we could put this just inside of If/With/Try which should allow us to skip putting it in While/For/Do. The idea being that all of these have a completion value of undefined || value and we aim to only produce the undefined if this is the last statement that can include a completion value. If an If/With/Try is nested inside of a While/For/Do then its completion value can change based on whether or not the if is reached.
> 
> I'll try some other cases I just thought of to confirm this a bit more. I've since forgotten relevant parts of the spec. Note to self, test these cases:
> 
>     99; do { 1; do { break; 2; } while (false); } while (false);
>     99; do { 1; do { break; 2; } while (false); 3; } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); 3; } while (false);

It might not be necessary. Perhaps we should just land what you have, I think the patch is elegant as is.
Comment 34 Joseph Pecoraro 2017-06-18 00:00:06 PDT
> I'll try some other cases I just thought of to confirm this a bit more. I've
> since forgotten relevant parts of the spec. Note to self, test these cases:
> 
>     99; do { 1; do { break; 2; } while (false); } while (false);
>     99; do { 1; do { break; 2; } while (false); 3; } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); 3; } while
> (false);

Yeah I was incorrect. We'll need to do this for do/for/while as well.

The last case produces `3` with this patch but should have been `undefined`.

All these smarts and I think we'll end up close to the original patch with some logic to avoid outputting a few bytecodes in some cases.
Comment 35 Joseph Pecoraro 2017-06-18 00:13:00 PDT
> The last case produces `3` with this patch but should have been `undefined`.

Err it produced `1` and should have been `undefined`. :)
Comment 36 Joseph Pecoraro 2017-06-19 00:23:25 PDT
Created attachment 313268 [details]
[PATCH] Proposed Fix v4
Comment 37 WebKit Commit Bot 2017-06-19 15:04:24 PDT
Comment on attachment 313268 [details]
[PATCH] Proposed Fix v4

Clearing flags on attachment: 313268

Committed r218512: <http://trac.webkit.org/changeset/218512>
Comment 38 WebKit Commit Bot 2017-06-19 15:04:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 WebKit Commit Bot 2017-06-29 12:31:22 PDT
Re-opened since this is blocked by bug 173981
Comment 40 Joseph Pecoraro 2017-09-05 01:05:10 PDT
Created attachment 319885 [details]
[PATCH] Rebaselined

The patch re-applied without any issue. I re-ran tests and things look good. I'll see what the EWS bots think and then cq this.
Comment 41 WebKit Commit Bot 2017-09-05 10:43:55 PDT
Comment on attachment 319885 [details]
[PATCH] Rebaselined

Clearing flags on attachment: 319885

Committed r221622: <http://trac.webkit.org/changeset/221622>
Comment 42 WebKit Commit Bot 2017-09-05 10:43:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 43 Radar WebKit Bug Importer 2017-09-27 12:59:06 PDT
<rdar://problem/34694423>