[PATCH] Add a no_sanitize_vptr function attribute.

[Oliver Chang]

In http://reviews.llvm.org/D9059#157331, @rsmith wrote:

> I'm not convinced that adding one attribute per sanitizer is the right design here -- it doesn't seem to scale very well. Have you considered an attribute like
>
>   __attribute__((no_sanitize("list,of,sanitizers"))) void fn() { ... }
>   
>
> where the list is parsed as if it were specified as `-fno-sanitize=list,of,sanitizers` on the command line?


This does seem like a much better way of doing this. Should I change this in this patch?

What does everyone else think?


================
Comment at: include/clang/Basic/Attr.td:1406
@@ -1405,1 +1405,3 @@
 
+// Attribute to disable UBSAN vptr checks.
+def NoSanitizeVptr : InheritableAttr {
----------------
rsmith wrote:
> UBSan is not usually capitalized this way.
Fixed

================
Comment at: include/clang/Basic/AttrDocs.td:965
@@ +964,3 @@
+Use ``__attribute__((no_sanitize_vptr))`` on a function declaration to
+specify that dynamic vptr checks for should not be inserted.
+  }];
----------------
rsmith wrote:
> Grammar error around "for should".
Fixed.

================
Comment at: test/CodeGen/no-sanitize-vtpr.cpp:1
@@ +1,2 @@
+// Verify ubsan doesn't emit checks for functions with the no_sanitize_vptr attribute.
+// RUN: %clang_cc1 -fsanitize=vptr -emit-llvm %s -o - | FileCheck %s
----------------
Quuxplusone wrote:
> Typo in the name of this test: `vtpr` should be `vptr`.
Good catch, fixed. 

================
Comment at: test/CodeGen/no-sanitize-vtpr.cpp:17
@@ +16,3 @@
+  // CHECK-NOT: call void @__ubsan_handle_dynamic_type_cache_miss
+  Foo* foo = static_cast<Foo*>(&bar); // down-casting
+  // CHECK: ret void
----------------
Quuxplusone wrote:
> If I understand the feature correctly, the idea is that UBSan inserts runtime checks for various undefined behaviors, including the undefined behavior of down-casting `Bar*` to `Foo*` in the case that the original `Bar*` doesn't actually point to an instance of `Foo`.
> 
> However, this particular test case is statically detectable as undefined behavior, isn't it? and then on top of that, the assignment is dead and shouldn't really be generating any code at all. I don't think a proper implementation of UBSan would insert any runtime check here (and if the current implementation *does* insert a check here, it's not a proper implementation yet).
> 
> A real test case would be something like
> 
> ```
> Foo *testfunc1(Bar *bar) {
>   // CHECK: testfunc1
>   // CHECK: call void @__ubsan_handle_dynamic_type_cache_miss
>   return static_cast<Foo*>(bar); // down-casting
> }
> __attribute__((no_sanitize_vptr)) Foo *testfunc2(Bar *bar) {
>   // CHECK: testfunc2
>   // CHECK-NOT: call void @__ubsan_handle_dynamic_type_cache_miss
>   return static_cast<Foo*>(bar); // down-casting
> }
> ```
I based these tests off an existing test - ubsan-type-blacklist.cpp, which led me to assumed it was OK to assume that these checks will be inserted for those cases. Should I change that too?

I've replaced this with something similar to yours (with added CHECK: ret) since call void @__ubsan_handle_dynamic_type_cache_miss gets inserted somewhere after testfunc2 too.

http://reviews.llvm.org/D9059

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