Bug 170934 - Computed Properties with increment sometimes produces incorrect results
Summary: Computed Properties with increment sometimes produces incorrect results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-18 00:37 PDT by Joseph Pecoraro
Modified: 2020-02-24 10:21 PST (History)
13 users (show)

See Also:


Attachments
patch-ish (713 bytes, patch)
2017-04-18 13:17 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2020-02-19 15:15 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (6.67 KB, patch)
2020-02-19 18:43 PST, Ross Kirsling
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-18 00:37:21 PDT
Summary:
Computed Properties with increment sometimes produces incorrect results

Test:

    let c = 0;
    let o1 = { [++c]: ++c };
    // Should be 1 => 2 and is.

    {
        let x = 0;
        let o2 = { [++x]: ++x };
        // Should be 1 => 2, but is not. Instead it is 2 => 2?!
    }

Notes:

> <global>#EZzSqs:[0x10f3740a0->0x10f37c520, NoneGlobal, 122]: 122 m_instructions; 976 bytes; 1 parameter(s); 12 callee register(s); 5 variable(s); scope at loc3
> [   0] enter             
> [   1] get_scope         loc3
> [   3] mov               loc4, loc3
> [   6] check_traps       
> [   7] mov               loc5, Undefined(const0)
> [  10] resolve_scope     loc6, loc3, c(@id0), <GlobalLexicalVar>, 0, 0x10f3c40a0
> [  17] put_to_scope      loc6, c(@id0), Int32: 0(const1), 1048578<DoNotThrowIfNotFound|GlobalLexicalVar|Initialization>, <structure>, 242782464
> [  24] resolve_scope     loc6, loc3, o1(@id1), <GlobalLexicalVar>, 0, 0x10f3c40a0
> [  31] new_object        loc7, 0
> [  35] resolve_scope     loc8, loc3, c(@id0), <GlobalLexicalVar>, 0, 0x10f3c40a0
> [  42] get_from_scope    loc9, loc8, c(@id0), 2050<ThrowIfNotFound|GlobalLexicalVar|NotInitialization>, 242782464    predicting None
> [  50] inc               loc9
> [  52] put_to_scope      loc8, c(@id0), loc9, 2050<ThrowIfNotFound|GlobalLexicalVar|NotInitialization>, <structure>, 242782464
> [  59] resolve_scope     loc10, loc3, c(@id0), <GlobalLexicalVar>, 0, 0x10f3c40a0
> [  66] get_from_scope    loc11, loc10, c(@id0), 2050<ThrowIfNotFound|GlobalLexicalVar|NotInitialization>, 242782464    predicting None
> [  74] inc               loc11
> [  76] put_to_scope      loc10, c(@id0), loc11, 2050<ThrowIfNotFound|GlobalLexicalVar|NotInitialization>, <structure>, 242782464
> [  83] put_by_val_direct loc7, loc9, loc11    Original
> [  88] put_to_scope      loc6, o1(@id1), loc7, 1048578<DoNotThrowIfNotFound|GlobalLexicalVar|Initialization>, <structure>, 242782472
> [  95] mov               loc6, <JSValue()>(const2)
> [  98] mov               loc7, <JSValue()>(const2)
> [ 101] mov               loc6, Int32: 0(const1)
> [ 104] new_object        loc8, 0
> [ 108] inc               loc6
> [ 110] inc               loc6
> [ 112] put_by_val_direct loc8, loc6, loc6    Original
> [ 117] mov               loc7, loc8
> [ 120] end               loc5

See Object 1's construction [31-88] compared to Object 2's [104-112].
Comment 1 Saam Barati 2017-04-18 13:17:49 PDT
Created attachment 307408 [details]
patch-ish

We weren't assigning the ++x to a new register, that was the bug.
Comment 2 Joseph Pecoraro 2017-04-18 13:27:00 PDT
Excellent, I'll include this when I also fix the order (computed property should happen first before the value).
Comment 3 Ross Kirsling 2020-02-19 15:15:20 PST
Created attachment 391209 [details]
Patch
Comment 4 Yusuke Suzuki 2020-02-19 17:22:06 PST
Comment on attachment 391209 [details]
Patch

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

r=me with one comment.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:742
>      if (const auto* identifier = node.name()) {

Let's insert `ASSERT(!propertyName);`.
Comment 5 Ross Kirsling 2020-02-19 18:43:16 PST
Created attachment 391240 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2020-02-19 19:01:09 PST
Comment on attachment 391240 [details]
Patch for landing

Clearing flags on attachment: 391240

Committed r257034: <https://trac.webkit.org/changeset/257034>
Comment 7 WebKit Commit Bot 2020-02-19 19:01:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2020-02-19 19:02:15 PST
<rdar://problem/59615634>
Comment 9 Caitlin Potter (:caitp) 2020-02-24 06:56:06 PST
It's nice to see some other uses of the new opcode unrelated to class fields :)
👍
Comment 10 Ross Kirsling 2020-02-24 10:21:29 PST
(In reply to Caitlin Potter (:caitp) from comment #9)
> It's nice to see some other uses of the new opcode unrelated to class fields
> :)
> 👍

Yeah! :D I realized in the previous bug (bug 207297) that it was exactly the thing we needed.