[PATCH] D15365: Cross-DSO control flow integrity (LLVM part)

[Peter Collingbourne via llvm-commits]

pcc added a comment.

Please also add design documentation to `docs/ControlFlowIntegrityDesign.rst`.


================
Comment at: include/llvm/Transforms/IPO/CrossDSOCFI.h:21
@@ +20,3 @@
+
+uint64_t BuildCFICallSiteTypeId(StringRef S);
+
----------------
Why is this in a header? Were you planning to add unit tests for this function?

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:1
@@ +1,2 @@
+//===-- CrossDSOCFI.cpp - Externalize this modules' CFI checks ------------===//
+//
----------------
module's

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:10
@@ +9,3 @@
+//
+// FIXME description
+//
----------------
Please do.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:48
@@ +47,3 @@
+  uint64_t id;
+  memcpy(&id, &result, sizeof(id));
+  return id;
----------------
This will yield a different result depending on the endianness of the host machine.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:63
@@ +62,3 @@
+
+  static uint64_t getCallSiteTypeId(MDString *MDS);
+  bool buildCFICheck();
----------------
This function doesn't look like it is defined anywhere.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:89
@@ +88,3 @@
+  // FIXME: handle hash collisions
+  // FIXME: insert at the end of the module to push it closer to the RO segment?
+  llvm::DenseSet<MDString *> BitSetIds;
----------------
This is already happening, no?

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:100
@@ +99,3 @@
+      auto F = dyn_cast<Function>(V->getValue());
+      // Don't emit a check if it's just a declaration.
+      if (F && F->isDeclaration())
----------------
This won't be necessary if we teach the clang frontend to not emit bitset entries for declarations when doing cross-DSO.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:104
@@ +103,3 @@
+      // This check excludes vtables for classes inside anonymous namespaces.
+      auto S = dyn_cast<MDString>(Op->getOperand(0));
+      if (!S)
----------------
This isn't exactly right; `MDNode::isDistinct()` corresponds directly to whether the bitset entry has global scope. I would check `isDistinct` here and assert that the identifier is an `MDString` (since we don't support hashing anything else).

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:598
@@ -597,1 +597,3 @@
 
+  // Control flow integrity for cross-DSO calls.
+  PM.add(createCrossDSOCFIPass());
----------------
I'd prefer if this stated what the function does (i.e. "create function that ...").

================
Comment at: test/Transforms/CrossDSOCFI/basic.ll:5
@@ +4,3 @@
+; CHECK: switch i64{{.*}} label %[[TRAP:.*]] [
+; CHECK:   i64 {{.*}}, label
+; CHECK:   i64 {{.*}}, label
----------------
Probably should check for hash values here since we know what they're going to be.

================
Comment at: test/Transforms/CrossDSOCFI/basic.ll:36-43
@@ +35,10 @@
+
+ at _ZTV1A = unnamed_addr constant [3 x i8*] [i8* null, i8* bitcast ({ i8*, i8* }* @_ZTI1A to i8*), i8* bitcast (void (%class.A*)* @_ZN1A1fEv to i8*)], align 8
+ at _ZTI1A = constant { i8*, i8* } { i8* bitcast (i8** getelementptr inbounds (i8*, i8** @_ZTVN10__cxxabiv117__class_type_infoE, i64 2) to i8*), i8* getelementptr inbounds ([3 x i8], [3 x i8]* @_ZTS1A, i32 0, i32 0) }
+ at _ZTVN10__cxxabiv117__class_type_infoE = external global i8*
+ at _ZTS1A = constant [3 x i8] c"1A\00"
+ at _ZTV1B = unnamed_addr constant [3 x i8*] [i8* null, i8* bitcast ({ i8*, i8*, i8* }* @_ZTI1B to i8*), i8* bitcast (void (%class.B*)* @_ZN1B1fEv to i8*)], align 8
+ at _ZTI1B = constant { i8*, i8*, i8* } { i8* bitcast (i8** getelementptr inbounds (i8*, i8** @_ZTVN10__cxxabiv120__si_class_type_infoE, i64 2) to i8*), i8* getelementptr inbounds ([3 x i8], [3 x i8]* @_ZTS1B, i32 0, i32 0), i8* bitcast ({ i8*, i8* }* @_ZTI1A to i8*) }
+ at _ZTVN10__cxxabiv120__si_class_type_infoE = external global i8*
+ at _ZTS1B = constant [3 x i8] c"1B\00"
+
----------------
The body of the test could be made a lot simpler. E.g. these don't need to be vtables.


Repository:
  rL LLVM

http://reviews.llvm.org/D15365