708428 - The BytecodeEmitter should transform JSOP_GETGNAME "undefined" into JSOP_UNDEFINED

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Jägermonkey already does this, also for Infinity and NaN, but I think converting them into doubles would confuse people.

Comment 1

[Tom S [:evilpie]]

Attachment: WIP

This feels like it might have been a bit to easy. I just looked at the points where we optimize nodes into jsop_getgname, and transformed them to jsop_undefined if appropriate. I first tried to follow jsop_arguments/jsop_callee, but I think they appear in a lot more places, because they apply in more places. 

Please a bit of feedback if this is sane :O

Comment 2

[Tom S [:evilpie]]

Comment on attachment 580042 [details] [diff] [review]
WIP

Review of attachment 580042 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsopcode.cpp
@@ +3144,5 @@
>                  /* FALL THROUGH */
>  
>                case JSOP_SETRVAL:
>                  rval = POP_STR();
> +                if (*rval == '\0' || pc[-oplen]  == JSOP_UNDEFINED)

This is of course wrong, but I currently can't think of a way to reliable get the last jsop. Suggestions?

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 580042 [details] [diff] [review]
WIP

This seems fine. Another way to do it would be to wait until EmitNameOp when we are just about to emit the JSOP_GETGNAME opcode and change it there, at the last minute. (I didn't look at the code; that approach might be worse.)

>-        if (!bce->mightAliasLocals() && !TryConvertToGname(bce, pn, &op))
>+        if (!TryConvertToGname(bce, pn, &op))

I didn't look into why it's OK to remove the mightAliasLocals() check.

(In reply to Tom Schuster (evilpie) from comment #2)
> This is of course wrong, but I currently can't think of a way to reliable
> get the last jsop. Suggestions?

Not sure, but I think ss->bytecodes[top] would be JSOP_UNDEFINED just before you call POP_STR().

I would add tests for the decompiler and for more different kinds of shadowing, including 'undefined' inside of a with-block.

Style nit: The new if/else in Decompile needs curly braces.

Comment 4

[Tom S [:evilpie]]

Not really worth the required hacks.