[PATCH] Patch that aligns on bundle size the targets of indirect jumps, and that adds masks before indirect jumps and returns

[Mark Seaborn]

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h:16
@@ +15,3 @@
+// Define NaCl alignment as log2 of the bundle size.
+static const unsigned MIPS_NACL_BUNDLE_ALIGN = 4u;
+
----------------
This should go inside the namespace

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:27
@@ +26,3 @@
+
+static const unsigned IndirectBranchMaskReg = Mips::T6;
+
----------------
This could go inside the anon namespace, without "static"

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:77
@@ +76,3 @@
+  }
+
+};
----------------
Nit: don't put an empty line at the end of the class definition, please

================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:78
@@ +77,3 @@
+
+  // In NaCl, all indirect jump targets must be aligned on bundle size.
+  if (Subtarget->isTargetNaCl())
----------------
Maybe "aligned to"?

================
Comment at: test/MC/Mips/nacl-mask.s:16
@@ +15,3 @@
+
+# CHECK:         test1:
+
----------------
You can use CHECK-LABEL here.  It's preferred for checking labels because it results in better diagnostics if a test fails.

================
Comment at: test/MC/Mips/nacl-mask.s:12
@@ +11,3 @@
+        jr	$a0
+        nop
+        jr	$ra
----------------
If you put two nops here, you could also check for the use of bundle lock, because a third nop should be inserted automatically.

================
Comment at: test/MC/Mips/nacl-align.ll:34
@@ +33,3 @@
+
+; CHECK-ASM:        test1:
+
----------------
Can use CHECK-ASM-LABEL (see other comment)

================
Comment at: test/MC/Mips/nacl-align.ll:76
@@ +75,3 @@
+
+; Note that there are two consecutive lables - one temporary and one for
+; basic block.
----------------
Typo: "labels"

================
Comment at: test/MC/Mips/nacl-align.ll:93
@@ +92,3 @@
+
+; This test tests that NaCl functions are bundle-aligned.
+
----------------
Could you also test this by testing the assembly output rather than the object file output, please?

================
Comment at: test/MC/Mips/nacl-mask.s:4
@@ +3,3 @@
+# RUN:  | FileCheck %s
+
+# This test tests that address-masking sandboxing is added when given assembly
----------------
How about naming this file "nacl-mask-indirect-branches.s", just to make it a bit more descriptive?

================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:929
@@ +928,3 @@
+    for (unsigned I = 0; I < JT.size(); ++I) {
+      std::vector<MachineBasicBlock*> MBBs = JT[I].MBBs;
+
----------------
Should this take a reference to avoid copying?

================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:940
@@ +939,3 @@
+    MachineBasicBlock &MBB = *I;
+    if (MBB.hasAddressTaken())
+      MBB.setAlignment(MIPS_NACL_BUNDLE_ALIGN);
----------------
Nit: Does it work to use I->hasAddressTaken() here (and rename "I" to "MBB"), for brevity?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:84
@@ +83,3 @@
+
+MCELFStreamer* createMipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB,
+                                         raw_ostream &OS,
----------------
Nit: Use " *" spacing

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h:15
@@ +14,3 @@
+
+// Define NaCl alignment as log2 of the bundle size.
+static const unsigned MIPS_NACL_BUNDLE_ALIGN = 4u;
----------------
This is a little unclear -- this value is the log2 of the alignment.  (I'd say the alignment and bundle size are the same thing.  One isn't the log2 of the other.)

How about: "Log2 of the NaCl MIPS sandbox's instruction bundle size"?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:36
@@ +35,3 @@
+public:
+  MipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB, raw_ostream &OS,
+                      MCCodeEmitter *Emitter)
----------------
You didn't reply on my previous comment about this, so I'm not sure if it's necessary, or you just missed the comment?


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