[PATCH] D15367: Cross-DSO control flow integrity (Clang part)

[Peter Collingbourne via cfe-commits]

pcc added inline comments.

================
Comment at: lib/CodeGen/CGClass.cpp:2564
@@ -2563,16 +2563,3 @@
 
-  SanitizerMask M;
-  switch (TCK) {
-  case CFITCK_VCall:
-    M = SanitizerKind::CFIVCall;
-    break;
-  case CFITCK_NVCall:
-    M = SanitizerKind::CFINVCall;
-    break;
-  case CFITCK_DerivedCast:
-    M = SanitizerKind::CFIDerivedCast;
-    break;
-  case CFITCK_UnrelatedCast:
-    M = SanitizerKind::CFIUnrelatedCast;
-    break;
-  }
+  if (CGM.getCodeGenOpts().SanitizeCfiCrossDso &&
+      isExternallyVisible(RD->getTypeForDecl()->getLinkage())) {
----------------
This could be an early return.

================
Comment at: lib/CodeGen/CGExpr.cpp:2560
@@ +2559,3 @@
+  llvm::MDString *MDS = dyn_cast<llvm::MDString>(MD);
+  assert(MDS);
+  llvm::Constant *TypeId =
----------------
Use cast instead of dyn_cast/assert (but see my other comment).

================
Comment at: lib/CodeGen/CGExpr.cpp:3871
@@ +3870,3 @@
+        isa<llvm::MDNode>(MD) && dyn_cast<llvm::MDNode>(MD)->isDistinct();
+    if (CGM.getCodeGenOpts().SanitizeCfiCrossDso && !hasLocalScope) {
+      EmitCfiSlowPathCheck(BitSetTest, MD, CastedCallee);
----------------
Early return.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:996
@@ +995,3 @@
+      !(isa<CXXMethodDecl>(FD) && !cast<CXXMethodDecl>(FD)->isStatic()) &&
+      !(CodeGenOpts.SanitizeCfiCrossDso && !FD->hasBody() &&
+        !(getContext().GetGVALinkageForFunction(FD) ==
----------------
This is a little hard to read and probably needs to go back into a function with early returns. Sorry, my bad. I also think the logic for `available_externally` is wrong (please add a test case).

================
Comment at: lib/CodeGen/CodeGenModule.cpp:1011
@@ +1010,3 @@
+      uint64_t TypeId =
+          BuildCFICallSiteTypeId(dyn_cast<llvm::MDString>(MD)->getString());
+      llvm::Metadata *BitsetOps2[] = {
----------------
This isn't handling function types with internal linkage.

We can probably do this more consistently (e.g. by introducing a function that takes a `Metadata` from `CreateMetadataIdentifierForType` and returns a `ConstantInt` or `nullptr` if none suitable, and using it here, in callers of `EmitCfiSlowPathCheck` and in `CreateVTableBitSetEntry`).


Repository:
  rL LLVM

http://reviews.llvm.org/D15367