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

[Tom Roeder]

================
Comment at: include/llvm/Target/TargetInstrInfo.h:431-433
@@ -430,1 +430,5 @@
 
+  /// getJumpInstrTableEntryBound - Get a number of bytes that suffices to hold
+  /// either the instruction returned by getUnconditionalBranch or the
+  /// instruction returned by getTrap. This only makes sense because
+  /// getUnconditionalBranch returns a single, specific instruction. This
----------------
nlewycky wrote:
> What if we're on an architecture where the instruction requires an alignment?
This value is used to compute the necessary alignment of the jumptable entry. So, if the instruction requires alignment, then that should be taken into account in specifying this bound. I'll add that information to this comment.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:41-42
@@ +40,4 @@
+
+#include <list>
+#include <vector>
+
----------------
nlewycky wrote:
> You don't appear to use these?
Ah, right. this is left over from before I switched to SmallVector etc.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:235
@@ +234,3 @@
+  FunctionType *WarningFunTy =
+      FunctionType::get(Type::getVoidTy(M.getContext()), WarningFunArgs, false);
+
----------------
nlewycky wrote:
> Does
>   FunctionType *WarningFunTy =
> 	​      FunctionType::get(Type::getVoidTy(M.getContext()), {CharPtrTy, CharPtrTy}, false);
> work?
> 
I tried it, and that doesn't compile; it complains about the initializer list not being convertible to ArrayRef.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:240-241
@@ +239,4 @@
+    if (!FailureFun)
+      llvm_unreachable("Could not get or insert the function specified by"
+                         " -cfi-func-name");
+  } else {
----------------
nlewycky wrote:
> I think this is a case for report_fatal_error. Unreachable will be deleted by the compiler entirely in an optimized build, it should only be used when it is logically impossible. Instead, I think this can be reached through choice of command line options.
Yes, you're right that this can be reached through command-line flags, so I'll changed it.

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:243-245
@@ +242,5 @@
+  } else {
+    // The default warning function swallows the warning and lets the call
+    // continue, since there's no generic way for it to print out this
+    // information.
+    Function *WarningFun = M.getFunction(cfi_failure_func_name);
----------------
nlewycky wrote:
> In the sanitizers, we have an option -fsanitize-recover which controls whether the "invariant violated" function will return or not. Apparently having 'noreturn' on the function declaration is a big deal for overall performance for them, probably due to the fact that it gives the register allocator less to worry about. Does this matter to you? And if so, would you use the same flag, or make it always noreturn?
I haven't experimented with the noreturn flag for the "invariant violated" function, but I think that's an interesting and important point and one that should be explored wrt performance here. Can I take that up in a future commit?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:380-384
@@ +379,7 @@
+
+    ParentNameGV = new GlobalVariable(M, ParentNameStringTy, true,
+                                      GlobalValue::PrivateLinkage, 0, ".str");
+    Constant *ParentNameStrConst =
+        ConstantDataArray::getString(M.getContext(), ParentName, true);
+    ParentNameGV->setInitializer(ParentNameStrConst);
+  }
----------------
nlewycky wrote:
> IRBuilder's CreateGlobalString/CreateGlobalStringPtr knows more tricks, like making it "unnamed_addr" so the string can be merged with duplicates. You don't need to give it an insertion point so long as you promise not to create instructions with it. Creating globals is fine.
Thanks! This lets me simplify this significantly.

================
Comment at: test/CodeGen/X86/cfi_enforcing.ll:27
@@ +26,3 @@
+}
+; XFAIL: win32
+
----------------
jfb wrote:
> Could you explain why?
I think this is an artifact of copy-paste from an earlier LTO test. I don't think this should be here.

http://reviews.llvm.org/D4167