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

Mark Seaborn mseaborn at chromium.org
Wed Feb 26 10:20:34 PST 2014


  Please make the commit message more informative -- it should mention NaCl.  Something like:

  "MIPS: Implement NaCl sandboxing of indirect jumps

  * Align targets of indirect jumps to instruction bundle boundaries (in MI layer).
  * Add masking instructions before indirect jumps (in MC layer)."

  Then LGTM, thanks.


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h:20
@@ +19,3 @@
+
+// This function creates ELF Streamer for Mips NaCl.
+MCELFStreamer *createMipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB,
----------------
Nit: "an MCELFStreamer"?  (Using the class name is more greppable.)

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:10
@@ +9,3 @@
+//
+// This file implements ELF Streamer for Mips NaCl.  It emits .o object files as
+// required by NaCl's SFI sandbox.  It inserts address-masking instructions
----------------
Nit: "MCELFStreamer"

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:14
@@ +13,3 @@
+// bundle size all functions and all targets of indirect branches.  It aligns
+// call instructions on bundle end, so that return address is always aligned on
+// bundle size.
----------------
Aligning call instructions isn't done yet, right?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:31
@@ +30,3 @@
+
+/// Extend the generic ELFStreamer class so that it can mask dangerous
+/// instructions.
----------------
Nit: MCELFStreamer

================
Comment at: test/MC/Mips/nacl-align.ll:52
@@ +51,3 @@
+
+; This test tests that block whose address is taken is bundle-aligned in NaCl.
+
----------------
Nit: "a block"

================
Comment at: test/MC/Mips/nacl-align.ll:98
@@ +97,3 @@
+}
+
+
----------------
Nit: remove empty lines at end of file

================
Comment at: test/MC/Mips/nacl-align.ll:88
@@ +87,3 @@
+
+; This test tests that NaCl functions are bundle-aligned.
+
----------------
You might want to put this one first, since it's the most fundamental.  Also, it would avoid ".align 4" accidentally matching something in the previous function.


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



More information about the llvm-commits mailing list