[tsan][clang] Introduce a function attribute to disable TSan checking at run time

[zaks.anna]

Summary

This introduces a function annotation that disables TSan checking for the function at run time. The benefit over attribute((no_sanitize("thread"))) is that the accesses within the callees will also be suppressed.

The motivation for this attribute is a guarantee given by the objective C language that the calls to the reference count decrement and object deallocation will be synchronized. To model this properly, we would need to intercept all ref count decrement calls (which are very common in ObjC due to use of ARC) and also every single message send. Instead, we propose to just ignore all accesses made from within dealloc at run time. The main downside is that this still does not introduce any synchronization, which means we might still report false positives if the code that relies on this synchronization is not executed from within dealloc. However, we have not seen this in practice so far and think these cases will be very rare.

(This problem is similar in nature to https://reviews.llvm.org/D21609; unfortunately, the same solution does not apply here.)

This is used by https://reviews.llvm.org/D25858.

[kubamracek]

Does this also disable checking for the compiler generated .cxx_destruct Obj-C methods? Can we add a testcase both here and into compiler-rt?

[dvyukov]

lib/CodeGen/CodeGenFunction.cpp

717

This is unpleasant. We had recursive ignores in the old Valgrind-based tsan, but managed to not reintroduce them in the new tsan so far. But I also don't see any better solution.

721

Can we do this check right in the tsan pass? Or this information is already lost there?

722

Also do:

Fn->removeFnAttr(llvm::Attribute::SanitizeThread);

That's what will effectively happen. And it will allow to simplify the other patch.

[zaks.anna]

Thanks for the review! I'll submit the updated patches soon.

[zaks.anna]

Addressed the review comments.

I also added ObjC +initialize method to the list because TSan does not observe the guaranteed synchronization between +initialize and initial object accesses.

[dvyukov]

test/CodeGen/sanitize-thread-no-checking-at-run-time.m

1 ↗

(On Diff #76643)

Are you sure this is the right location for the test?
test/CodeGen does not seem to contain any tests, only subdirs.

35 ↗

(On Diff #76643)

Does this check actually work?
I would expect that sanitize_thread, if present, will be eaten by the previous line.
Not sure what's the best way to fix it. What is the exact list of attributes on the previous line? Maybe we can just specify them all without using {{.*}}?

[zaks.anna]

test/CodeGen/sanitize-thread-no-checking-at-run-time.m

1 ↗

(On Diff #76643)

You are looking at the llvm/test/CodeGen instead of the clang/test/CodeGen. The latter contains individual test files and I think it's the right place for the test.

35 ↗

(On Diff #76643)

Listing all attributes won't be low maintenance as more attributes could be added. Here is the list we have now:

attributes #0 = { nounwind sanitize_thread "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }

I could check that there are double quotes after nounwind:
TSAN: attributes [[ATTR]] = { nounwind "{{.*}} "sanitize_thread_no_checking_at_run_time" {{.*}} }

Alternatively, I could drop this aspect of the test altogether since I have an assert in the llvm pass.

[dvyukov]

Alternatively, I could drop this aspect of the test altogether since I have an assert in the llvm pass.

Dropping the check is fine in this case.

lib/CodeGen/CodeGenFunction.cpp

if (SanOpts.hasOneOf(SanitizerKind::Address | SanitizerKind::KernelAddress))
     Fn->addFnAttr(llvm::Attribute::SanitizeAddress);
   if (SanOpts.has(SanitizerKind::Thread))
     Fn->addFnAttr(llvm::Attribute::SanitizeThread);
   if (SanOpts.has(SanitizerKind::Memory))
     Fn->addFnAttr(llvm::Attribute::SanitizeMemory);
   if (SanOpts.has(SanitizerKind::SafeStack))
     Fn->addFnAttr(llvm::Attribute::SafeStack);
 
+  // Ignore TSan memory acesses from within dealloc and all of its calees at
+  // run time.
+  if (SanOpts.has(SanitizerKind::Thread))
+    if (const auto *M = dyn_cast_or_null<ObjCMethodDecl>(D))
+      if (M->getMethodFamily() == OMF_dealloc)
+        Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
+
   // Apply xray attributes to the function (as a string, for now)
   if (D && ShouldXRayInstrumentFunction()) {
     if (const auto *XRayAttr = D->getAttr<XRayInstrumentAttr>()) {
       if (XRayAttr->alwaysXRayInstrument())
         Fn->addFnAttr("function-instrument", "xray-always");
       if (XRayAttr->neverXRayInstrument())
         Fn->addFnAttr("function-instrument", "xray-never");
     } else {

test/CodeGen/sanitize-thread-attr-dealloc.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+__attribute__((objc_root_class))
+@interface NSObject
+- (void)dealloc;
+@end
+
+@interface MyObject : NSObject
+- (void) dealloc;
+@end
+
+@implementation MyObject
+- (void)dealloc {
+  [super dealloc];
+}
+@end
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
+
+// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: attributes [[ATTR]] = { nounwind sanitize_thread {{.*}} "sanitize_thread_no_checking_at_run_time" {{.*}} }
+