[PATCH] Introduce bitset metadata format and bitset lowering pass.

[Peter Collingbourne]

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:10
@@ +9,3 @@
+//
+// This pass lowers bitset metadata and calls to the llvm.bitset.test intrinsic.
+// See http://llvm.org/docs/LangRef.html#bitsets for more information.
----------------
kcc wrote:
> are we going to handle InvokeInst?
> Check llvm/IR/CallSite.h
It isn't normally possible to invoke an intrinsic -- I think this is only possible for two specific intrinsics. In any case, this intrinsic cannot unwind.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:55
@@ +54,3 @@
+  // Mapping from bitset mdstrings to the call sites that test them.
+  DenseMap<MDString *, std::vector<CallInst *>> BitSetTestCallSites;
+
----------------
kcc wrote:
> SmallVector maybe
I don't think we can make any prediction one way or another how large these are going to get. For vptr CFI it depends on the number of virtual calls made using a particular class, which is essentially unbounded. In the absence of any evidence I'd prefer to leave this as a `std::vector`.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:106
@@ +105,3 @@
+  if (DL)
+    IntPtrTy = DL->getIntPtrType(M.getContext(), 0);
+
----------------
kcc wrote:
> I'd prefer if we report_fatal_error w/o DL, just like in AddressSanitizer.cpp
Done.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:115
@@ +114,3 @@
+
+LowerBitSets::BitSetInfo LowerBitSets::buildBitSet(
+    Module &M,
----------------
kcc wrote:
> I wonder if this thing is unit-testable separately? 
> We are not doing these things frequently, but I think we should. 
> E.g. see ASanStackFrameLayoutTest.cpp
Most likely. I think this already has enough test coverage for the moment though.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:121
@@ +120,3 @@
+
+  std::vector<uint64_t> Offsets;
+  uint64_t Min = std::numeric_limits<uint64_t>::max(), Max = 0;
----------------
kcc wrote:
> SmallVector, here and maybe in other places. 
Done. This is probably better as a SmallVector since it lives on the stack and we can probably make an educated guess on its size.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:222
@@ +221,3 @@
+  TerminatorInst *Term = SplitBlockAndInsertIfThen(OffsetInRange, CI, false);
+  IRBuilder<> ThenB(Term);
+
----------------
kcc wrote:
> Do we really want a separate BB here? 
> On valid programs, i.e. in 99.9999% cases both checks will pass, so it might be better to do this w/o branches. 
It seems better to have separate BBs for the llvm.bitset.test intrinsic at least. Without the separate BBs, we could segv on out-of-bounds addresses, and it's possible that the caller might always need the boolean result (say if it wanted to print a diagnostic message if the check fails). If we do introduce intrinsics that are required to abort the program if the address is out of bounds, we could probably do both checks in the same BB, as either way the program would abort (but it would be harder to distinguish CFI failures from other types of crashes).

http://reviews.llvm.org/D7288

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/