[webkit-changes] [WebKit/WebKit] 31319e: Difference in semantics between jsTypeStringForVal...

[Justin Michaud]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 31319e7a0b96c61cd61e17377adc648e4b8b5692
      https://github.com/WebKit/WebKit/commit/31319e7a0b96c61cd61e17377adc648e4b8b5692
  Author: Justin Michaud <justin at justinmichaud.com>
  Date:   2024-05-16 (Thu, 16 May 2024)

  Changed paths:
    A JSTests/stress/getter-setter-ai.js
    M Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
    M Source/JavaScriptCore/runtime/Operations.cpp

  Log Message:
  -----------
  Difference in semantics between jsTypeStringForValueWithConcurrency and buildTypeOf
https://bugs.webkit.org/show_bug.cgi?id=270659
rdar://124116542

Reviewed by Yusuke Suzuki.

Consider the given test case:

Object1: 0x30000bba0 %DL (should never getByOffset p3 of this, it is a GetterSetter)
Object2: 0x30000bc10 %DS (p3 is fine)

 Before LICM:
32  0 40:   D at 26:<!0:->	FilterGetByStatus(Check:Untyped:D at 7, MustGen, (Simple, <id='uid:(p1)', [0x30000bc10:[0xbc10/48144, Function, (0/0, 3/4){p1:64, p2:65, p3:66}, NonArray, Proto:0x11706ddc8, Leaf (Watched)], 0x30000bba0:[0xbba0/48032, Function, (0/0, 3/4){p1:64, p2:65, p3:66}, NonArray, Proto:0x11706ddc8, Leaf (Watched)]], [], offset = 64>, seenInJIT = true), W:SideState, bc#4, ExitValid)
33  0 40:   D at 15:<!0:->	AssertNotEmpty(Check:Untyped:D at 7, MustGen, W:SideState, Exits, bc#4, ExitValid)
34  0 40:   D at 28:<!0:->	CheckStructure(Cell:D at 7, MustGen, [%DS:Function, %DL:Function], R:JSCell_structureID, Exits, bc#4, ExitValid)
35  0 40:   D at 29:< 2:->	GetButterfly(Cell:D at 7, Storage|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Other, R:JSObject_butterfly, bc#4, ExitValid)
36  0 40:   D at 30:< 1:->	GetByOffset(Check:Untyped:D at 29, KnownCell:D at 7, JS|PureInt, Int32, id0{p1}, 64, R:NamedProperties(0), bc#4, ExitValid)  predicting Int32

... branching

6  7 40:   D at 87:<!0:->	FilterGetByStatus(Check:Untyped:D at 7, MustGen, (Simple, <id='uid:(p3)', [0x30000bc10:[0xbc10/48144, Function, (0/0, 3/4){p1:64, p2:65, p3:66}, NonArray, Proto:0x11706ddc8, Leaf (Watched)]], [], offset = 66>, seenInJIT = true), W:SideState, bc#45, ExitValid)
7  7 40:   D at 89:<!0:->	CheckStructure(Cell:D at 7, MustGen, [%DS:Function], R:JSCell_structureID, Exits, bc#45, ExitValid)
8  7 40:   D at 91:< 2:->	GetByOffset(Check:Untyped:D at 29, KnownCell:D at 7, JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, BoolInt32, id3{p3}, 66, R:NamedProperties(3), bc#45, ExitValid)  predicting BoolInt32
11  7 40:   D at 94:< 2:->	TypeOf(Check:Untyped:Kill:D at 91, JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Exits, bc#51, ExitValid)

Note that we never get p3 of DL

After LICM blind hoist:
 34  0 41:   D at 28:<!0:->	CheckStructure(Cell:D at 7, MustGen, [%DS:Function, %DL:Function], R:JSCell_structureID, Exits, bc#4, ExitValid)
 35  0 41:   D at 29:< 2:->	GetButterfly(Cell:D at 7, Storage|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, Other, R:JSObject_butterfly, bc#4, ExitValid)
 36  0 41:   D at 30:< 1:->	GetByOffset(Check:Untyped:D at 29, KnownCell:D at 7, JS|PureInt, Int32, id0{p1}, 64, R:NamedProperties(0), bc#4, ExitValid)  predicting Int32
 44  0 41:   D at 48:<!0:->	CheckIsConstant(Cell:D at 7, MustGen, <0x13908f140, Function>, object1#B5FU55/<nogen>:[0x13909da00], Exits, bc#25, exit: bc#17, ExitValid, WasHoisted)
 45  0 41:   D at 91:< 2:->	GetByOffset(Check:Untyped:D at 29, KnownCell:D at 7, JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, BoolInt32, id3{p3}, 66, R:NamedProperties(3), bc#45, exit: bc#17, ExitValid)  predicting BoolInt32
 46  0 41:   D at 94:< 2:->	TypeOf(Check:Untyped:Kill:D at 91, JS|PureNum|NeedsNegZero|NeedsNaNOrInfinity|UseAsOther, StringIdent, Exits, bc#51, exit: bc#17, ExitValid, WasHoisted)

The GetByOffset is hoisted without its guarding CheckStructure, and it accesses p3 unexpectedly. SafeToExecute says it is
safe because it won't crash or produce a malformed JSValue. Honestly, fair.

This patch fixes the semantic difference between AI and runtime for GetterSetter objects.
Stopping the GetterSetter from being hoisted may be too costly and restrictive, and it
doesn't get leaked anyway.

The string result (which was [object] but is now [symbol]) doesn't really matter, it should
never leak to user code anyway. Even if it does, it is just a string.

* JSTests/stress/getter-setter-ai.js: Added.
(opt):
(watchP3.cache):
(watchP3):
(main.let.object1):
(main.let.object2):
(main):
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):
* Source/JavaScriptCore/runtime/Operations.cpp:
(JSC::jsTypeStringForValueWithConcurrency):

Originally-landed-as: 272448.708 at safari-7618-branch (b42cc4168b71). rdar://128089110
Canonical link: https://commits.webkit.org/278868@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications