Bug 1456524 - Maintain MoveResolver invariants. r=jandem, a=RyanVM
authorSean Stangl <sstangl@mozilla.com>
Thu, 17 May 2018 14:06:00 -0400
changeset 470838 c3d8e5732769
parent 470837 ab893afd788e
child 470839 b6653a9f24fc
push id9239
push userryanvm@gmail.com
push date2018-05-18 15:23 +0000
treeherdermozilla-beta@617990cee57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem, RyanVM
bugs1456524
milestone61.0
Bug 1456524 - Maintain MoveResolver invariants. r=jandem, a=RyanVM The MoveResolver is a persistent data structure that requires state to not leak between borrowers. An early-exit optimization in the case of OOM allows for state to leak, leading to impossible move list inputs, tripping assertions.
js/src/jit/MacroAssembler.h
js/src/jit/MoveResolver.cpp
js/src/jit/MoveResolver.h
js/src/jit/arm/MacroAssembler-arm.cpp
js/src/jit/mips32/MacroAssembler-mips32.cpp
js/src/jit/mips64/MacroAssembler-mips64.cpp
--- a/js/src/jit/MacroAssembler.h
+++ b/js/src/jit/MacroAssembler.h
@@ -329,16 +329,21 @@ class MacroAssembler : public MacroAssem
     explicit MacroAssembler(JSContext* cx);
 
     // wasm compilation handles its own JitContext-pushing
     struct WasmToken {};
     explicit MacroAssembler(WasmToken, TempAllocator& alloc);
 
   public:
     MoveResolver& moveResolver() {
+        // As an optimization, the MoveResolver is a persistent data structure
+        // shared between visitors in the CodeGenerator. This assertion
+        // checks that state is not leaking from visitor to visitor
+        // via an unresolved addMove().
+        MOZ_ASSERT(moveResolver_.hasNoPendingMoves());
         return moveResolver_;
     }
 
     size_t instructionsSize() const {
         return size();
     }
 
 #ifdef JS_HAS_HIDDEN_SP
--- a/js/src/jit/MoveResolver.cpp
+++ b/js/src/jit/MoveResolver.cpp
@@ -2,16 +2,17 @@
  * vim: set ts=8 sts=4 et sw=4 tw=99:
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "jit/MoveResolver.h"
 
 #include "mozilla/Attributes.h"
+#include "mozilla/ScopeExit.h"
 
 #include "jit/MacroAssembler.h"
 #include "jit/RegisterSets.h"
 
 using namespace js;
 using namespace js::jit;
 
 MoveOperand::MoveOperand(MacroAssembler& masm, const ABIArg& arg)
@@ -173,22 +174,28 @@ SplitIntoUpperHalf(const MoveOperand& mo
         return MoveOperand(upperSingle);
     }
 
     MOZ_ASSERT(move.isMemoryOrEffectiveAddress());
     return MoveOperand(move.base(), move.disp() + sizeof(float));
 }
 #endif
 
+// Resolves the pending_ list to a list in orderedMoves_.
 bool
 MoveResolver::resolve()
 {
     resetState();
     orderedMoves_.clear();
 
+    // Upon return from this function, the pending_ list must be cleared.
+    auto clearPending = mozilla::MakeScopeExit([this]() {
+        pending_.clear();
+    });
+
 #ifdef JS_CODEGEN_ARM
     // Some of ARM's double registers alias two of its single registers,
     // but the algorithm below assumes that every register can participate
     // in at most one cycle. To satisfy the algorithm, any double registers
     // that may conflict are split into their single-register halves.
     //
     // This logic is only applicable because ARM only uses registers d0-d15,
     // all of which alias s0-s31. Double registers d16-d31 are unused.
--- a/js/src/jit/MoveResolver.h
+++ b/js/src/jit/MoveResolver.h
@@ -328,16 +328,19 @@ class MoveResolver
         return orderedMoves_.length();
     }
     const MoveOp& getMove(size_t i) const {
         return orderedMoves_[i];
     }
     uint32_t numCycles() const {
         return numCycles_;
     }
+    bool hasNoPendingMoves() const {
+        return pending_.empty();
+    }
     void setAllocator(TempAllocator& alloc) {
         movePool_.setAllocator(alloc);
     }
 };
 
 } // namespace jit
 } // namespace js
 
--- a/js/src/jit/arm/MacroAssembler-arm.cpp
+++ b/js/src/jit/arm/MacroAssembler-arm.cpp
@@ -4588,17 +4588,17 @@ MacroAssembler::callWithABIPre(uint32_t*
                                              ABIStackAlignment);
     }
 
     *stackAdjust = stackForCall;
     reserveStack(stackForCall);
 
     // Position all arguments.
     {
-        enoughMemory_ = enoughMemory_ && moveResolver_.resolve();
+        enoughMemory_ &= moveResolver_.resolve();
         if (!enoughMemory_)
             return;
 
         MoveEmitter emitter(*this);
         emitter.emit(moveResolver_);
         emitter.finish();
     }
 
--- a/js/src/jit/mips32/MacroAssembler-mips32.cpp
+++ b/js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ -2186,17 +2186,17 @@ MacroAssembler::callWithABIPre(uint32_t*
 
     // Save $ra because call is going to clobber it. Restore it in
     // callWithABIPost. NOTE: This is needed for calls from SharedIC.
     // Maybe we can do this differently.
     storePtr(ra, Address(StackPointer, stackForCall - sizeof(intptr_t)));
 
     // Position all arguments.
     {
-        enoughMemory_ = enoughMemory_ && moveResolver_.resolve();
+        enoughMemory_ &= moveResolver_.resolve();
         if (!enoughMemory_)
             return;
 
         MoveEmitter emitter(*this);
         emitter.emit(moveResolver_);
         emitter.finish();
     }
 
--- a/js/src/jit/mips64/MacroAssembler-mips64.cpp
+++ b/js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ -2035,17 +2035,17 @@ MacroAssembler::callWithABIPre(uint32_t*
 
     // Save $ra because call is going to clobber it. Restore it in
     // callWithABIPost. NOTE: This is needed for calls from SharedIC.
     // Maybe we can do this differently.
     storePtr(ra, Address(StackPointer, stackForCall - sizeof(intptr_t)));
 
     // Position all arguments.
     {
-        enoughMemory_ = enoughMemory_ && moveResolver_.resolve();
+        enoughMemory_ &= moveResolver_.resolve();
         if (!enoughMemory_)
             return;
 
         MoveEmitter emitter(*this);
         emitter.emit(moveResolver_);
         emitter.finish();
     }