[PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

[Peter Collingbourne via cfe-commits]

pcc added inline comments.

================
Comment at: lib/CodeGen/CGExpr.cpp:2494
@@ +2493,3 @@
+  llvm::Value *ValidVtable = nullptr;
+  if (CheckAndAppendValidVtable) {
+    llvm::Value *AllVtables = llvm::MetadataAsValue::get(
----------------
eugenis wrote:
> samsonov wrote:
> > This is really ugly. Why are you not passing it down in DynamicArgs? Is it performance penalty you don't want to pay if the check will not succeed? How large will it be?
> Yes, I want this code to be on the failing side of the check.
> This would cost about the same as the check itself, so I suspect it could double the overhead.
> 
I would just emit the call unconditionally. We don't care too much about the performance in non-trapping mode, and if it becomes a problem in practice we can see if we can have the optimizer move the call into the conditional block (which I suspect it already knows how to do).

================
Comment at: lib/CodeGen/CodeGenModule.cpp:4053
@@ +4052,3 @@
+
+  if (!CodeGenOpts.SanitizeTrap.has(SanitizerKind::CFIVCall) ||
+      !CodeGenOpts.SanitizeTrap.has(SanitizerKind::CFINVCall) ||
----------------
This conditional doesn't look right. It should be something like 

```if (sanitize.has(this) && !sanitizetrap.has(this)) ||
   (sanitize.has(that) && !sanitizetrap.has(that)) ||
...
```
But that's sufficiently ugly that I wonder if we should just do this unconditionally. It shouldn't make a difference to the generated code either way.


Repository:
  rL LLVM

http://reviews.llvm.org/D16823