Restore ability to trace initprop/setprop with non-branded scope and function rhs

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
Regressed by me in bug 502890. See example in bug 503191.

I'm pretty sure this is safe. Stop me if you think otherwise!
We should trace object initialisers. I have a patch coming to optimize away cloned function objects, but not brand prematurely (branding on call). BTW, what is the problem tracing the implicit toString call, that valueOf is undefined?

/be
(Assignee)

Comment 2

10 years ago
Created attachment 387558 [details] [diff] [review]
v1

I don't remember why I thought this wouldn't be safe earlier. Probably I was just being paranoid since this has to be safe for the global object too. I think it is.
Attachment #387558 - Flags: review?(brendan)
Attachment #387558 - Flags: review?(brendan) → review+
Blocks: 502890
(Assignee)

Comment 3

10 years ago
(In reply to comment #1)
> We should trace object initialisers. I have a patch coming to optimize away
> cloned function objects, but not brand prematurely (branding on call).

If under the current system we're supposed to brand prematurely, then v1 might be unsafe!

> what is the problem tracing the implicit toString call, that valueOf is
> undefined?

No, the problem is illustrated by this test in the patch:

+function testMethodInitSafety() {
+    function f() { return 'fail'; }
+    function g() { return 'ok'; }
+
+    var s;
+    var arr = [f, f, f, f, g];
+    assertEq(arr.length > RUNLOOP, true);
+    for (var i = 0; i < arr.length; i++) {
+        var x = {m: arr[i]};
+        s = x.m();
+    }
+    return s;
+}

After `var x = {m: arr[i]};`, with patch v1, x will have the same shape each time through the loop.  On trace, x will *not* have a branded scope.

When we reach the point of calling the method, we must bail off trace to avoid recording f and executing it instead of g.
No longer blocks: 502890
(Assignee)

Updated

10 years ago
Blocks: 502890
(In reply to comment #3)
> (In reply to comment #1)
> > We should trace object initialisers. I have a patch coming to optimize away
> > cloned function objects, but not brand prematurely (branding on call).
> 
> If under the current system we're supposed to brand prematurely, then v1 might
> be unsafe!

We're not supposed to brand prematurely, now or later.

> When we reach the point of calling the method, we must bail off trace to avoid
> recording f and executing it instead of g.

That's bail off trace, not abort recording, right?

/be
(Assignee)

Comment 5

10 years ago
(In reply to comment #4)
> That's bail off trace, not abort recording, right?

Right. As long as we brand at record time (or guard on the exact object, not the shape, as we do for certain other cases) stuff like testMethodInitSafety is safe.

http://hg.mozilla.org/tracemonkey/rev/bfd1f4324a2f
Whiteboard: fixed-in-tracemonkey

Comment 6

10 years ago
http://hg.mozilla.org/mozilla-central/rev/bfd1f4324a2f
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 7

9 years ago
trace-test testMethodInit|testMethodSet|testMethodInitSafety
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.