Bug 161492

Summary: ObjectAllocationSinkingPhase::insertOSRHintsForUpdate() fails to emit updated hints in some cases
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 143073    
Bug Blocks: 160125    
Attachments:
Description Flags
the patch mark.lam: review+

Filip Pizlo
Reported 2016-09-01 10:39:37 PDT
This seems to cause trouble: a: CreateActivation() MovHint(@a, loc1) b: NewFunction(@a) MovHint(@b, loc2) c: NewFunction(@b) MovHint(@c, loc3) Escape(@b) I'm seeing code like this: a: PhantomCreateActivation() MovHint(@a, loc1) b: PhantomNewFunction(@a) PutHint(@b, @a, scope) MovHint(@b, loc2) c: PhantomNewFunction(@b) PutHint(@c, @a, scope) MovHint(@c, loc3) d: MaterializeCreateActivation() e: NewFunction(@d) MovHint(@d, loc1) MovHint(@e, loc2) Escape(@e) But this is wrong, because if we exit, then we will recreate @c pointing at a newly materialized activation, rather than at @d. In other words, the phase should have produced this code instead: a: PhantomCreateActivation() MovHint(@a, loc1) b: PhantomNewFunction(@a) PutHint(@b, @a, scope) MovHint(@b, loc2) c: PhantomNewFunction(@b) PutHint(@c, @a, scope) MovHint(@c, loc3) d: MaterializeCreateActivation() e: NewFunction(@d) PutHint(@c, @e, scope) <---- THIS MovHint(@d, loc1) MovHint(@e, loc2) Escape(@e) The phase seems to have code to do this. I'm still investigating what goes wrong.
Attachments
the patch (6.82 KB, patch)
2016-09-01 11:48 PDT, Filip Pizlo 		mark.lam: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2016-09-01 11:12:33 PDT
It looks like this is handled by m_materializationSiteToRecoveries. I wonder why the scope isn't in there.
Filip Pizlo
Comment 2 2016-09-01 11:18:10 PDT
100% repro: function bar() { } noInline(bar); function foo(p, x) { var value = 1; function inc() { return value + 1; } function dec() { return value - 1; } if (!p) return 0; bar(inc); x += 2000000000; value = 42; return dec(); } noInline(foo); function test(x) { var result = foo(true, x); if (result != 42 - 1) throw "Error: bad result: " + result; } for (var i = 0; i < 100000; ++i) test(0); test(2000000000);
Filip Pizlo
Comment 3 2016-09-01 11:42:51 PDT
Wow this is an epic one-line fix!
Filip Pizlo
Comment 4 2016-09-01 11:48:31 PDT
Created attachment 287648 [details] the patch
Mark Lam
Comment 5 2016-09-01 11:55:13 PDT
Comment on attachment 287648 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=287648&action=review r=me > Source/JavaScriptCore/ChangeLog:16 > + it's a special meta-data field initialed on construction. But just because it's immutable /initialed/initialized/ ? > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1942 > - > + Please remove.
Filip Pizlo
Comment 6 2016-09-01 11:56:17 PDT
(In reply to comment #5) > Comment on attachment 287648 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287648&action=review > > r=me > > > Source/JavaScriptCore/ChangeLog:16 > > + it's a special meta-data field initialed on construction. But just because it's immutable > > /initialed/initialized/ ? Fixed. > > > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1942 > > - > > + > > Please remove. Fixed.
Filip Pizlo
Comment 7 2016-09-01 11:58:01 PDT
Landed in http://trac.webkit.org/changeset/205304
Radar WebKit Bug Importer
Comment 8 2016-09-01 11:58:54 PDT
<rdar://problem/28120011>
Note You need to log in before you can comment on or make changes to this bug.