[PATCH] Add Forward-Edge Control-Flow Integrity support

[JF Bastien]

================
Comment at: include/llvm/Analysis/JumpInstrTableInfo.h:41
@@ -40,2 +40,3 @@
   JumpInstrTableInfo();
+  JumpInstrTableInfo(uint64_t Align);
   virtual ~JumpInstrTableInfo();
----------------
I'm not a fan of default constructor parameters, but it seems like the right thing here.

The power-of-two invariant should also be specified here. Also, you should name the variable ByteAlign (it's pretty obvious it's not BitAlign, but I like explicit), as well as all the other Align.

================
Comment at: include/llvm/Analysis/JumpInstrTableInfo.h:61
@@ +60,3 @@
+
+  /// A power-of-two alignment of a jumptable entry.
+  uint64_t Alignment;
----------------
This invariant isn't actually enforced in the code, instead the code tries to round up on use. I think it would be better to enforce the invariant on construction.

================
Comment at: include/llvm/Analysis/JumpInstrTableInfo.h:66
@@ -58,1 +65,3 @@
+/// Creates a JumpInstrTableInfo pass with the given bound on entry size.
+ModulePass *createJumpInstrTableInfoPass(unsigned Bound);
 }
----------------
It's not clear what Bound is, it would be good to summarize TargetInstrInfo's explanation, or refer to it.

================
Comment at: include/llvm/CodeGen/ForwardControlFlowIntegrity.h:82
@@ +81,3 @@
+  /// CFIEnforcing is false. There is a default function that ignores
+  /// violations.
+  std::string CFIFuncName;
----------------
What's the default function that ignores violations?

================
Comment at: include/llvm/CodeGen/ForwardControlFlowIntegrity.h:86
@@ +85,3 @@
+  /// The alignment of each entry in the table, from JumpInstrTableInfo.
+  uint64_t Alignment;
+
----------------
Power-of-2 restriction?

================
Comment at: include/llvm/Target/TargetInstrInfo.h:342
@@ +341,3 @@
+  /// either the instruction returned by getUnconditionalBranch or the
+  /// instruction returned by getTrap. This only makes sense because
+  /// getUnconditionalBranch returns a single, specific instruction. This
----------------
The maximum of uncond branch and trap?

================
Comment at: include/llvm/Target/TargetOptions.h:243
@@ +242,3 @@
+    /// the program to halt.
+    bool CFIEnforcing;
+
----------------
It looks like someone thought size was important above, since bitfields are used for bools. I'm not sure whether you should follow that lead (it would require reshuffling members around).

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:4364
@@ -4363,2 +4363,3 @@
 
+// This must be kept in sync with getJumpInstrTableEntryBound.
 void ARMBaseInstrInfo::getUnconditionalBranch(
----------------
This comment (and the others below and in other files) isn't clear: what has to be kept in sync exactly?

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:4391
@@ +4390,3 @@
+  // At worst, this will be a branch to a 32-bit value. So, that's one byte for
+  // the branch instruction, and 4 bytes for the value. The trap instruction
+  // fits in 4 bytes, so this suffices as a bound.
----------------
Thumb instructions are 2 or 4 bytes, and ARM instructions are always 4. The largest immediate on a direct branch has 26 bits, so that's insufficient for a lot of cases, you'll therefore need a PC-relative load followed by an indirect branch, and a location to store the 32-bit constant pool entry for the address. Assuming you do:
  ldr rX, [PC, +#8]
  bx rX
  0xdeadbeef ; The address
Then you need 12 bytes, which I think is the worst case. Note that the constant pool entry could be elsewhere, preferably *not* in executable memory, but that would require another register and more infrastructure.

Note that you may want to use blx instead, if you intend on balancing the CPU's call/return stack: blx would be for calls, see the following page for returns
  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0438i/BABGEAEF.html

http://reviews.llvm.org/D4167