Add a no_sanitize_vptr function attribute.

[ochang]

fix comment

[ochang]

Move check

[ochang]

Fix comment

[rsmith]

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?

include/clang/Basic/Attr.td

1406

UBSan is not usually capitalized this way.

include/clang/Basic/AttrDocs.td

965

Grammar error around "for should".

[• Quuxplusone]

test/CodeGen/no-sanitize-vtpr.cpp

1 ↗

(On Diff #23886)

Typo in the name of this test: vtpr should be vptr.

17 ↗

(On Diff #23886)

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
}

[ochang]

address some comments

[ochang]

In 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?

include/clang/Basic/Attr.td

1406

Fixed

include/clang/Basic/AttrDocs.td

965

Fixed.

test/CodeGen/no-sanitize-vtpr.cpp

1 ↗

(On Diff #23886)

Good catch, fixed.

17 ↗

(On Diff #23886)

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.

[samsonov]

In D9059#157697, @ochang wrote:

In 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?

I agree with this suggestion. It would be cool to have a single attribute like this, and later deprecate no_sanitize_address, no_sanitize_thread and no_sanitize_memory.

[ochang]

In D9059#158733, @samsonov wrote:

In D9059#157697, @ochang wrote:

In 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?

I agree with this suggestion. It would be cool to have a single attribute like this, and later deprecate no_sanitize_address, no_sanitize_thread and no_sanitize_memory.

I can put together another patch in the next few days, unless someone else (more experienced) wants to take this?

[samsonov]

+pcc who also encountered the requirement for this attribute in http://reviews.llvm.org/D6095

include/clang/Basic/Attr.td

 
 // Attribute to disable MemorySanitizer checks.
 def NoSanitizeMemory : InheritableAttr {
   let Spellings = [GNU<"no_sanitize_memory">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [NoSanitizeMemoryDocs];
 }
 
+// Attribute to disable UBSan vptr checks.
+def NoSanitizeVptr : InheritableAttr {
+  let Spellings = [GNU<"no_sanitize_vptr">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [NoSanitizeVptrDocs];
+}
+
 // C/C++ Thread safety attributes (e.g. for deadlock, data race checking)
 
 def GuardedVar : InheritableAttr {
   let Spellings = [GNU<"guarded_var">];
   let Subjects = SubjectList<[Field, SharedVar], WarnDiag,
                              "ExpectedFieldOrGlobalVar">;
   let Documentation = [Undocumented];
 }

include/clang/Basic/AttrDocs.td

 
 Use ``__attribute__((no_sanitize_memory))`` on a function declaration to
 specify that checks for uninitialized memory should not be inserted 
 (e.g. by MemorySanitizer). The function may still be instrumented by the tool
 to avoid false positives in other places.
   }];
 }
 
+def NoSanitizeVptrDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Use ``__attribute__((no_sanitize_vptr))`` on a function declaration to
+specify that dynamic vptr checks (e.g. checks for bad downcasting) should not
+be inserted.
+  }];
+}
+
 def DocCatTypeSafety : DocumentationCategory<"Type Safety Checking"> {
   let Content = [{
 Clang supports additional attributes to enable checking type safety properties
 that can't be enforced by the C type system.  Use cases include:
 
 * MPI library implementations, where these attributes enable checking that
   the buffer type matches the passed ``MPI_Datatype``;
 * for HDF5 library there is a similar use case to MPI;

lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
   //   The program has undefined behavior if:
   //    -- the [pointer or glvalue] is used to access a non-static data member
   //       or call a non-static member function
   CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
   if (SanOpts.has(SanitizerKind::Vptr) &&
       (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
     //        fingerprinting mechanism once LLVM provides one. For the time
     //        being the implementation happens to be deterministic.
     SmallString<64> MangledName;
     llvm::raw_svector_ostream Out(MangledName);
     CGM.getCXXABI().getMangleContext().mangleCXXRTTI(Ty.getUnqualifiedType(),

lib/Sema/SemaDeclAttr.cpp

case AttributeList::AT_NoThreadSafetyAnalysis:
     handleSimpleAttribute<NoThreadSafetyAnalysisAttr>(S, D, Attr);
     break;
   case AttributeList::AT_NoSanitizeThread:
     handleSimpleAttribute<NoSanitizeThreadAttr>(S, D, Attr);
     break;
   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;
   case AttributeList::AT_PtGuardedBy:
     handlePtGuardedByAttr(S, D, Attr);
     break;
   case AttributeList::AT_ExclusiveTrylockFunction:
     handleExclusiveTrylockFunctionAttr(S, D, Attr);

test/CodeGen/no-sanitize-vptr.cpp

+// 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
+
+class Bar {
+public:
+  virtual ~Bar() {}
+};
+class Foo : public Bar {};
+
+Bar bar;
+
+Foo *testfunc1(Bar *bar) {
+  // CHECK: testfunc1
+  // CHECK: call void @__ubsan_handle_dynamic_type_cache_miss
+  return static_cast<Foo*>(bar); // down-casting
+  // CHECK: ret
+}
+__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
+  // CHECK: ret
+}

test/SemaCXX/attr-no-sanitize-vptr.cpp

+// 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}}