1760969 - BoxPolicy breaks dependency on guarding MUnbox

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect)

Description

[lukas.bernhard]

Attachment: Spidermonkey Instruction Reordering Patch

Steps to reproduce:

During fuzzing of a non-vanilla spidermonkey I encountered an assertion violation. This bug is filed as the modification to spidermonkey is probably not the root-cause but instead exposes an underlying issue affecting the vanilla engine as well.

sample.js

function main() {
  async function v1(v2) {
      const v6 = new Set();
      const v7 = v6.has(v2);
      for (let v11 = 0; v11 < 100; v11++) { const v12 = undefined; }
  }

  v1();
  v1({});
  v1(16n);
}

main();

invoked as

obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --no-threads sample.js

Triggers this assert:

Assertion failure: Unexpected BigInt, at js/src/jit/VMFunctions.cpp:2710

The issue seems to be related to the "Apply types" pass. Before applying the types, we have:

44:unbox (37:loadfixedslot (8:newcallobject))
45:hashobject (43:guardtoclass (42:unbox)) (44:unbox (37:loadfixedslot))
46:setobjecthasnonbigint (43:guardtoclass (42:unbox)) (44:unbox (37:loadfixedslot)) (45:hashobject (43:guardtoclass) (44:unbox))

after applying types, we get:

44:unbox (37:loadfixedslot (8:newcallobject))
45:hashobject (43:guardtoclass (42:unbox)) (37:loadfixedslot (8:newcallobject))
46:setobjecthasnonbigint (43:guardtoclass (42:unbox)) (37:loadfixedslot (8:newcallobject)) (45:hashobject (43:guardtoclass) (37:loadfixedslot))

The second input of hashobject no longer depends on the unbox which would trigger a bailout due to changing types. Combining this with the attached tweak to instruction reordering exposes the issue.

• MIROps.yaml defines HashObject with input: Value
• the TypePolicy becomes BoxPolicy
• calling BoxPolicy<Op>::staticAdjustInputs replaces the second operand with the result of BoxAt of the respective operand
• BoxAt sees that the operand is an MUnbox
• Instead of MBox(MUnbox(op)) the operand is replaced with the input of the fallible MUnbox, breaking the dependency of MHashObject on the guarding MUnbox.

I guess the dependency could be broken for other MIR instructions as well as the BoxPolicy is used in multiple places.
The patch (same as in bug 1760853) is necessary to expose the issue; it should apply cleanly to 1c54648c082efdeb08cf6a5e3a8187e83f7549b9

Comment 1

[Iain Ireland [:iain]]

Looking at the instruction reordering patch, this bit stands out:

+      if (ins->isGuard() && next->isEffectful()) {
+        break;
+      }

This seems to be the code that is intended to prevent us from hoisting instructions past guards / effectful instructions, but it only triggers if both are true. In general, guards do not have side effects, so this condition doesn't ever seem to trigger.

Maybe the && was supposed to be || here?

Comment 2

[lukas.bernhard]

The idea is that guards (ins) can be moved past instructions (next) which are not effectful, hence the &&. The condition is still wrong, it should have been: if (ins->isGuard() && (next->isEffectful() || next->isMovable())). Fixing this prevents the assert from triggering.

This bug is invalid as it boils down to an error in my patch.

Comment 3

[Iain Ireland [:iain]]

Oh, I misread that line as ins->isEffectful(), not next->isEffectful().

I will resolve this bug but keep it hidden for now because of the attached patch. Jan is going to resurrect an old patch to stress-test instruction reordering that he never landed; we can open this up once we're a little more confident we've flushed out any remaining issues / figured out what we want to do with bug 1760853.