[PATCH] D16821: Add whole-program vtable optimization feature to Clang.

[Peter Collingbourne via cfe-commits]

pcc added inline comments.

================
Comment at: lib/CodeGen/CGVTables.cpp:904-919
@@ -900,5 +903,18 @@
+
+bool CodeGenModule::IsBitSetBlacklistedRecord(const CXXRecordDecl *RD) {
+  std::string TypeName = RD->getQualifiedNameAsString();
+  auto isInBlacklist = [&](const SanitizerBlacklist &BL) {
+    if (RD->hasAttr<UuidAttr>() && BL.isBlacklistedType("attr:uuid"))
+      return true;
+
+    return BL.isBlacklistedType(TypeName);
+  };
 
-  return getContext().getSanitizerBlacklist().isBlacklistedType(
-      RD->getQualifiedNameAsString());
+  return isInBlacklist(WholeProgramVTablesBlacklist) ||
+         ((LangOpts.Sanitize.has(SanitizerKind::CFIVCall) ||
+           LangOpts.Sanitize.has(SanitizerKind::CFINVCall) ||
+           LangOpts.Sanitize.has(SanitizerKind::CFIDerivedCast) ||
+           LangOpts.Sanitize.has(SanitizerKind::CFIUnrelatedCast)) &&
+          isInBlacklist(getContext().getSanitizerBlacklist()));
 }
 
----------------
rsmith wrote:
> It looks like putting a class in a sanitizer blacklist turns off the vptr optimizations for the class and putting it in the vptr blacklist turns off CFI checks for it. Can we avoid that, perhaps by using separate bitsets for the vptr checks and CFI?
> It looks like putting a class in a sanitizer blacklist turns off the vptr optimizations for the class and putting it in the vptr blacklist turns off CFI checks for it

This is approximately what we want to do in any case. The blacklists identify classes defined outside of the linkage unit, and both CFI checks (modulo the cross-DSO feature) and devirtualization both rely on the classes being defined in the current linkage unit.

One can imagine adding classes to the blacklist that are somehow incompatible with CFI checks, but in general I'd expect that list to be pretty small (Chromium's list [1] currently has 18 entries, most of which are non-whole-program issues) so it probably isn't a huge loss for devirtualization, and we can always try to find some way to improve on that later.

Likewise, I think we could be a little smarter about using the correct blacklists in cross-DSO mode, but that can probably come later.

> Can we avoid that, perhaps by using separate bitsets for the vptr checks and CFI?

I think we could probably share bitsets for both use cases and filter for devirt/CFI at call sites. I suspect it will be important for devirt and CFI to share bitsets, as I will want to eliminate CFI checks in devirtualized calls.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/tools/cfi/blacklist.txt&q=cfi/blacklist&sq=package:chromium&type=cs&l=2

================
Comment at: lib/CodeGen/CodeGenModule.h:492
@@ -491,1 +491,3 @@
 
+  SanitizerBlacklist WholeProgramVTablesBlacklist;
+
----------------
rsmith wrote:
> Now might be a good time to rename the `SanitizerBlacklist` class to something more general (but not as part of this commit).
Ack.

================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:1605
@@ -1604,5 +1604,3 @@
 
-  if (CGF.SanOpts.has(SanitizerKind::CFIVCall))
-    CGF.EmitVTablePtrCheckForCall(MethodDecl, VTable,
-                                  CodeGenFunction::CFITCK_VCall, Loc);
+  CGF.EmitBitSetCodeForVCall(MethodDecl->getParent(), VTable, Loc);
 
----------------
rsmith wrote:
> You can be a lot more aggressive than this -- you can make an assumption about the value of the vptr from within `EmitTypeCheck` in every case where the vptr sanitizer would emit a dynamic type check. I'm not sure that doing so will allow you to deduce a lot more vptrs, but it seems like it could help in some cases.
Maybe, but that probably wouldn't help yet. The devirtualization optimization pass looks for a very specific IR pattern, and if we generate extra vtable loads for casts to assume against, we would need additional machinery to ensure the load from the cast is invariant with those from any calls (maybe Piotr's work would help here, not sure).

One thing that I'd like to do (and this would help CFI as well) is to specifically recognize cases like this:

```
struct B {
  virtual void vf();
};

struct D1 {
};

struct D2 : B {
  virtual void vf();
};

void f(D1 *d) {
  d->vf();
}
```

In this case I'd like to devirtualize the virtual call in `f()` to `B::vf()`. But because the implicit cast to `B` in `f()` removes the information that `d` cannot be of type `D2`, we cannot eliminate `D2::vf()` as a candidate target (see also `test/cfi/sibling.cpp` in the CFI test suite).

Although this could possibly be emitted and pattern matched at the IR level, it seems simpler (and would probably catch enough cases) to have Clang look through the implicit cast when IR gen'ing for the call.


http://reviews.llvm.org/D16821