[PATCH] Patch that adds address-masking instructions before loads and stores, and after the instructions that change stack-pointer.

[Mark Seaborn]

  As with the previous change, can you make the commit message more informative by explaining the context about NaCl sandboxing?


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:13
@@ +12,3 @@
+// before dangerous control-flow and memory access instructions.  It inserts
+// address-masking instructions after the instructions that change stack
+// pointer.  It aligns on bundle size all functions and all targets of indirect
----------------
Nit: "after instructions that change the stack pointer"

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:32
@@ +31,3 @@
+
+bool isDangerousLoadOpcode(unsigned Opcode, int *AddrIdx) {
+  switch (Opcode) {
----------------
Could this be combined with isDangerousStoreOpcode() as isDangerousMemoryAccess()?  There's not a big difference between loads and stores for sandboxing.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:184
@@ -69,1 +183,3 @@
   virtual void EmitInstruction(const MCInst &Inst, const MCSubtargetInfo &STI) {
+    int AddrIdx;
+
----------------
Nit: this should probably be unsigned

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:192
@@ +191,3 @@
+      else
+        sandboxStackChangeLoad(Inst, AddrIdx, STI);
+    } else if (isDangerousStore(Inst, &AddrIdx))
----------------
Having a separate function for that seems a bit repetitive.  How about:

bool SandboxAddr = isDangerousMemAccess(Inst, &AddrIdx);
bool SandboxStack = isStackChange(Inst);
if (SandboxAddr || SandboxStack) {
  EmitBundleLock(false);
  if (SandboxAddr) // emit masking...
  MCELFStreamer::EmitInstruction(MI, STI);
  if (SandboxStack) // emit masking...
  EmitBundleUnlock();
} else {
  MCELFStreamer::EmitInstruction(MI, STI);
}

And omitting the EmitBundleLock() in the normal case is just on the hunch that it might be slow.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:206
@@ -80,1 +205,3 @@
 
+bool isForbiddenInDelaySlot(const MachineInstr &MI,
+                            const TargetRegisterInfo *TRI) {
----------------
I don't think you want to be referring to MachineInstr from a file in the MC layer, because MachineInstr is in a higher layer.

This search currently gives no matches:
git ls-files | grep MCTargetDesc | xargs grep MachineInstr

isDangerousMemAccess() isn't very large, so I think it would be OK to duplicate it in MipsDelaySlotFiller.cpp, and export isDangerousMemAccessOpcode() from the MC layer.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:549
@@ -547,1 +548,3 @@
 
+    // In NaCl, instructions that could be masked are forbidden in delay slots.
+    if (Triple(TM.getTargetTriple()).isOSNaCl()
----------------
Do you mean "instructions that must be masked"?

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:550
@@ +549,3 @@
+    // In NaCl, instructions that could be masked are forbidden in delay slots.
+    if (Triple(TM.getTargetTriple()).isOSNaCl()
+        && isForbiddenInDelaySlot(*I, TM.getRegisterInfo()))
----------------
I think this Triple() constructor parses the string, so, for performance, you might not want to be doing this inside a loop.

Can Subtarget->isTargetNaCl() be got in this context?

================
Comment at: test/MC/Mips/nacl-align.ll:2
@@ -1,3 +1,3 @@
 ; RUN: llc -filetype=asm -mtriple=mipsel-none-nacl -relocation-model=static \
-; RUN:     -O3 < %s
+; RUN:     -O3 < %s | FileCheck %s
 
----------------
If you're fixing a mistake in a previous commit, really that should be commented in the commit message.  But I would prefer if you fixed this in a standalone change first.


http://llvm-reviews.chandlerc.com/D2904