[mips] Don't emit redundant mask instructions if the mask is already present.

[sstankovic]

Summary

The situation where masks are already present can only happen for sandboxed inline assembly. Since PNaCl currently doesn't allow inline assembly, this patch is only useful for tests that contain inline assembly.

[mseaborn]

This would need test cases.

Can you explain the use case for this, please? Is this for assembly code that has already had NaCl sandboxing added to it manually?

[sstankovic]

Yes. Currently two tests fail because of the redundant sandboxing - syscall_return_sandboxing_test and exception_test. For syscall_return_sandboxing_test, to make the test pass we can either remove sandboxing from the assembly, or apply this patch. For exception test, neither removing the sandboxing nor applying the patch will work. Here is the problematic inline assembly from exception_test.c file:

crash_at_known_address:
and $zero, $zero, $t7
prog_ctr_at_crash:
sw $t0, 0($zero)

exception_test works by crashing when executing instruction labeled by "prog_ctr_at_crash", and then checking whether the saved program counter is identical to the value of "prog_ctr_at_crash". Test currently fails because sandboxing will add mask before "sw", moving the "sw" to the address prog_ctr_at_crash + 4. Removing the mask in the assembly obviously wouldn't make the test pass. But even with the patch applied this test would still fail. That's because patch is implemented so that it first saves the mask, and then emits it when emitting next instruction, surrounded by BundleLock() and BundleUnlock(). So patch would first save mask, then emit label "prog_ctr_at_crash", and then emit mask and "sw", placing "sw" at address prog_ctr_at_crash + 4.

The only way to make exception_test pass is either to add #ifdef for MIPS that would check that program counter has value prog_ctr_at_crash + 4, or find instruction that doesn't require masking and that can cause program to crash.

[sstankovic]

Is there a MIPS breakpoint/trap instruction that would work here?

"break" works - test passes when sw is replaced with break.

So I suppose all this means that this patch should be abandoned, and the tests modified.

Regards,
Sasa

lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp

 
 #define DEBUG_TYPE "mips-mc-nacl"
 
 namespace {
 
 const unsigned IndirectBranchMaskReg = Mips::T6;
 const unsigned LoadStoreStackMaskReg = Mips::T7;
 
+static MCInst EmptyMCInst;
+
 /// Extend the generic MCELFStreamer class so that it can mask dangerous
 /// instructions.
 
 class MipsNaClELFStreamer : public MipsELFStreamer {
 public:
   MipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB, raw_ostream &OS,
                       MCCodeEmitter *Emitter, const MCSubtargetInfo &STI)
-    : MipsELFStreamer(Context, TAB, OS, Emitter, STI), PendingCall(false) {}
+    : MipsELFStreamer(Context, TAB, OS, Emitter, STI), PendingCall(false),
+      HasPrevBranchMask(false), PrevBranchMask(EmptyMCInst),
+      HasPrevLoadStoreMask(false), PrevLoadStoreMask(EmptyMCInst),
+      PendingStackMask(false) {}
 
   ~MipsNaClELFStreamer() {}
 
 private:
   // Whether we started the sandboxing sequence for calls.  Calls are bundled
   // with branch delays and aligned to the bundle end.
   bool PendingCall;
 
+  bool HasPrevBranchMask;
+  MCInst &PrevBranchMask;
+  bool HasPrevLoadStoreMask;
+  MCInst &PrevLoadStoreMask;
+  bool PendingStackMask;
+
   bool isIndirectJump(const MCInst &MI) {
     if (MI.getOpcode() == Mips::JALR) {
       // MIPS32r6/MIPS64r6 doesn't have a JR instruction and uses JALR instead.
       // JALR is an indirect branch if the link register is $0.
       assert(MI.getOperand(0).isReg());
       return MI.getOperand(0).getReg() == Mips::ZERO;
     }
     return MI.getOpcode() == Mips::JR;
   }
 
   bool isStackPointerFirstOperand(const MCInst &MI) {
     return (MI.getNumOperands() > 0 && MI.getOperand(0).isReg()
             && MI.getOperand(0).getReg() == Mips::SP);
   }
 
+  bool isMask(const MCInst &MI, unsigned MaskReg) {
+    return (MI.getOpcode() == Mips::AND &&
+            MI.getOperand(0).getReg() == MI.getOperand(1).getReg() &&
+            MI.getOperand(2).isReg() && MI.getOperand(2).getReg() == MaskReg);
+  }
+
+  bool isIndirectBranchMask(const MCInst &MI) {
+    return isMask(MI, IndirectBranchMaskReg);
+  }
+
+  bool isLoadStoreMask(const MCInst &MI) {
+    return (isMask(MI, LoadStoreStackMaskReg) &&
+            baseRegNeedsLoadStoreMask(MI.getOperand(0).getReg()));
+  }
+
+  bool isStackChangeMask(const MCInst &MI) {
+    return (isMask(MI, LoadStoreStackMaskReg) &&
+            MI.getOperand(0).getReg() == Mips::SP);
+  }
+
   bool isCall(const MCInst &MI, bool *IsIndirectCall) {
     unsigned Opcode = MI.getOpcode();
 
     *IsIndirectCall = false;
 
     switch (Opcode) {
     default:
       return false;
void emitMask(unsigned AddrReg, unsigned MaskReg,
     MaskInst.addOperand(MCOperand::CreateReg(MaskReg));
     MipsELFStreamer::EmitInstruction(MaskInst, STI);
   }
 
   // Sandbox indirect branch or return instruction by inserting mask operation
   // before it.
   void sandboxIndirectJump(const MCInst &MI, const MCSubtargetInfo &STI) {
     unsigned AddrReg = MI.getOperand(0).getReg();
-
     EmitBundleLock(false);
+    if (HasPrevBranchMask)
+      MipsELFStreamer::EmitInstruction(PrevBranchMask, STI);
+    else
       emitMask(AddrReg, IndirectBranchMaskReg, STI);
     MipsELFStreamer::EmitInstruction(MI, STI);
     EmitBundleUnlock();
   }
 
   // Sandbox memory access or SP change.  Insert mask operation before and/or
   // after the instruction.
   void sandboxLoadStoreStackChange(const MCInst &MI, unsigned AddrIdx,
                                    const MCSubtargetInfo &STI, bool MaskBefore,
                                    bool MaskAfter) {
     EmitBundleLock(false);
     if (MaskBefore) {
       // Sandbox memory access.
       unsigned BaseReg = MI.getOperand(AddrIdx).getReg();
+      if (HasPrevLoadStoreMask)
+        MipsELFStreamer::EmitInstruction(PrevLoadStoreMask, STI);
+      else
         emitMask(BaseReg, LoadStoreStackMaskReg, STI);
     }
     MipsELFStreamer::EmitInstruction(MI, STI);
+
     if (MaskAfter) {
       // Sandbox SP change.
       unsigned SPReg = MI.getOperand(0).getReg();
       assert((Mips::SP == SPReg) && "Unexpected stack-pointer register.");
       emitMask(SPReg, LoadStoreStackMaskReg, STI);
     }
     EmitBundleUnlock();
   }
 
 public:
   /// This function is the one used to emit instruction data into the ELF
   /// streamer.  We override it to mask dangerous instructions.
   void EmitInstruction(const MCInst &Inst,
                        const MCSubtargetInfo &STI) override {
     // Sandbox indirect jumps.
     if (isIndirectJump(Inst)) {
       if (PendingCall)
         report_fatal_error("Dangerous instruction in branch delay slot!");
+
       sandboxIndirectJump(Inst, STI);
+      HasPrevBranchMask = false;
+      HasPrevLoadStoreMask = false;
+      PendingStackMask = false;
       return;
     }
 
     // Sandbox loads, stores and SP changes.
     unsigned AddrIdx;
     bool IsStore;
     bool IsMemAccess = isBasePlusOffsetMemoryAccess(Inst.getOpcode(), &AddrIdx,
                                                     &IsStore);
-    bool IsSPFirstOperand = isStackPointerFirstOperand(Inst);
+    bool IsSPFirstOperand = (isStackPointerFirstOperand(Inst) &&
+                             !isStackChangeMask(Inst));
     if (IsMemAccess || IsSPFirstOperand) {
       bool MaskBefore = (IsMemAccess
                          && baseRegNeedsLoadStoreMask(Inst.getOperand(AddrIdx)
                                                           .getReg()));
       bool MaskAfter = IsSPFirstOperand && !IsStore;
       if (MaskBefore || MaskAfter) {
         if (PendingCall)
           report_fatal_error("Dangerous instruction in branch delay slot!");
+
         sandboxLoadStoreStackChange(Inst, AddrIdx, STI, MaskBefore, MaskAfter);
+        HasPrevBranchMask = false;
+        HasPrevLoadStoreMask = false;
+        PendingStackMask = MaskAfter;
         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, &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();
+        if (HasPrevBranchMask)
+          MipsELFStreamer::EmitInstruction(PrevBranchMask, STI);
+        else
           emitMask(TargetReg, IndirectBranchMaskReg, STI);
       }
       MipsELFStreamer::EmitInstruction(Inst, STI);
       PendingCall = true;
+      HasPrevBranchMask = false;
+      HasPrevLoadStoreMask = false;
+      PendingStackMask = false;
       return;
     }
     if (PendingCall) {
       // Finish the sandboxing sequence by emitting branch delay.
       MipsELFStreamer::EmitInstruction(Inst, STI);
       EmitBundleUnlock();
       PendingCall = false;
       return;
     }
+    if (PendingStackMask) {
+      PendingStackMask = false;
+      if (isStackChangeMask(Inst))
+        return;
+    }
+
+    if (isIndirectBranchMask(Inst)) {
+      PrevBranchMask = Inst;
+      HasPrevBranchMask = true;
+      return;
+    }
+    if (isLoadStoreMask(Inst)) {
+      PrevLoadStoreMask = Inst;
+      HasPrevLoadStoreMask = true;
+      return;
+    }
 
     // None of the sandboxing applies, just emit the instruction.
     MipsELFStreamer::EmitInstruction(Inst, STI);
+    HasPrevBranchMask = false;
+    HasPrevLoadStoreMask = false;
+    PendingStackMask = false;
   }
 };
 
 } // end anonymous namespace
 
 namespace llvm {
 
 bool isBasePlusOffsetMemoryAccess(unsigned Opcode, unsigned *AddrIdx,