Bug 905148 - Check that a safepoint's live registers are not modified between a VM call and its OsiPoint. r=nbp

js/src/builtin/TestingFunctions.cpp

@@ -892,6 +892,16 @@ DisableSPSProfiling(JSContext *cx, unsigned argc, jsval *vp)
     return true;
 }
 
+static bool
+EnableOsiPointRegisterChecks(JSContext *, unsigned, jsval *vp)
+{
+#ifdef CHECK_OSIPOINT_REGISTERS
+    ion::js_IonOptions.checkOsiPointRegisters = true;
+#endif
+    JS_SET_RVAL(cx, vp, JSVAL_VOID);
+    return true;
+}
+
 static bool
 DisplayName(JSContext *cx, unsigned argc, jsval *vp)
 {
@@ -1136,6 +1146,11 @@ static const JSFunctionSpecWithHelp TestingFunctions[] = {
 "disableSPSProfiling()",
 "  Disables SPS instrumentation"),
 
+    JS_FN_HELP("enableOsiPointRegisterChecks", EnableOsiPointRegisterChecks, 0, 0,
+"enableOsiPointRegisterChecks()",
+"Emit extra code to verify live regs at the start of a VM call are not\n"
+"modified before its OsiPoint."),
+
     JS_FN_HELP("displayName", DisplayName, 1, 0,
 "displayName(fn)",
 "  Gets the display name for a function, which can possibly be a guessed or\n"

js/src/jit-test/tests/basic/testStringToNumber.js

@@ -1,3 +1,5 @@
+enableOsiPointRegisterChecks();
+
 function convertToInt(str) {
     return str | 0;
 }

js/src/jit-test/tests/ion/bug860838.js

@@ -1,3 +1,5 @@
+enableOsiPointRegisterChecks();
+
 function DiagModule(stdlib, foreign) {
     "use asm";
 

js/src/jit-test/tests/jaeger/bug832670.js

@@ -1,3 +1,4 @@
+enableOsiPointRegisterChecks();
 
 gczeal(4);
 eval("(function() { " + "\

js/src/jit/CodeGenerator.cpp

@@ -832,6 +832,11 @@ CodeGenerator::visitOsiPoint(LOsiPoint *lir)
     }
 #endif
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+    if (shouldVerifyOsiPointRegs(safepoint))
+        verifyOsiPointRegs(safepoint);
+#endif
+
     return true;
 }
 
@@ -2731,6 +2736,20 @@ CodeGenerator::generateBody()
                     return false;
             }
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+            if (iter->safepoint() && shouldVerifyOsiPointRegs(iter->safepoint())) {
+                // Set checkRegs to 0. If we perform a VM call, the instruction
+                // will set it to 1.
+                GeneralRegisterSet allRegs(GeneralRegisterSet::All());
+                Register scratch = allRegs.takeAny();
+                masm.push(scratch);
+                masm.loadJitActivation(scratch);
+                Address checkRegs(scratch, JitActivation::offsetOfCheckRegs());
+                masm.store32(Imm32(0), checkRegs);
+                masm.pop(scratch);
+            }
+#endif
+
             if (!callTraceLIR(i, *iter))
                 return false;
 

js/src/jit/Ion.h

@@ -92,6 +92,14 @@ struct IonOptions
     // Default: true iff there are at least two CPUs available
     bool parallelCompilation;
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+    // Emit extra code to verify live regs at the start of a VM call
+    // are not modified before its OsiPoint.
+    //
+    // Default: false
+    bool checkOsiPointRegisters;
+#endif
+
     // How many invocations or loop iterations are needed before functions
     // are compiled with the baseline compiler.
     //
@@ -207,6 +215,9 @@ struct IonOptions
         uce(true),
         eaa(true),
         parallelCompilation(false),
+#ifdef CHECK_OSIPOINT_REGISTERS
+        checkOsiPointRegisters(false),
+#endif
         baselineUsesBeforeCompile(10),
         usesBeforeCompile(1000),
         usesBeforeInliningFactor(.125),

js/src/jit/IonFrames.cpp

@@ -1021,6 +1021,16 @@ MarkIonExitFrame(JSTracer *trc, const IonFrameIterator &frame)
 static void
 MarkJitActivation(JSTracer *trc, const JitActivationIterator &activations)
 {
+#ifdef CHECK_OSIPOINT_REGISTERS
+    if (js_IonOptions.checkOsiPointRegisters) {
+        // GC can modify spilled registers, breaking our register checks.
+        // To handle this, we disable these checks for the current VM call
+        // when a GC happens.
+        JitActivation *activation = activations.activation()->asJit();
+        activation->setCheckRegs(false);
+    }
+#endif
+
     for (IonFrameIterator frames(activations); !frames.done(); ++frames) {
         switch (frames.type()) {
           case IonFrame_Exit:

js/src/jit/IonTypes.h

@@ -204,6 +204,10 @@ IsNullOrUndefined(MIRType type)
 #ifdef DEBUG
 // Track the pipeline of opcodes which has produced a snapshot.
 #define TRACK_SNAPSHOTS 1
+
+// Make sure registers are not modified between an instruction and
+// its OsiPoint.
+#define CHECK_OSIPOINT_REGISTERS 1
 #endif
 
 } // namespace ion

js/src/jit/LIR.h

@@ -985,6 +985,12 @@ class LSafepoint : public TempObject
     // The subset of liveRegs which contains gcthing pointers.
     GeneralRegisterSet gcRegs_;
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+    // Temp regs of the current instruction. This set is never written to the
+    // safepoint; it's only used by assertions during compilation.
+    RegisterSet tempRegs_;
+#endif
+
     // Offset to a position in the safepoint stream, or
     // INVALID_SAFEPOINT_OFFSET.
     uint32_t safepointOffset_;
@@ -1029,6 +1035,14 @@ class LSafepoint : public TempObject
     const RegisterSet &liveRegs() const {
         return liveRegs_;
     }
+#ifdef CHECK_OSIPOINT_REGISTERS
+    void addTempRegister(AnyRegister reg) {
+        tempRegs_.addUnchecked(reg);
+    }
+    const RegisterSet &tempRegs() const {
+        return tempRegs_;
+    }
+#endif
     void addGcRegister(Register reg) {
         gcRegs_.addUnchecked(reg);
     }

js/src/jit/LiveRangeAllocator.h

@@ -625,6 +625,11 @@ class LiveRangeAllocator : public RegisterAllocator
 
             LSafepoint *safepoint = ins->safepoint();
             safepoint->addLiveRegister(a->toRegister());
+
+#ifdef CHECK_OSIPOINT_REGISTERS
+            if (reg->isTemp())
+                safepoint->addTempRegister(a->toRegister());
+#endif
         }
     }
 

js/src/jit/Registers.h

@@ -85,6 +85,21 @@ struct FloatRegister {
     }
 };
 
+class RegisterDump
+{
+  protected: // Silence Clang warning.
+    uintptr_t regs_[Registers::Total];
+    double fpregs_[FloatRegisters::Total];
+
+  public:
+    static size_t offsetOfRegister(Register reg) {
+        return offsetof(RegisterDump, regs_) + reg.code() * sizeof(uintptr_t);
+    }
+    static size_t offsetOfRegister(FloatRegister reg) {
+        return offsetof(RegisterDump, fpregs_) + reg.code() * sizeof(double);
+    }
+};
+
 // Information needed to recover machine register state.
 class MachineState
 {

js/src/jit/shared/CodeGenerator-shared.cpp

@@ -422,6 +422,159 @@ CodeGeneratorShared::markOsiPoint(LOsiPoint *ins, uint32_t *callPointOffset)
     return osiIndices_.append(OsiIndex(*callPointOffset, so));
 }
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+template <class Op>
+static void
+HandleRegisterDump(Op op, MacroAssembler &masm, RegisterSet liveRegs, Register activation,
+                   Register scratch)
+{
+    const size_t baseOffset = JitActivation::offsetOfRegs();
+
+    // Handle live GPRs.
+    for (GeneralRegisterIterator iter(liveRegs.gprs()); iter.more(); iter++) {
+        Register reg = *iter;
+        Address dump(activation, baseOffset + RegisterDump::offsetOfRegister(reg));
+
+        if (reg == activation) {
+            // To use the original value of the activation register (that's
+            // now on top of the stack), we need the scratch register.
+            masm.push(scratch);
+            masm.loadPtr(Address(StackPointer, sizeof(uintptr_t)), scratch);
+            op(scratch, dump);
+            masm.pop(scratch);
+        } else {
+            op(reg, dump);
+        }
+    }
+
+    // Handle live FPRs.
+    for (FloatRegisterIterator iter(liveRegs.fpus()); iter.more(); iter++) {
+        FloatRegister reg = *iter;
+        Address dump(activation, baseOffset + RegisterDump::offsetOfRegister(reg));
+        op(reg, dump);
+    }
+}
+
+class StoreOp
+{
+    MacroAssembler &masm;
+
+  public:
+    StoreOp(MacroAssembler &masm)
+      : masm(masm)
+    {}
+
+    void operator()(Register reg, Address dump) {
+        masm.storePtr(reg, dump);
+    }
+    void operator()(FloatRegister reg, Address dump) {
+        masm.storeDouble(reg, dump);
+    }
+};
+
+static void
+StoreAllLiveRegs(MacroAssembler &masm, RegisterSet liveRegs)
+{
+    // Store a copy of all live registers before performing the call.
+    // When we reach the OsiPoint, we can use this to check nothing
+    // modified them in the meantime.
+
+    // Load pointer to the JitActivation in a scratch register.
+    GeneralRegisterSet allRegs(GeneralRegisterSet::All());
+    Register scratch = allRegs.takeAny();
+    masm.push(scratch);
+    masm.loadJitActivation(scratch);
+
+    Address checkRegs(scratch, JitActivation::offsetOfCheckRegs());
+    masm.store32(Imm32(1), checkRegs);
+
+    StoreOp op(masm);
+    HandleRegisterDump<StoreOp>(op, masm, liveRegs, scratch, allRegs.getAny());
+
+    masm.pop(scratch);
+}
+
+class VerifyOp
+{
+    MacroAssembler &masm;
+    Label *failure_;
+
+  public:
+    VerifyOp(MacroAssembler &masm, Label *failure)
+      : masm(masm), failure_(failure)
+    {}
+
+    void operator()(Register reg, Address dump) {
+        masm.branchPtr(Assembler::NotEqual, dump, reg, failure_);
+    }
+    void operator()(FloatRegister reg, Address dump) {
+        masm.loadDouble(dump, ScratchFloatReg);
+        masm.branchDouble(Assembler::DoubleNotEqual, ScratchFloatReg, reg, failure_);
+    }
+};
+
+static void
+OsiPointRegisterCheckFailed()
+{
+    // Any live register captured by a safepoint (other than temp registers)
+    // must remain unchanged between the call and the OsiPoint instruction.
+    MOZ_ASSUME_UNREACHABLE("Modified registers between VM call and OsiPoint");
+}
+
+void
+CodeGeneratorShared::verifyOsiPointRegs(LSafepoint *safepoint)
+{
+    // Ensure the live registers stored by callVM did not change between
+    // the call and this OsiPoint. Try-catch relies on this invariant.
+
+    // Load pointer to the JitActivation in a scratch register.
+    GeneralRegisterSet allRegs(GeneralRegisterSet::All());
+    Register scratch = allRegs.takeAny();
+    masm.push(scratch);
+    masm.loadJitActivation(scratch);
+
+    // If we should not check registers (because the instruction did not call
+    // into the VM, or a GC happened), we're done.
+    Label failure, done;
+    Address checkRegs(scratch, JitActivation::offsetOfCheckRegs());
+    masm.branch32(Assembler::Equal, checkRegs, Imm32(0), &done);
+
+    // Ignore temp registers. Some instructions (like LValueToInt32) modify
+    // temps after calling into the VM. This is fine because no other
+    // instructions (including this OsiPoint) will depend on them.
+    RegisterSet liveRegs = safepoint->liveRegs();
+    liveRegs = RegisterSet::Intersect(liveRegs, RegisterSet::Not(safepoint->tempRegs()));
+
+    VerifyOp op(masm, &failure);
+    HandleRegisterDump<VerifyOp>(op, masm, liveRegs, scratch, allRegs.getAny());
+
+    masm.jump(&done);
+
+    masm.bind(&failure);
+    masm.setupUnalignedABICall(0, scratch);
+    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, OsiPointRegisterCheckFailed));
+    masm.breakpoint();
+
+    masm.bind(&done);
+    masm.pop(scratch);
+}
+
+bool
+CodeGeneratorShared::shouldVerifyOsiPointRegs(LSafepoint *safepoint)
+{
+    if (!js_IonOptions.checkOsiPointRegisters)
+        return false;
+
+    if (gen->info().executionMode() != SequentialExecution)
+        return false;
+
+    if (safepoint->liveRegs().empty(true) && safepoint->liveRegs().empty(false))
+        return false; // No registers to check.
+
+    return true;
+}
+#endif
+
 // Before doing any call to Cpp, you should ensure that volatile
 // registers are evicted by the register allocator.
 bool
@@ -455,6 +608,11 @@ CodeGeneratorShared::callVM(const VMFunction &fun, LInstruction *ins, const Regi
     if (!wrapper)
         return false;
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+    if (shouldVerifyOsiPointRegs(ins->safepoint()))
+        StoreAllLiveRegs(masm, ins->safepoint()->liveRegs());
+#endif
+
     // Call the wrapper function.  The wrapper is in charge to unwind the stack
     // when returning from the call.  Failures are handled with exceptions based
     // on the return value of the C functions.  To guard the outcome of the

js/src/jit/shared/CodeGenerator-shared.h

@@ -200,6 +200,11 @@ class CodeGeneratorShared : public LInstructionVisitor
         return index;
     }
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+    bool shouldVerifyOsiPointRegs(LSafepoint *safepoint);
+    void verifyOsiPointRegs(LSafepoint *safepoint);
+#endif
+
   public:
     // This is needed by addCache to update the cache with the jump
     // informations provided by the out-of-line path.

js/src/vm/Stack.h

@@ -1291,6 +1291,14 @@ class JitActivation : public Activation
     bool firstFrameIsConstructing_;
     bool active_;
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+  protected:
+    // Used to verify that live registers don't change between a VM call and
+    // the OsiPoint that follows it. Protected to silence Clang warning.
+    uint32_t checkRegs_;
+    RegisterDump regs_;
+#endif
+
   public:
     JitActivation(JSContext *cx, bool firstFrameIsConstructing, bool active = true);
     ~JitActivation();
@@ -1309,6 +1317,18 @@ class JitActivation : public Activation
     bool firstFrameIsConstructing() const {
         return firstFrameIsConstructing_;
     }
+
+#ifdef CHECK_OSIPOINT_REGISTERS
+    void setCheckRegs(bool check) {
+        checkRegs_ = check;
+    }
+    static size_t offsetOfCheckRegs() {
+        return offsetof(JitActivation, checkRegs_);
+    }
+    static size_t offsetOfRegs() {
+        return offsetof(JitActivation, regs_);
+    }
+#endif
 };
 
 // A filtering of the ActivationIterator to only stop at JitActivations.