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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
