459446 - Trace JSOP_POPV

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: v1

I like it when stuff works in the shell.  I can #ifdef DEBUG this junk, if desired, but I don't think it's enough code to hurt anyone.

I was hoping this would help eval() too, but the property cache is disabled in eval frames.

I claim sticking a hard-coded pointer to fp->rval in the trace is OK because the frame isn't going away.  I think only toplevel script frames and eval frames can have this opcode.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 342665 [details] [diff] [review]
v1

>+    lir->insStorei(rval_ins, INS_CONSTPTR(&fp->rval), 0);
>+    return true;

Please comment here, NB: or equivalent, about how this assumes that JSOP_POPV is only ever emitted for global code, and how such a trace will never be called from an outer tree across callDepth > 0 (meaning there is no inner fp or fp->rval).

/be

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2

I think I found a bug-- v1 could crash in the case where an embedding compiles a script and executes it multiple times.  Right?

So v2 instead uses cx->fp->rval and has a comment explaining why I think this is correct.

(If we ever do support either JSOP_EVAL or leaving the starting frame, we're *still* safe, I think, as long as we only support one of the two and we only trace loops.)

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 342874 [details] [diff] [review]
v2

Yes, your point about global shapes reminded me that I should not forget about triggering traces for global code from API calls.

/be

Comment 4

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/eaf0d0a2b292