Bug 1393011 - Part 3: Fix a bug where BufferInstructionIterator advances too far. r=nbp
authorSean Stangl <sstangl@mozilla.com>
Wed, 13 Dec 2017 15:56:29 -0600
changeset 396319 d05acb5f6a7b2b40d002c145ff8111d0011ed000
parent 396318 5c155dd29e4c069cb8959021bdaf24c1d1c4805a
child 396320 92dc6b7471f6327002f194c93523e78c1861787f
push id56975
push userdluca@mozilla.com
push dateThu, 14 Dec 2017 09:59:07 +0000
treeherderautoland@16bcfaad13e1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp
bugs1393011
milestone59.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 1393011 - Part 3: Fix a bug where BufferInstructionIterator advances too far. r=nbp In the case of InstIsGuard(), the BufferInstructionIterator implementation forgot that the underlying AssemblerBufferInstIterator is based on BufferOffsets, the incrementation of which skips pools already.
js/src/jit/arm/Assembler-arm.cpp
js/src/jit/arm/Assembler-arm.h
--- a/js/src/jit/arm/Assembler-arm.cpp
+++ b/js/src/jit/arm/Assembler-arm.cpp
@@ -3284,36 +3284,31 @@ Instruction::maybeSkipAutomaticInstructi
             return this;
         return (this + 1 + ph->size())->maybeSkipAutomaticInstructions();
     }
     if (InstIsBNop(this))
         return (this + 1)->maybeSkipAutomaticInstructions();
     return this;
 }
 
-void
+Instruction*
 BufferInstructionIterator::maybeSkipAutomaticInstructions()
 {
+    const PoolHeader* ph;
     // If this is a guard, and the next instruction is a header, always work
     // around the pool. If it isn't a guard, then start looking ahead.
-
-    const PoolHeader* ph;
     if (InstIsGuard(*this, &ph)) {
         // Don't skip a natural guard.
         if (ph->isNatural())
-            return;
-        advance(sizeof(Instruction) * (1 + ph->size()));
-        maybeSkipAutomaticInstructions();
-        return;
+            return cur();
+        return next();
     }
-
-    if (InstIsBNop(cur())) {
-        next();
-        maybeSkipAutomaticInstructions();
-    }
+    if (InstIsBNop(cur()))
+        return next();
+    return cur();
 }
 
 // Cases to be handled:
 // 1) no pools or branches in sight => return this+1
 // 2) branch to next instruction => return this+2, because a nop needed to be inserted into the stream.
 // 3) this+1 is an artificial guard for a pool => return first instruction after the pool
 // 4) this+1 is a natural guard => return the branch
 // 5) this is a branch, right before a pool => return first instruction after the pool
--- a/js/src/jit/arm/Assembler-arm.h
+++ b/js/src/jit/arm/Assembler-arm.h
@@ -1132,17 +1132,16 @@ PatchJump(CodeLocationJump& jump_, CodeL
           ReprotectCode reprotect = DontReprotect);
 
 static inline void
 PatchBackedge(CodeLocationJump& jump_, CodeLocationLabel label, JitZoneGroup::BackedgeTarget target)
 {
     PatchJump(jump_, label);
 }
 
-class InstructionIterator;
 class Assembler;
 typedef js::jit::AssemblerBufferWithConstantPools<1024, 4, Instruction, Assembler> ARMBuffer;
 
 class Assembler : public AssemblerShared
 {
   public:
     // ARM conditional constants:
     enum ARMCondition {
@@ -2285,23 +2284,34 @@ class InstructionIterator
         return inst_;
     }
 
     Instruction* cur() const {
         return inst_;
     }
 };
 
+// Compile-time iterator over instructions, with a safe interface that
+// references not-necessarily-linear Instructions by linear BufferOffset.
 class BufferInstructionIterator : public ARMBuffer::AssemblerBufferInstIterator
 {
   public:
     BufferInstructionIterator(BufferOffset bo, ARMBuffer* buffer)
       : ARMBuffer::AssemblerBufferInstIterator(bo, buffer)
     {}
-    void maybeSkipAutomaticInstructions();
+
+    // Advances the buffer to the next intentionally-inserted instruction.
+    Instruction* next() {
+        advance(cur()->size());
+        maybeSkipAutomaticInstructions();
+        return cur();
+    }
+
+    // Advances the BufferOffset past any automatically-inserted instructions.
+    Instruction* maybeSkipAutomaticInstructions();
 };
 
 static const uint32_t NumIntArgRegs = 4;
 
 // There are 16 *float* registers available for arguments
 // If doubles are used, only half the number of registers are available.
 static const uint32_t NumFloatArgRegs = 16;