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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
