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

[Nick Lewycky]

This looks right about ready to land to me.

================
Comment at: include/llvm/CodeGen/CommandFlags.h:256-257
@@ +255,4 @@
+// cfi-enforcing is false and no cfi-func-name is set, then a default function
+// will be generated that ignores all CFI violations. The expected signature for
+// functions called with CFI violations is
+//
----------------
So there's nothing wrong with this as is, but I'd really rather see something in the ubsan style. It turns out that including all the strings for the names of all the functions may be expensive (string data plus relocations) in terms of file size, and may be redundant with getting the same information in other ways (like debug info). In any event, this can wait for a future patch, when it's needed.

================
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
----------------
What if we're on an architecture where the instruction requires an alignment?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:41-42
@@ +40,4 @@
+
+#include <list>
+#include <vector>
+
----------------
You don't appear to use these?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:119
@@ +118,3 @@
+        CallSite CS(&I);
+        if (!((CS.isCall() || CS.isInvoke()) && isIndirectCall(CS)))
+          continue;
----------------
Just checking "CS" in bool context is equivalent to "CS.isCall() || CS.isInvoke". That makes this

  if (!(CS && isIndirectCall(CS)))

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:235
@@ +234,3 @@
+  FunctionType *WarningFunTy =
+      FunctionType::get(Type::getVoidTy(M.getContext()), WarningFunArgs, false);
+
----------------
Does
  FunctionType *WarningFunTy =
	​      FunctionType::get(Type::getVoidTy(M.getContext()), {CharPtrTy, CharPtrTy}, false);
work?


================
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 {
----------------
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.

================
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);
----------------
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?

================
Comment at: lib/CodeGen/ForwardControlFlowIntegrity.cpp:373
@@ +372,3 @@
+  std::string GVName =
+      Twine(cfi_func_name_prefix).concat(ParentFun->getName()).str();
+  GlobalVariable *ParentNameGV = M.getNamedGlobal(GVName);
----------------
"ParentFun->getName()" --> "ParentName" as declared one line above?

================
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);
+  }
----------------
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.

================
Comment at: test/CodeGen/X86/cfi_invoke.ll:35
@@ +34,1 @@
+}
\ No newline at end of file

----------------
Newline at end of file please!

http://reviews.llvm.org/D4167