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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:84
@@ +83,3 @@
+  // Sanity check.
+  if (C) {
+    auto FM = dyn_cast_or_null<ValueAsMetadata>(MD->getOperand(1));
----------------
Early return here.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:91
@@ +90,3 @@
+      assert(!F || !F->isDeclaration());
+    }
+  }
----------------
Sure, so let's word the FIXME like that.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:111
@@ +110,3 @@
+          Type::getVoidTy(Ctx),
+          {Type::getInt64Ty(Ctx), PointerType::getUnqual(Type::getInt8Ty(Ctx))},
+          false));
----------------
I think this will break if the `ConstantInt`s are not of type i64. Can you handle this gracefully?

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:140
@@ +139,3 @@
+  SwitchInst *SI = IRB.CreateSwitch(TypeId, TrapBB, BitSetIds.size());
+  for (ConstantInt *CaseTypeId : BitSetIds) {
+    BasicBlock *TestBB = BasicBlock::Create(Ctx, "test", F);
----------------
Isn't this non-deterministic? I think it will depend on the `ConstantInt` addresses.

================
Comment at: test/Transforms/CrossDSOCFI/basic.ll:17
@@ +16,3 @@
+
+; CHECK:   call i1 @llvm.bitset.test
+; CHECK:   br {{.*}} label %[[EXIT]], label %[[TRAP]]
----------------
I think you can make these tests more specific once the non-determinism has been fixed in the code.


Repository:
  rL LLVM

http://reviews.llvm.org/D15365