942804 - Ion-compile scripts with unaliased "let" bindings

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Andy Wingo [:wingo]]

Attachment: Ion-compile scripts with unaliased let bindings

Comment 2

[Jan de Mooij [:jandem]]

Oops, reset the assignee :)

Comment 3

[Andy Wingo [:wingo]]

Comment on attachment 8337704 [details] [diff] [review]
Ion-compile scripts with unaliased let bindings

After bug 927782, scripts with unaliased let bindings will not have ENTERBLOCK / LEAVEBLOCK opcodes emitted.  Instead the block chain can be found by looking at the PC, and is only used in a debug situation.  However there is a DEBUGLEAVEBLOCK opcode, used by the interpreter and the baseline JIT in debug mode, which invalidates any DebugScope proxy, also copying out the unaliased locals to the heap.

When we Ion compile there will be a DEBUGLEAVEBLOCK opcode but it is a no-op.  Adding it to IonBuilder makes let scopes with all unaliased values ion-compile.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8337704 [details] [diff] [review]
Ion-compile scripts with unaliased let bindings

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

Nice :)

Comment 5

[Andy Wingo [:wingo]]

One problem: when jit-tests run with --tbpl, we get an error on --ion-eager --ion-parallel-compile=off builds, in basic/expression-autopsy.js

The issue can be seen with this snippet:

function anonymous() {
var undef, o; for (let z in [1, 2]) { o = {};undef.random_prop }
}
anonymous()

Normally it raises "TypeError: undef is undefined".  However with Ion it raises "TypeError: undef.random_prop is undefined".

The reason appears to be related to bug 831120.  In the non-Ion case, FindStartPC begins with valuepc = main() + 51, and then finishes with valuepc = main() + 48.  Here is the function:

js> print (disassemble(anonymous))
flags:
loc     op
-----   --
main:
00000:  getlocal 0
00003:  pop
00004:  getlocal 1
00007:  pop
00008:  undefined
00009:  newarray 2
00013:  one
00014:  initelem_array 0
00018:  int8 2
00020:  initelem_array 1
00024:  endinit
00025:  iter 1
00027:  goto 57 (+30)
00032:  loophead
00033:  iternext
00034:  setlocal 2
00037:  pop
00038:  newobject ({})
00043:  endinit
00044:  setlocal 1
00047:  pop
00048:  getlocal 0
00051:  getprop "random_prop"
00056:  pop
00057:  loopentry 1
00059:  moreiter
00060:  ifne 32 (-28)
00065:  enditer
00066:  debugleaveblock
00067:  popn 1
00070:  retrval

However in Ion mode the FindStartPC cuts out early, because the frame is an Ion frame, and so it stays at PC 51.

Comment 6

[Andy Wingo [:wingo]]

If I elide the "if (iter.isIon()) return true" shortcut, the problem appears to be that iter.numFrameSlots() returns 6 rather than 3 for Ion versus baseline/interpreter.

Comment 7

[Andy Wingo [:wingo]]

Indeed, the snapshotreader returns 8 slots, which, minus the 2 fixed slots, gives us 6 value slots.  But I'm still a bit perplexed how Ion is working here.

Comment 8

[Andy Wingo [:wingo]]

OK, one more thing before I bail and hack the test.  For the Ion frame:

(gdb) p iter.frameSlotValue(0)
$2 = JSVAL_VOID
(gdb) p iter.frameSlotValue(1)
$3 = JSVAL_VOID
(gdb) p iter.frameSlotValue(2)
$4 = JSVAL_VOID
(gdb) p iter.frameSlotValue(3)
$5 = $jsval("0")
(gdb) p iter.frameSlotValue(4)
$6 = $jsval((JSObject *) 0x7ffff5846160 [object Iterator])
(gdb) p iter.frameSlotValue(5)
$7 = JSVAL_VOID

It seems slots 0 through 2 should somehow be discounted.  Slots 3 through 5 do correspond to the non-fixed locals.

Comment 9

[Andy Wingo [:wingo]]

A little bit more, and I'm really bailing:

(gdb) p readSlot()
$1 = {mode_ = js::jit::SnapshotReader::JS_UNDEFINED, {fpu_ = 32767, known_type_ = {type = 255, payload = {reg_ = 4294949552, stackSlot_ = 32767}}, unknown_type_ = {value = {reg_ = 32767, stackSlot_ = -17744}}, 
    value_ = 32767}}
(gdb) p readSlot()
$2 = {mode_ = js::jit::SnapshotReader::UNTYPED, {fpu_ = JSC::X86Registers::xmm0, known_type_ = {type = JSVAL_TYPE_DOUBLE, payload = {reg_ = JSC::X86Registers::edx, stackSlot_ = 0}}, unknown_type_ = {value = {
        reg_ = JSC::X86Registers::eax, stackSlot_ = 2}}, value_ = 0}}
(gdb) p readSlot()
$3 = {mode_ = js::jit::SnapshotReader::JS_UNDEFINED, {fpu_ = 32767, known_type_ = {type = 255, payload = {reg_ = 4294949552, stackSlot_ = 32767}}, unknown_type_ = {value = {reg_ = 32767, stackSlot_ = -17744}}, 
    value_ = 32767}}
(gdb) p readSlot()
$4 = {mode_ = js::jit::SnapshotReader::JS_UNDEFINED, {fpu_ = 32767, known_type_ = {type = 255, payload = {reg_ = 4294949552, stackSlot_ = 32767}}, unknown_type_ = {value = {reg_ = 32767, stackSlot_ = -17744}}, 
    value_ = 32767}}
(gdb) p readSlot()
$5 = {mode_ = js::jit::SnapshotReader::JS_UNDEFINED, {fpu_ = 32767, known_type_ = {type = 255, payload = {reg_ = 4294949552, stackSlot_ = 32767}}, unknown_type_ = {value = {reg_ = 32767, stackSlot_ = -17744}}, 
    value_ = 32767}}
(gdb) p readSlot()
$6 = {mode_ = js::jit::SnapshotReader::TYPED_STACK, {fpu_ = 32517, known_type_ = {type = JSVAL_TYPE_STRING, payload = {reg_ = JSC::X86Registers::eax, stackSlot_ = 1}}, unknown_type_ = {value = {reg_ = 32517, 
        stackSlot_ = 0}}, value_ = 32517}}
(gdb) p readSlot()
$7 = {mode_ = js::jit::SnapshotReader::TYPED_STACK, {fpu_ = 32519, known_type_ = {type = JSVAL_TYPE_OBJECT, payload = {reg_ = JSC::X86Registers::eax, stackSlot_ = 3}}, unknown_type_ = {value = {reg_ = 32519, 
        stackSlot_ = 0}}, value_ = 32519}}
(gdb) p readSlot()
$8 = {mode_ = js::jit::SnapshotReader::UNTYPED, {fpu_ = JSC::X86Registers::xmm1, known_type_ = {type = JSVAL_TYPE_INT32, payload = {reg_ = 4294967295, stackSlot_ = 0}}, unknown_type_ = {value = {
        reg_ = JSC::X86Registers::ecx, stackSlot_ = -1}}, value_ = 1}}
(gdb) p moreSlots()
$9 = false

It doesn't seem that we have the information we need.

Comment 10

[Andy Wingo [:wingo]]

Attachment: Ion-compile scripts with unaliased let bindings

Comment 11

[Andy Wingo [:wingo]]

Comment on attachment 8349371 [details] [diff] [review]
Ion-compile scripts with unaliased let bindings

Quick r? for the error message workaround, suggested by jandem on IRC.

Comment 12

[Andy Wingo [:wingo]]

Try build here: https://tbpl.mozilla.org/?tree=Try&rev=99db0728f5c8.  Green on all except debug bc not in yet.  Let's go ahead and do this.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb804b2f1e96

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb804b2f1e96

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This was backed out from mozilla29 in bug 981522.