[PATCH] D11856: LowerBitSets: Extend pass to support functions as bitset members.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:227
@@ -223,2 +226,3 @@
 
   // Mapping from bitset mdstrings to the call sites that test them.
+  DenseMap<Metadata *, std::vector<CallInst *>> BitSetTestCallSites;
----------------
jfb wrote:
> Comment is out of date.
Updated.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:571
@@ +570,3 @@
+    Constant *CombinedGlobalIdxs[] = {ConstantInt::get(Int32Ty, 0),
+                                      ConstantInt::get(Int32Ty, I * 2)};
+    Constant *CombinedGlobalElemPtr = ConstantExpr::getGetElementPtr(
----------------
jfb wrote:
> I'm not sure I understand padding elements. Could you explain further?
I've added a little more explanation to the comments above. Regarding the purpose of the padding elements, this is already documented at http://clang.llvm.org/docs/ControlFlowIntegrityDesign.html#padding-to-powers-of-2

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:579
@@ +578,3 @@
+          GlobalAlias::create(Globals[I]->getType(), Globals[I]->getLinkage(),
+                              "", CombinedGlobalElemPtr, M);
+      GAlias->takeName(Globals[I]);
----------------
jfb wrote:
> Does this work with thread-local globals?
> I'm also a bit worried about visibility / section / linkage type, but I may be misunderstanding the implications of what this patch adds.
> Does this work with thread-local globals?

It does not. I've added a couple of checks below that ensure that no thread-local globals or globals with an explicit section can be members of bitsets.

> I'm also a bit worried about visibility / section / linkage type

Conceptually, linkage and visiblity are both properties of the symbol, rather than of its position in the object file (as thread-localness and section are), so they can be copied from the original entity to the alias.

Linkage is already copied to the alias. I'm also copying visibility now.

> I may be misunderstanding the implications of what this patch adds.

This isn't actually new code; see line 579 of the old code below. For some reason the diff doesn't match up correctly here. But thanks all the same for raising these issues :)

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:589
@@ +588,3 @@
+  if (Arch != Triple::x86 && Arch != Triple::x86_64)
+    report_fatal_error("Unsupported architecture for jump tables");
+
----------------
jfb wrote:
> Could this be a virtual function instead? `report_fatal_error` by default, and only implemented for x86. I'm assuming the creation will also need to be specialized.
It could be, but I would prefer to do this when we add support for another architecture (which is mostly blocked on an implementation of http://lists.llvm.org/pipermail/llvm-dev/2015-July/088748.html or something similar). Once something like that goes in, we should be able to simplify this code significantly as well.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:651
@@ +650,3 @@
+void LowerBitSets::buildBitSetsFromFunctions(
+    ArrayRef<Metadata *> BitSets, ArrayRef<Function *> Globals) {
+  // This simple layout is based on the regular layout of jump tables.
----------------
jfb wrote:
> Can Globals ever be empty? It seems a bit silly, but not entirely unlikely if someone just runs the pass.
It cannot. The `buildBitSetsFromGlobalVariables` function handles any empty global sets -- see line 894 of the new code. I've added a comment here to make this clear.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:666
@@ +665,3 @@
+        FunctionType::get(Type::getVoidTy(M->getContext()), /*isVarArg=*/false),
+        Function::PrivateLinkage, "", M);
+    BasicBlock *BB = BasicBlock::Create(M->getContext(), "", NewFn);
----------------
jfb wrote:
> Give it some dummy internal name starting with `__` so it's easier to figure things out. I'm assuming there's a test that'll check for that function name too.
Done, but it isn't too useful as we end up renaming all functions appearing in bitsets anyway. Not sure if there's much we can do about that without making this code even more complicated.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:695
@@ +694,3 @@
+  // references to the original functions with references to the aliases.
+  for (unsigned I = 0; I != Globals.size()-1; ++I) {
+    Constant *CombinedGlobalElemPtr = ConstantExpr::getIntToPtr(
----------------
jfb wrote:
> `clang-format`, here and a few other places.
Done

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:889
@@ +888,3 @@
+    // Order bitsets by BitSetNM index for determinism.
+    std::sort(BitSets.begin(), BitSets.end(), [&](Metadata *M1, Metadata *M2) {
+      return BitSetIdIndices[M1] < BitSetIdIndices[M2];
----------------
jfb wrote:
> IIUC duplicate indices can't occur?
Correct, clarified in the comments.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:906
@@ -739,1 +905,3 @@
+                       reinterpret_cast<Function **>(OrderedGlobals.data() +
+                                                     OrderedGlobals.size())));
   }
----------------
jfb wrote:
> Eek, the above lines make me cry. LLVM doesn't have a way to create `ArrayRef` taking into account its magical type hierarchy? It would be nice to avoid the `reinterpret_cast` here, since it relies heavily on the `GlobalObject` / `GlobalVariable` / `Function` class hierarchy layout...
> LLVM doesn't have a way to create ArrayRef taking into account its magical type hierarchy?

I'm not sure if there can be a general way to do this due to covariance/contravariance issues.

> It would be nice to avoid the reinterpret_cast here, since it relies heavily on the GlobalObject / GlobalVariable / Function class hierarchy layout...

I decided to just rebuild the array separately on each branch of this conditional. It's a little duplicative, but the code is probably clearest this way.


http://reviews.llvm.org/D11856