Bug 905148 - Check that a safepoint's live registers are not modified between a VM call and its OsiPoint. r=nbp
authorJan de Mooij <jdemooij@mozilla.com>
Fri, 16 Aug 2013 11:16:46 +0200
changeset 155769 57c6f4392a6e71b067392ac815c814c91db7518a
parent 155768 c51c5ea1758de1d55920db4a5cf6d7cab8e1f3f1
child 155770 6d1ad1b829744b569478908b82abdf9e0129b634
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp
bugs905148
milestone26.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
js/src/jit-test/tests/basic/testStringToNumber.js
js/src/jit-test/tests/ion/bug860838.js
js/src/jit-test/tests/jaeger/bug832670.js
js/src/jit/CodeGenerator.cpp
js/src/jit/Ion.h
js/src/jit/IonFrames.cpp
js/src/jit/IonTypes.h
js/src/jit/LIR.h
js/src/jit/LiveRangeAllocator.h
js/src/jit/Registers.h
js/src/jit/shared/CodeGenerator-shared.cpp
js/src/jit/shared/CodeGenerator-shared.h
js/src/vm/Stack.h
--- a/js/src/builtin/TestingFunctions.cpp
+++ b/js/src/builtin/TestingFunctions.cpp
@@ -888,16 +888,26 @@ static bool
 DisableSPSProfiling(JSContext *cx, unsigned argc, jsval *vp)
 {
     if (cx->runtime()->spsProfiler.installed())
         cx->runtime()->spsProfiler.enable(false);
     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)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     if (argc == 0 || !args[0].isObject() || !args[0].toObject().is<JSFunction>()) {
         RootedObject arg(cx, &args.callee());
         ReportUsageError(cx, arg, "Must have one function argument");
         return false;
     }
@@ -1131,16 +1141,21 @@ static const JSFunctionSpecWithHelp Test
 "  true, then even slower assertions are enabled for all generated JIT code.\n"
 "  When 'slow' is false, then instrumentation is enabled, but the slow\n"
 "  assertions are disabled."),
 
     JS_FN_HELP("disableSPSProfiling", DisableSPSProfiling, 1, 0,
 "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"
 "  inferred name based on where the function was defined. This can be\n"
 "  different from the 'name' property on the function."),
 
     JS_FN_HELP("isAsmJSCompilationAvailable", IsAsmJSCompilationAvailable, 0, 0,
 "isAsmJSCompilationAvailable",
--- a/js/src/jit-test/tests/basic/testStringToNumber.js
+++ b/js/src/jit-test/tests/basic/testStringToNumber.js
@@ -1,8 +1,10 @@
+enableOsiPointRegisterChecks();
+
 function convertToInt(str) {
     return str | 0;
 }
 
 function convertToIntOnTrace(str) {
     var z;
     for (var i = 0; i < 9; ++i) {
         z = str | 0;
--- a/js/src/jit-test/tests/ion/bug860838.js
+++ b/js/src/jit-test/tests/ion/bug860838.js
@@ -1,8 +1,10 @@
+enableOsiPointRegisterChecks();
+
 function DiagModule(stdlib, foreign) {
     "use asm";
 
     var sqrt = stdlib.Math.sqrt;
     var test = foreign.test;
 
     function square(x) {
         x = x|0;
--- a/js/src/jit-test/tests/jaeger/bug832670.js
+++ b/js/src/jit-test/tests/jaeger/bug832670.js
@@ -1,8 +1,9 @@
+enableOsiPointRegisterChecks();
 
 gczeal(4);
 eval("(function() { " + "\
 for ( var CHARCODE = 1024; CHARCODE < 65536; CHARCODE+= 1234 ) {\
 	unescape( '%u'+(ToUnicodeString(CHARCODE)).substring(0,3) )\
 }\
 function ToUnicodeString( n ) {\
   var string = ToHexString(n);\
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -827,16 +827,21 @@ CodeGenerator::visitOsiPoint(LOsiPoint *
         if (*iter == lir || iter->isNop())
             continue;
         JS_ASSERT(!iter->isMoveGroup());
         JS_ASSERT(iter->safepoint() == safepoint);
         break;
     }
 #endif
 
+#ifdef CHECK_OSIPOINT_REGISTERS
+    if (shouldVerifyOsiPointRegs(safepoint))
+        verifyOsiPointRegs(safepoint);
+#endif
+
     return true;
 }
 
 bool
 CodeGenerator::visitGoto(LGoto *lir)
 {
     jumpToBlock(lir->target());
     return true;
@@ -2726,16 +2731,30 @@ CodeGenerator::generateBody()
             if (counts)
                 blockCounts.ref().visitInstruction(*iter);
 
             if (iter->safepoint() && pushedArgumentSlots_.length()) {
                 if (!markArgumentSlots(iter->safepoint()))
                     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;
 
             if (!iter->accept(this))
                 return false;
         }
         if (masm.oom())
             return false;
--- a/js/src/jit/Ion.h
+++ b/js/src/jit/Ion.h
@@ -87,16 +87,24 @@ struct IonOptions
     // Default: true
     bool eaa;
 
     // Toggles whether compilation occurs off the main thread.
     //
     // 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.
     //
     // Default: 10
     uint32_t baselineUsesBeforeCompile;
 
     // How many invocations or loop iterations are needed before functions
     // are compiled.
@@ -202,16 +210,19 @@ struct IonOptions
         limitScriptSize(true),
         registerAllocator(RegisterAllocator_LSRA),
         inlining(true),
         edgeCaseAnalysis(true),
         rangeAnalysis(true),
         uce(true),
         eaa(true),
         parallelCompilation(false),
+#ifdef CHECK_OSIPOINT_REGISTERS
+        checkOsiPointRegisters(false),
+#endif
         baselineUsesBeforeCompile(10),
         usesBeforeCompile(1000),
         usesBeforeInliningFactor(.125),
         osrPcMismatchesBeforeRecompile(6000),
         frequentBailoutThreshold(10),
         exceptionBailoutThreshold(10),
         compileTryCatch(false),
         maxStackArgs(4096),
--- a/js/src/jit/IonFrames.cpp
+++ b/js/src/jit/IonFrames.cpp
@@ -1016,16 +1016,26 @@ MarkIonExitFrame(JSTracer *trc, const Io
             break;
         }
     }
 }
 
 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:
             MarkIonExitFrame(trc, frames);
             break;
           case IonFrame_BaselineJS:
             frames.baselineFrame()->trace(trc);
             break;
--- a/js/src/jit/IonTypes.h
+++ b/js/src/jit/IonTypes.h
@@ -199,14 +199,18 @@ static inline bool
 IsNullOrUndefined(MIRType type)
 {
     return type == MIRType_Null || type == MIRType_Undefined;
 }
 
 #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
 } // namespace js
 
 #endif /* jit_IonTypes_h */
--- a/js/src/jit/LIR.h
+++ b/js/src/jit/LIR.h
@@ -980,16 +980,22 @@ class LSafepoint : public TempObject
     // registers: if passed to the call, the values passed will be marked via
     // MarkIonExitFrame, and no registers can be live after the instruction
     // except its outputs.
     RegisterSet liveRegs_;
 
     // 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_;
 
     // Assembler buffer displacement to OSI point's call location.
     uint32_t osiCallPointOffset_;
 
     // List of stack slots which have gcthing pointers.
@@ -1024,16 +1030,24 @@ class LSafepoint : public TempObject
 #endif
     { }
     void addLiveRegister(AnyRegister reg) {
         liveRegs_.addUnchecked(reg);
     }
     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);
     }
     GeneralRegisterSet gcRegs() const {
         return gcRegs_;
     }
     bool addGcSlot(uint32_t slot) {
         return gcSlots_.append(slot);
--- a/js/src/jit/LiveRangeAllocator.h
+++ b/js/src/jit/LiveRangeAllocator.h
@@ -620,16 +620,21 @@ class LiveRangeAllocator : public Regist
             if (interval->end() < pos)
                 break;
 
             if (!interval->covers(pos))
                 continue;
 
             LSafepoint *safepoint = ins->safepoint();
             safepoint->addLiveRegister(a->toRegister());
+
+#ifdef CHECK_OSIPOINT_REGISTERS
+            if (reg->isTemp())
+                safepoint->addTempRegister(a->toRegister());
+#endif
         }
     }
 
     // Finds the first safepoint that is within range of an interval.
     size_t findFirstSafepoint(const LiveInterval *interval, size_t startFrom) const
     {
         size_t i = startFrom;
         for (; i < graph.numSafepoints(); i++) {
--- a/js/src/jit/Registers.h
+++ b/js/src/jit/Registers.h
@@ -80,16 +80,31 @@ struct FloatRegister {
     bool operator !=(const FloatRegister &other) const {
         return code_ != other.code_;
     }
     bool volatile_() const {
         return !!((1 << code()) & FloatRegisters::VolatileMask);
     }
 };
 
+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
 {
     mozilla::Array<uintptr_t *, Registers::Total> regs_;
     mozilla::Array<double *, FloatRegisters::Total> fpregs_;
 
   public:
     static MachineState FromBailout(uintptr_t regs[Registers::Total],
--- a/js/src/jit/shared/CodeGenerator-shared.cpp
+++ b/js/src/jit/shared/CodeGenerator-shared.cpp
@@ -417,16 +417,169 @@ CodeGeneratorShared::markOsiPoint(LOsiPo
 
     ensureOsiSpace();
 
     *callPointOffset = masm.currentOffset();
     SnapshotOffset so = ins->snapshot()->snapshotOffset();
     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
 CodeGeneratorShared::callVM(const VMFunction &fun, LInstruction *ins, const Register *dynStack)
 {
     // Different execution modes have different sets of VM functions.
     JS_ASSERT(fun.executionMode == gen->info().executionMode());
 
@@ -450,16 +603,21 @@ CodeGeneratorShared::callVM(const VMFunc
     pushedArgs_ = 0;
 #endif
 
     // Get the wrapper of the VM function.
     IonCode *wrapper = gen->ionRuntime()->getVMWrapper(fun);
     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
     // returned value, use another LIR instruction.
     uint32_t callOffset;
     if (dynStack)
         callOffset = masm.callWithExitFrame(wrapper, *dynStack);
     else
--- a/js/src/jit/shared/CodeGenerator-shared.h
+++ b/js/src/jit/shared/CodeGenerator-shared.h
@@ -195,16 +195,21 @@ class CodeGeneratorShared : public LInst
     // class.
     size_t allocateCache(const IonCache &, size_t size) {
         size_t dataOffset = allocateData(size);
         size_t index = cacheList_.length();
         masm.propagateOOM(cacheList_.append(dataOffset));
         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.
     IonCache *getCache(size_t index) {
         return reinterpret_cast<IonCache *>(&runtimeData_[cacheList_[index]]);
     }
 
   protected:
--- a/js/src/vm/Stack.h
+++ b/js/src/vm/Stack.h
@@ -1286,16 +1286,24 @@ namespace ion {
 // A JitActivation is used for frames running in Baseline or Ion.
 class JitActivation : public Activation
 {
     uint8_t *prevIonTop_;
     JSContext *prevIonJSContext_;
     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();
 
     bool isActive() const {
         return active_;
     }
     void setActive(JSContext *cx, bool active = true);
@@ -1304,16 +1312,28 @@ class JitActivation : public Activation
         return prevIonTop_;
     }
     JSCompartment *compartment() const {
         return compartment_;
     }
     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.
 class JitActivationIterator : public ActivationIterator
 {
     void settle() {
         while (!done() && !activation_->isJit())
             ActivationIterator::operator++();