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

[JF Bastien]

================
Comment at: include/llvm/CodeGen/CommandFlags.h:245
@@ +244,3 @@
+CFIFuncName("cfi-func-name", cl::desc("The name of the CFI function to call"),
+            cl::init(""));
+
----------------
Could you clarify that the above two options are linked? Or just have cfi-func-name, implying a call to abort otherwise? It kind of seems like the default cfi-func could just call abort?

Also, what's the function's expected signature?

What happens if the cfi-func itself gets CFI instrumented, and it fails the check? *That* should probably call abort directly, not recurse :)

================
Comment at: include/llvm/Target/TargetOptions.h:54
@@ +53,3 @@
+  namespace CFIntegrity {
+    enum CFIntegrityType {
+      Sub,             // Use subtraction-based checks.
----------------
Use `enum class`.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:146
@@ +145,3 @@
+    PointerType *PTy = dyn_cast<PointerType>(VTy);
+    if (!PTy)
+      continue;
----------------
When does this happen?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:151
@@ +150,3 @@
+    FunctionType *FunTy = dyn_cast<FunctionType>(EltTy);
+    if (!FunTy)
+      continue;
----------------
Same.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:158
@@ +157,3 @@
+    Constant *JumpTableMask = NULL;
+    Constant *JumpTableSize = NULL;
+
----------------
`nullptr`.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:198
@@ +197,3 @@
+    // really get this information from the Target in some way.
+    int64_t MaskValue = ((TableSize << 3) - 1) & -8;
+    Constant *JumpTableMaskValue = ConstantInt::get(Int64Ty, MaskValue);
----------------
Could you fix this?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:244
@@ +243,3 @@
+    assert(FailureFun && "Could not get or insert the function specified by"
+                         " -cfi-func-name");
+  } else {
----------------
Shouldn't this be a fatal error?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:278
@@ +277,3 @@
+  Value *Check = NULL;
+  if (CFIType == CFIntegrity::Sub) {
+    // This is the subtract, mask, and add version.
----------------
I'd use a `switch` here, so LLVM warns if a new CFIntegrity is added and unhandled.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:297
@@ +296,3 @@
+    Constant *RightShift = ConstantInt::get(Int64Ty, 3);
+    Constant *LeftShift = ConstantInt::get(Int64Ty, 64 - 3);
+
----------------
Shouldn't you get the pointer size from the Target here too?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:354
@@ +353,3 @@
+  // Get the function to call right before the instruction.
+  Function *WarningFun = NULL;
+  if (CFIFuncName.empty()) {
----------------
`nullptr`.

================
Comment at: lib/CodeGen/JumpInstrTables.cpp:264
@@ -262,3 +263,3 @@
   for (Function &F : M) {
-    if (F.hasFnAttribute(Attribute::JumpTable)) {
+    if (F.hasFnAttribute(Attribute::JumpTable) && F.hasAddressTaken(nullptr)) {
       assert(F.hasUnnamedAddr() &&
----------------
`nullptr` is already the default for this function, no need to specify it.

http://reviews.llvm.org/D4167