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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Regressed by me in bug 502890. See example in bug 503191.

I'm pretty sure this is safe. Stop me if you think otherwise!

Comment 1

[Brendan Eich [:brendan]]

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

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: 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.

Comment 3

[Jason Orendorff [:jorendorff]]

(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.

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[Jason Orendorff [:jorendorff]]

(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

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bfd1f4324a2f

Comment 7

[Bob Clary [:bc] (inactive)]

trace-test testMethodInit|testMethodSet|testMethodInitSafety