[PATCH] Add a no_sanitize_vptr function attribute.

[Aaron Ballman]

On Thu, Apr 16, 2015 at 6:49 PM, Oliver Chang <ochang at google.com> wrote:
> Fix comment
>
>
> http://reviews.llvm.org/D9059
>
> Files:
>   include/clang/Basic/Attr.td
>   include/clang/Basic/AttrDocs.td
>   lib/CodeGen/CGExpr.cpp
>   lib/Sema/SemaDeclAttr.cpp
>   test/CodeGen/no-sanitize-vtpr.cpp
>   test/SemaCXX/attr-no-sanitize-vptr.cpp
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/

> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -1403,6 +1403,13 @@
>    let Documentation = [NoSanitizeMemoryDocs];
>  }
>
> +// Attribute to disable UBSAN vptr checks.
> +def NoSanitizeVptr : InheritableAttr {
> +  let Spellings = [GNU<"no_sanitize_vptr">];

I agree with Richard's comments about this being extended to apply to
a list of sanitizers. I wish we would have done that for the
no_sanitize_blah attributes.

Also, why no C++-style attribute?

> +  let Subjects = SubjectList<[Function], ErrorDiag>;

Would it make sense for people to disable this on an ObjCMethodDecl as well?

> +  let Documentation = [NoSanitizeVptrDocs];
> +}
> +
>  // C/C++ Thread safety attributes (e.g. for deadlock, data race checking)
>
>  def GuardedVar : InheritableAttr {
> Index: include/clang/Basic/AttrDocs.td
> ===================================================================
> --- include/clang/Basic/AttrDocs.td
> +++ include/clang/Basic/AttrDocs.td
> @@ -958,6 +958,14 @@
>    }];
>  }
>
> +def NoSanitizeVptrDocs : Documentation {
> +  let Category = DocCatFunction;
> +  let Content = [{
> +Use ``__attribute__((no_sanitize_vptr))`` on a function declaration to
> +specify that dynamic vptr checks for should not be inserted.
> +  }];

I think some more documentation would be nice. People aren't always
familiar with what those vptr checks would look like, or why they
might want to have them removed. What's the benefit to applying this
attribute, how would you decide when it's appropriate to add it, etc.

> +}
> +
>  def DocCatTypeSafety : DocumentationCategory<"Type Safety Checking"> {
>    let Content = [{
>  Clang supports additional attributes to enable checking type safety properties
> Index: lib/CodeGen/CGExpr.cpp
> ===================================================================
> --- lib/CodeGen/CGExpr.cpp
> +++ lib/CodeGen/CGExpr.cpp
> @@ -580,7 +580,8 @@
>        (TCK == TCK_MemberAccess || TCK == TCK_MemberCall ||
>         TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference ||
>         TCK == TCK_UpcastToVirtualBase) &&
> -      RD && RD->hasDefinition() && RD->isDynamicClass()) {
> +      RD && RD->hasDefinition() && RD->isDynamicClass() &&
> +      !CurFuncDecl->hasAttr<NoSanitizeVptrAttr>()) {
>      // Compute a hash of the mangled name of the type.
>      //
>      // FIXME: This is not guaranteed to be deterministic! Move to a
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -4772,6 +4772,9 @@
>    case AttributeList::AT_NoSanitizeMemory:
>      handleSimpleAttribute<NoSanitizeMemoryAttr>(S, D, Attr);
>      break;
> +  case AttributeList::AT_NoSanitizeVptr:
> +    handleSimpleAttribute<NoSanitizeVptrAttr>(S, D, Attr);
> +    break;
>    case AttributeList::AT_GuardedBy:
>      handleGuardedByAttr(S, D, Attr);
>      break;
> Index: test/CodeGen/no-sanitize-vtpr.cpp
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/no-sanitize-vtpr.cpp
> @@ -0,0 +1,26 @@
> +// 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
> +
> +// REQUIRES: shell

Why is the shell required for this test?

> +
> +class Bar {
> +public:
> +  virtual ~Bar() {}
> +};
> +class Foo : public Bar {};
> +
> +Bar bar;
> +
> +__attribute__((no_sanitize_vptr)) void testfunc() {
> +  // CHECK: testfunc
> +  // CHECK-NOT: call void @__ubsan_handle_dynamic_type_cache_miss
> +  Foo* foo = static_cast<Foo*>(&bar); // down-casting
> +  // CHECK: ret void
> +}
> +
> +void testfunc2() {
> +  // CHECK: testfunc2
> +  // CHECK: call void @__ubsan_handle_dynamic_type_cache_miss
> +  Foo* foo = static_cast<Foo*>(&bar); // down-casting
> +  // CHECK: ret void
> +}
> Index: test/SemaCXX/attr-no-sanitize-vptr.cpp
> ===================================================================
> --- /dev/null
> +++ test/SemaCXX/attr-no-sanitize-vptr.cpp
> @@ -0,0 +1,37 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify  %s
> +
> +#define NO_SANITIZE_VPTR __attribute__((no_sanitize_vptr))
> +
> +#if !__has_attribute(no_sanitize_vptr)
> +#error "Should support no_sanitize_vptr"
> +#endif
> +
> +void noanal_fun() NO_SANITIZE_VPTR;
> +
> +void noanal_fun_args() __attribute__((no_sanitize_vptr(1))); // \
> +  // expected-error {{'no_sanitize_vptr' attribute takes no arguments}}
> +
> +int noanal_testfn(int y) NO_SANITIZE_VPTR;
> +
> +int noanal_testfn(int y) {
> +  int x NO_SANITIZE_VPTR = y; // \
> +    // expected-error {{'no_sanitize_vptr' attribute only applies to functions}}
> +  return x;
> +}
> +
> +int noanal_test_var NO_SANITIZE_VPTR; // \
> +  // expected-error {{'no_sanitize_vptr' attribute only applies to functions}}
> +
> +class NoanalFoo {
> + private:
> +  int test_field NO_SANITIZE_VPTR; // \
> +    // expected-error {{'no_sanitize_vptr' attribute only applies to functions}}
> +  void test_method() NO_SANITIZE_VPTR;
> +};
> +
> +class NO_SANITIZE_VPTR NoanalTestClass { // \
> +  // expected-error {{'no_sanitize_vptr' attribute only applies to functions}}
> +};
> +
> +void noanal_fun_params(int lvar NO_SANITIZE_VPTR); // \
> +  // expected-error {{'no_sanitize_vptr' attribute only applies to functions}}
>

~Aaron