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

[Mark Seaborn]

================
Comment at: test/MC/Mips/nacl-branch-delay.ll:3
@@ +2,3 @@
+; RUN:     -O3 < %s | FileCheck %s
+
+; RUN: llc -filetype=asm -mtriple=mipsel-none-nacl -relocation-model=static \
----------------
This file belongs in test/CodeGen -- it's not testing the MC layer.

I didn't notice this when you added test/MC/Mips/nacl-align.ll.  Could you do a separate change to move that to test/CodeGen, please?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:120
@@ +119,3 @@
+      sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, false);
+    else if (isStackChange(Inst))
+      sandboxLoadStoreStackChange(Inst, -1U/*not used*/, STI, false, true);
----------------
Currently this case kicks in for:
  sw $sp, 123($sp)

and will convert it to:
  sw $sp, 123($sp)
  and $sp, $sp, $15

which is a bug.  Can you fix this and add a test for it, please?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:58
@@ +57,3 @@
+
+  bool isStackChange(const MCInst &MI) {
+    return (MI.getNumOperands() > 0 && MI.getOperand(0).isReg()
----------------
This name is a bit misleading because it will return true for store instructions which only read $sp.  You're relying on the caller first checking whether the instruction is a store, which fails in one case -- see other comment.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:114
@@ +113,3 @@
+    else if (isDangerousLoad(Inst, &AddrIdx)) {
+      if (!isStackChange(Inst))
+        sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, false);
----------------
Since only one arg changes below, you could write:
  sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, isStackChange(Inst));
without a conditional.  (But see other comments.)

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:32
@@ +31,3 @@
+
+bool isDangerousLoadOpcode(unsigned Opcode, int *AddrIdx) {
+  switch (Opcode) {
----------------
Sasa Stankovic wrote:
> Mark Seaborn wrote:
> > Could this be combined with isDangerousStoreOpcode() as isDangerousMemoryAccess()?  There's not a big difference between loads and stores for sandboxing.
> There have to be separate methods for loads and stores because of the case where
> load also changes SP (or they can be combined in one method with one additional
> parameter that tells whether we check loads or stores).
I see what you mean.  However, this leads to a bug (see other comment).  How about having an isMemoryAccess() which also returns whether the instruction is a load or a store?


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