Don't report the error when non-dangerous load/store is in branch delay slot

[sstankovic]

Summary

[mips] Fix a bug - Don't report the error when non-dangerous load/store is in branch delay slot.

[mseaborn]

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

171

I don't get why you've reordered this chunk of code. Surely the ordering doesn't matter, because Inst can't be both a call and a memory access on MIPS, can it?

[sstankovic]

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

171

If for the load/store or stack change instruction the condition (MaskBefore || MaskAfter) is false, then in the original ordering the code would first unnecessary check whether the instruction is call (it isn't), and then would check whether the instruction is in the branch delay slot ("if (PendingCall)"). With the ordering in the patch, the unnecessary check whether the instruction is call is avoided.

[mseaborn]

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

171

I don't get it. Surely if you have an instruction for which "IsMemAccess || IsSPFirstOperand" is true but "MaskBefore || MaskAfter" is false, then the "isCall(Inst.getOpcode(), &IsIndirectCall)" check is going to get executed regardless of the ordering of these two chunks of code? Is that what you mean?

Can you restore the original ordering to make the correctness fix clearer, please?

If I've misunderstood this, and the ordering affects compile time (or code clarity), could you do the reordering as a separate change?

[sstankovic]

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

171

What I wanted is basically this: since only the case for load/store or stack change instruction has fall-through path (the cases for jumps and calls both end with a return instruction), I wanted the code that is executed in the fall-through case to be right below it. But OK, I'll restore the original ordering in the patch. As for new ordering, the code should probably first check whether the instruction is load/store (since usually there are more loads/stores than calls or jumps in a function), then check for call and at the end check for indirect jump.

[sstankovic]

I restored the orignal ordering.

[mseaborn]

Thanks. It's a lot clearer what the change is now. LGTM.

[sstankovic]

Committed in r210470.

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

void EmitInstruction(const MCInst &Inst,
 
     // Sandbox loads, stores and SP changes.
     unsigned AddrIdx;
     bool IsStore;
     bool IsMemAccess = isBasePlusOffsetMemoryAccess(Inst.getOpcode(), &AddrIdx,
                                                     &IsStore);
     bool IsSPFirstOperand = isStackPointerFirstOperand(Inst);
     if (IsMemAccess || IsSPFirstOperand) {
-      if (PendingCall)
-        report_fatal_error("Dangerous instruction in branch delay slot!");
-
       bool MaskBefore = (IsMemAccess
                          && baseRegNeedsLoadStoreMask(Inst.getOperand(AddrIdx)
                                                           .getReg()));
       bool MaskAfter = IsSPFirstOperand && !IsStore;
-      if (MaskBefore || MaskAfter)
+      if (MaskBefore || MaskAfter) {
+        if (PendingCall)
+          report_fatal_error("Dangerous instruction in branch delay slot!");
         sandboxLoadStoreStackChange(Inst, AddrIdx, STI, MaskBefore, MaskAfter);
-      else
-        MipsELFStreamer::EmitInstruction(Inst, STI);
         return;
       }
+      // fallthrough
+    }
 
     // Sandbox calls by aligning call and branch delay to the bundle end.
     // For indirect calls, emit the mask before the call.
     bool IsIndirectCall;
     if (isCall(Inst.getOpcode(), &IsIndirectCall)) {
       if (PendingCall)
         report_fatal_error("Dangerous instruction in branch delay slot!");
 
       // Start the sandboxing sequence by emitting call.
       EmitBundleLock(true);
       if (IsIndirectCall) {
         unsigned TargetReg = Inst.getOperand(1).getReg();
         emitMask(TargetReg, IndirectBranchMaskReg, STI);
       }
       MipsELFStreamer::EmitInstruction(Inst, STI);
       PendingCall = true;
       return;
     }
     if (PendingCall) {
       // Finish the sandboxing sequence by emitting branch delay.
       MipsELFStreamer::EmitInstruction(Inst, STI);
       EmitBundleUnlock();
       PendingCall = false;
       return;
     }
 
     // None of the sandboxing applies, just emit the instruction.
     MipsELFStreamer::EmitInstruction(Inst, STI);

test/MC/Mips/nacl-mask.s

 # CHECK-NEXT:        nop
 # CHECK-NEXT:        bgezal
 # CHECK-NEXT:        addiu   $4, $zero, 4
 
 # CHECK-NEXT:        nop
 # CHECK-NEXT:        and     $25, $25, $14
 # CHECK-NEXT:        jalr    $25
 # CHECK-NEXT:        addiu   $4, $zero, 5
+
+
+
+# Test that we can put non-dangerous loads and stores in branch delay slot.
+
+  .align  4
+test6:
+  .set  noreorder
+
+        jal func1
+        sw      $4, 0($sp)
+
+        bal func2
+        lw      $5, 0($t8)
+
+        jalr $t9
+        sw      $sp, 0($sp)
+
+# CHECK-LABEL:   test6:
+
+# CHECK:             jal
+# CHECK-NEXT:        sw      $4, 0($sp)
+
+# CHECK:             bal
+# CHECK-NEXT:        lw      $5, 0($24)
+
+# CHECK:             jalr
+# CHECK-NEXT:        sw      $sp, 0($sp)