add __attribute__ no_uninitialized_checks. Map no_uninitialized_checks/no_thread_safety_analysis to LLVM function attributes.

[kcc]

Summary

• Adding attribute((no_uninitialized_checks))
• Mapping attribute((no_uninitialized_checks)) to llvm::Attribute::UninitializedChecks (under MSAN)
• Mapping attribute((no_thread_safety_analysis)) llvm::Attribute::ThreadSafety (under TSAN)
• Updating Docs & Tests

[gribozavr]

lib/Sema/SemaDeclAttr.cpp

655–656

Can we promote this to an error? There's no existing code that (mis)uses this attribute, so there should be no compatibility concerns.

Please also add tests for this warning or error, and for checkAttributeNumArgs() condition above.

[kcc]

lib/Sema/SemaDeclAttr.cpp

655–656

I copy-pasted the code from the pre-existing code for no_thread_safety_analysis
and I did not fund such tests for that one.
I guess we need tests for all 3 (+ no_address_safety_analysis).
Where is the best place for them? Do you know similar existing tests?

Yes, I guess we can make it an error. Will do the change tomorrow.

[gribozavr]

lib/Sema/SemaDeclAttr.cpp

655–656

There are tests for no_address_safety_analysis in test/SemaCXX/warn-thread-safety-parsing.cpp (lines 93-121).

For this attribute, tests should probably go into test/SemaCXX/attr-no-uninitialized-checks.cpp (because we don't do static analysis, it is attr- instead of warn-).

[kcc]

Promoted warnings to errors for no_uninitialized_checks and no_address_safety_analysis and added tests

[gribozavr]

test/SemaCXX/attr-no-address-safety-analysis.cpp

20

Extra semicolon here.

test/SemaCXX/attr-no-uninitialized-checks.cpp

20

Extra semicolon here.

[kcc]

removed redundant ';'

[chandlerc]

Feel free to submit as-is to get bots green and make progress. See comments below -- we should discuss those separately. I'll try to send a cfe-dev email when I can?

docs/LanguageExtensions.rst

1609

I actually find it really unfortunate that this attribute and the -Wthread-safety escape hatch attribute are the same. I suspect there are many scenarios where users can't accomodate the reduced programming model checked by -Wthread-safety, use this attribute to escape that analysis, and would still like TSan to point out the actual races.

Thoughts?

docs/MemorySanitizer.rst

83

I think we should aim for more consistency across the attribute names. I think that will benefit users more than anything else...

Is there a reason not to tie these to the 'sanitize' naming scheme?

[kcc]

Feel free to submit as-is to get bots green and make progress. See comments below -- we should discuss those separately. I'll >> try to send a cfe-dev email when I can?

I'd better commit the final thing. Bots are not a problem, I'll disable the only failing test for now.

I actually find it really unfortunate that this attribute and the -Wthread-safety escape hatch attribute are the same. I suspect there >> are many scenarios where users can't accomodate the reduced programming model checked by -Wthread-safety, use this ?
attribute to escape that analysis, and would still like TSan to point out the actual races.
Thoughts?

There are pros and cons here.
If we add yet another attribute to disable tsan we will have two.
Users will be confused. They will be adding one instead of another.

But yes, we will lose finer granularity.
I slightly prefer to to have one attribute here rather than two, but I am open to other suggestions.

I think we should aim for more consistency across the attribute names. I think that will benefit users more than anything else...
Is there a reason not to tie these to the 'sanitize' naming scheme?

Will we have to rename the existing no_address_safety_analysis then?
That's not trivial given that gcc already uses it too.
Also, the idea behind these attributes was that we don't tie them to particular tools.

[kcc]

Any other thoughts, suggestions?

[Eugene.Zelenko]

Looks like patch was not committed.

docs/LanguageExtensions.rst

 
 Use ``__has_feature(address_sanitizer)`` to check if the code is being built
 with :doc:`AddressSanitizer`.
 
 Use ``__attribute__((no_address_safety_analysis))`` on a function declaration
 to specify that address safety instrumentation (e.g. AddressSanitizer) should
 not be applied to that function.
 
+.. _langext-thread_sanitizer:
+
+ThreadSanitizer
+----------------
+
+Use ``__has_feature(thread_sanitizer)`` to check if the code is being built
+with :doc:`ThreadSanitizer`.
+
+Use ``__attribute__((no_thread_safety_analysis))`` on a function declaration
+to specify that checks for data races on plain (non-atomic) memory accesses
+should not be inserted by ThreadSanitizer.
+The function may still be instrumented by the tool
+to avoid false positives in other places.
+
+.. _langext-memory_sanitizer:
+
+MemorySanitizer
+----------------
+Use ``__has_feature(memory_sanitizer)`` to check if the code is being built
+with :doc:`MemorySanitizer`.
+
+Use ``__attribute__((no_uninitialized_checks))`` 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.
+
+
 Thread-Safety Annotation Checking
 =================================
 
 Clang supports additional attributes for checking basic locking policies in
 multithreaded programs.  Clang currently parses the following list of
 attributes, although **the implementation for these annotations is currently in
 development.** For more details, see the `GCC implementation
 <http://gcc.gnu.org/wiki/ThreadSafetyAnnotation>`_.
 
 ``no_thread_safety_analysis``
 -----------------------------
 
 Use ``__attribute__((no_thread_safety_analysis))`` on a function declaration to
 specify that the thread safety analysis should not be run on that function.
 This attribute provides an escape hatch (e.g. for situations when it is
 difficult to annotate the locking policy).
+This attribute is also used by :ref:`ThreadSanitizer <langext-thread_sanitizer>`.
 
 ``lockable``
 ------------
 
 Use ``__attribute__((lockable))`` on a class definition to specify that it has
 a lockable type (e.g. a Mutex class).  This annotation is primarily used to
 check consistency.
 

docs/MemorySanitizer.rst

 .. code-block:: c
 
     #if defined(__has_feature)
     #  if __has_feature(memory_sanitizer)
     // code that builds only under MemorySanitizer
     #  endif
     #endif
 
+``__attribute__((no_uninitialized_checks))``
+-----------------------------------------------
+
+Some code should not be checked by MemorySanitizer.
+One may use the function attribute
+:ref:`no_uninitialized_checks <langext-memory_sanitizer>`
+to disable uninitialized checks in a particular function.
+MemorySanitizer may still instrument such functions to avoid false positives.
+This attribute may not be
+supported by other compilers, so we suggest to use it together with
+``__has_feature(memory_sanitizer)``. Note: currently, this attribute will be
+lost if the function is inlined.
+
 Origin Tracking
 ===============
 
 MemorySanitizer can track origins of unitialized values, similar to
 Valgrind's --track-origins option. This feature is enabled by
 ``-fsanitize-memory-track-origins`` Clang option. With the code from
 the example above,
 

docs/ThreadSanitizer.rst

 
 In some cases one may need to execute different code depending on whether
 ThreadSanitizer is enabled.
 :ref:`\_\_has\_feature <langext-__has_feature-__has_extension>` can be used for
 this purpose.
 
 .. code-block:: c
 
-    #if defined(__has_feature) && __has_feature(thread_sanitizer)
+    #if defined(__has_feature)
+    #  if __has_feature(thread_sanitizer)
     // code that builds only under ThreadSanitizer
-    #endif
+    #  endif
+    #endif
+
+``__attribute__((no_thread_safety_analysis))``
+-----------------------------------------------
+
+Some code should not be instrumented by ThreadSanitizer.
+One may use the function attribute
+:ref:`no_thread_safety_analysis <langext-thread_sanitizer>`
+to disable instrumentation of plain (non-atomic) loads/stores in a particular function.
+ThreadSanitizer may still instrument such functions to avoid false positives.
+This attribute may not be
+supported by other compilers, so we suggest to use it together with
+``__has_feature(thread_sanitizer)``. Note: currently, this attribute will be
+lost if the function is inlined.
 
 Limitations
 -----------
 
 * ThreadSanitizer uses more real memory than a native run. At the default
   settings the memory overhead is 5x plus 1Mb per each thread. Settings with 3x
   (less accurate analysis) and 9x (more accurate analysis) overhead are also
   available.

include/clang/Basic/Attr.td

def X86ForceAlignArgPointer : InheritableAttr {
   let Spellings = [];
 }
 
 // AddressSafety attribute (e.g. for AddressSanitizer)
 def NoAddressSafetyAnalysis : InheritableAttr {
   let Spellings = [GNU<"no_address_safety_analysis">];
 }
 
+// UninitializedChecks attribute (e.g. for MemorySanitizer)
+def NoUninitializedChecks : InheritableAttr {
+  let Spellings = [GNU<"no_uninitialized_checks">];
+}
+
 // C/C++ Thread safety attributes (e.g. for deadlock, data race checking)
 
 def GuardedVar : InheritableAttr {
   let Spellings = [GNU<"guarded_var">];
 }
 
 def PtGuardedVar : InheritableAttr {
   let Spellings = [GNU<"pt_guarded_var">];
 }
 
 def Lockable : InheritableAttr {
   let Spellings = [GNU<"lockable">];
 }
 
 def ScopedLockable : InheritableAttr {
   let Spellings = [GNU<"scoped_lockable">];
 }
 
+// ThreadSafety attribute (e.g. for ThreadSanitizer or static analysis)
 def NoThreadSafetyAnalysis : InheritableAttr {
   let Spellings = [GNU<"no_thread_safety_analysis">];
 }
 
 def GuardedBy : InheritableAttr {
   let Spellings = [GNU<"guarded_by">];
   let Args = [ExprArgument<"Arg">];
   let LateParsed = 1;

lib/CodeGen/CGDeclCXX.cpp

if (const char *Section =
       Fn->setSection(Section);
   }
 
   if (!CGM.getLangOpts().Exceptions)
     Fn->setDoesNotThrow();
 
   if (CGM.getSanOpts().Address)
     Fn->addFnAttr(llvm::Attribute::AddressSafety);
+  if (CGM.getSanOpts().Thread)
+    Fn->addFnAttr(llvm::Attribute::ThreadSafety);
+  if (CGM.getSanOpts().Memory)
+    Fn->addFnAttr(llvm::Attribute::UninitializedChecks);
 
   return Fn;
 }
 
 void
 CodeGenModule::EmitCXXGlobalVarDeclInitFunc(const VarDecl *D,
                                             llvm::GlobalVariable *Addr,
                                             bool PerformInit) {

lib/CodeGen/CodeGenModule.cpp

if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D))
     if (MD->isVirtual())
       F->setUnnamedAddr(true);
 
   if (LangOpts.getStackProtector() == LangOptions::SSPOn)
     F->addFnAttr(llvm::Attribute::StackProtect);
   else if (LangOpts.getStackProtector() == LangOptions::SSPReq)
     F->addFnAttr(llvm::Attribute::StackProtectReq);
 
-  if (SanOpts.Address) {
-    // When AddressSanitizer is enabled, set AddressSafety attribute
-    // unless __attribute__((no_address_safety_analysis)) is used.
-    if (!D->hasAttr<NoAddressSafetyAnalysisAttr>())
-      F->addFnAttr(llvm::Attribute::AddressSafety);
-  }
+  if (SanOpts.Address && !D->hasAttr<NoAddressSafetyAnalysisAttr>())
+  // Same for ThreadSanitizer and __attribute__((no_thread_safety_analysis))
+  if (SanOpts.Thread && !D->hasAttr<NoThreadSafetyAnalysisAttr>())
+    F->addFnAttr(llvm::Attribute::ThreadSafety);
+  // Same for MemorySanitizer and __attribute__((no_uninitialized_checks))
+  if (SanOpts.Memory && !D->hasAttr<NoUninitializedChecksAttr>())
+    F->addFnAttr(llvm::Attribute::UninitializedChecks);
 
   unsigned alignment = D->getMaxAlignment() / Context.getCharWidth();
   if (alignment)
     F->setAlignment(alignment);
 
   // C++ ABI requires 2-byte alignment for member functions.
   if (F->getAlignment() < 2 && isa<CXXMethodDecl>(D))
     F->setAlignment(2);

lib/Sema/SemaDeclAttr.cpp

 static void handleNoAddressSafetyAttr(Sema &S, Decl *D,
                                       const AttributeList &Attr) {
   assert(!Attr.isInvalid());
 
   if (!checkAttributeNumArgs(S, Attr, 0))
     return;
 
   if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
-    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
   }
 
   D->addAttr(::new (S.Context)
              NoAddressSafetyAnalysisAttr(Attr.getRange(), S.Context,
                                          Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleNoUninitializedChecksAttr(Sema &S, Decl *D,
+                                            const AttributeList &Attr) {
+  assert(!Attr.isInvalid());
+
+  if (!checkAttributeNumArgs(S, Attr, 0))
+    return;
+
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_decl_type)
+      << Attr.getName() << ExpectedFunctionOrMethod;
+    return;
+  }
+
+  D->addAttr(::new (S.Context) NoUninitializedChecksAttr(Attr.getRange(),
+                                                         S.Context));
+}
+
 static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D,
                                         const AttributeList &Attr,
                                         SmallVector<Expr*, 1> &Args) {
   assert(!Attr.isInvalid());
 
   if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return false;
 
case AttributeList::AT_ScopedLockable:
     handleScopedLockableAttr(S, D, Attr);
     break;
   case AttributeList::AT_NoAddressSafetyAnalysis:
     handleNoAddressSafetyAttr(S, D, Attr);
     break;
   case AttributeList::AT_NoThreadSafetyAnalysis:
     handleNoThreadSafetyAttr(S, D, Attr);
     break;
+  case AttributeList::AT_NoUninitializedChecks:
+    handleNoUninitializedChecksAttr(S, D, Attr);
+    break;
   case AttributeList::AT_Lockable:
     handleLockableAttr(S, D, Attr);
     break;
   case AttributeList::AT_GuardedBy:
     handleGuardedByAttr(S, D, Attr);
     break;
   case AttributeList::AT_PtGuardedBy:
     handlePtGuardedByAttr(S, D, Attr);

test/CodeGen/thread-safety-attr.cpp

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix TSAN %s
+// RUN: echo "src:%s" > %t
+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=thread -fsanitize-blacklist=%t | FileCheck %s
+
+// REQUIRES: shell
+
+// The thread_safety attribute should be attached to functions
+// when ThreadSanitizer is enabled, unless no_thread_safety_analysis attribute
+// is present.
+
+// CHECK-NOT:  NoThreadSafety1{{.*}} thread_safety
+// TSAN-NOT:  NoThreadSafety1{{.*}} thread_safety
+__attribute__((no_thread_safety_analysis))
+int NoThreadSafety1(int *a) { return *a; }
+
+// CHECK-NOT:  NoThreadSafety2{{.*}} thread_safety
+// TSAN-NOT:  NoThreadSafety2{{.*}} thread_safety
+__attribute__((no_thread_safety_analysis))
+int NoThreadSafety2(int *a);
+int NoThreadSafety2(int *a) { return *a; }
+
+// CHECK-NOT:  ThreadSafetyOk{{.*}} thread_safety
+// TSAN: ThreadSafetyOk{{.*}} thread_safety
+int ThreadSafetyOk(int *a) { return *a; }
+
+// CHECK-NOT:  TemplateNoThreadSafety{{.*}} thread_safety
+// TSAN-NOT: TemplateNoThreadSafety{{.*}} thread_safety
+template<int i>
+__attribute__((no_thread_safety_analysis))
+int TemplateNoThreadSafety() { return i; }
+
+// CHECK-NOT:  TemplateThreadSafetyOk{{.*}} thread_safety
+// TSAN: TemplateThreadSafetyOk{{.*}} thread_safety
+template<int i>
+int TemplateThreadSafetyOk() { return i; }
+
+int force_instance = TemplateThreadSafetyOk<42>()
+                   + TemplateNoThreadSafety<42>();
+
+// Check that __cxx_global_var_init* get the thread_safety attribute.
+int global1 = 0;
+int global2 = *(int*)((char*)&global1+1);
+// CHECK-NOT: @__cxx_global_var_init{{.*}}thread_safety
+// TSAN: @__cxx_global_var_init{{.*}}thread_safety

test/CodeGen/uninitialized-checks-attr.cpp

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=memory | FileCheck -check-prefix MSAN %s
+// RUN: echo "src:%s" > %t
+// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=memory -fsanitize-blacklist=%t | FileCheck %s
+
+// REQUIRES: shell
+
+// The uninitialized_checks attribute should be attached to functions
+// when MemorySanitizer is enabled, unless no_uninitialized_checks attribute
+// is present.
+
+// CHECK-NOT:  NoUninitializedChecks1{{.*}} uninitialized_checks
+// MSAN-NOT:  NoUninitializedChecks1{{.*}} uninitialized_checks
+__attribute__((no_uninitialized_checks))
+int NoUninitializedChecks1(int *a) { return *a; }
+
+// CHECK-NOT:  NoUninitializedChecks2{{.*}} uninitialized_checks
+// MSAN-NOT:  NoUninitializedChecks2{{.*}} uninitialized_checks
+__attribute__((no_uninitialized_checks))
+int NoUninitializedChecks2(int *a);
+int NoUninitializedChecks2(int *a) { return *a; }
+
+// CHECK-NOT:  UninitializedChecksOk{{.*}} uninitialized_checks
+// MSAN: UninitializedChecksOk{{.*}} uninitialized_checks
+int UninitializedChecksOk(int *a) { return *a; }
+
+// CHECK-NOT:  TemplateNoUninitializedChecks{{.*}} uninitialized_checks
+// MSAN-NOT: TemplateNoUninitializedChecks{{.*}} uninitialized_checks
+template<int i>
+__attribute__((no_uninitialized_checks))
+int TemplateNoUninitializedChecks() { return i; }
+
+// CHECK-NOT:  TemplateUninitializedChecksOk{{.*}} uninitialized_checks
+// MSAN: TemplateUninitializedChecksOk{{.*}} uninitialized_checks
+template<int i>
+int TemplateUninitializedChecksOk() { return i; }
+
+int force_instance = TemplateUninitializedChecksOk<42>()
+                   + TemplateNoUninitializedChecks<42>();
+
+// Check that __cxx_global_var_init* get the uninitialized_checks attribute.
+int global1 = 0;
+int global2 = *(int*)((char*)&global1+1);
+// CHECK-NOT: @__cxx_global_var_init{{.*}}uninitialized_checks
+// MSAN: @__cxx_global_var_init{{.*}}uninitialized_checks

test/SemaCXX/attr-no-address-safety-analysis.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify  %s
+
+#define NO_ADDRESS_SAFETY_ANALYSIS __attribute__((no_address_safety_analysis))
+#if !__has_attribute(no_address_safety_analysis)
+#error "Should support no_address_safety_analysis"
+#endif
+
+void noanal_fun() NO_ADDRESS_SAFETY_ANALYSIS;
+
+void noanal_fun_args() __attribute__((no_address_safety_analysis(1))); // \
+  // expected-error {{attribute takes no arguments}}
+
+int noanal_testfn(int y) NO_ADDRESS_SAFETY_ANALYSIS;
+
+int noanal_testfn(int y) {
+  int x NO_ADDRESS_SAFETY_ANALYSIS = y; // \
+    // expected-error {{'no_address_safety_analysis' attribute only applies to functions and methods}}
+  return x;
+}
+
+int noanal_test_var NO_ADDRESS_SAFETY_ANALYSIS; // \
+  // expected-error {{'no_address_safety_analysis' attribute only applies to functions and methods}}
+
+class NoanalFoo {
+ private:
+  int test_field NO_ADDRESS_SAFETY_ANALYSIS; // \
+    // expected-error {{'no_address_safety_analysis' attribute only applies to functions and methods}}
+  void test_method() NO_ADDRESS_SAFETY_ANALYSIS;
+};
+
+class NO_ADDRESS_SAFETY_ANALYSIS NoanalTestClass { // \
+  // expected-error {{'no_address_safety_analysis' attribute only applies to functions and methods}}
+};
+
+void noanal_fun_params(int lvar NO_ADDRESS_SAFETY_ANALYSIS); // \
+  // expected-error {{'no_address_safety_analysis' attribute only applies to functions and methods}}

test/SemaCXX/attr-no-uninitialized-checks.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify  %s
+
+#define NO_UNINITIALIZED_CHECKS __attribute__((no_uninitialized_checks))
+#if !__has_attribute(no_uninitialized_checks)
+#error "Should support no_uninitialized_checks"
+#endif
+
+void noanal_fun() NO_UNINITIALIZED_CHECKS;
+
+void noanal_fun_args() __attribute__((no_uninitialized_checks(1))); // \
+  // expected-error {{attribute takes no arguments}}
+
+int noanal_testfn(int y) NO_UNINITIALIZED_CHECKS;
+
+int noanal_testfn(int y) {
+  int x NO_UNINITIALIZED_CHECKS = y; // \
+    // expected-error {{'no_uninitialized_checks' attribute only applies to functions and methods}}
+  return x;
+}
+
+int noanal_test_var NO_UNINITIALIZED_CHECKS; // \
+  // expected-error {{'no_uninitialized_checks' attribute only applies to functions and methods}}
+
+class NoanalFoo {
+ private:
+  int test_field NO_UNINITIALIZED_CHECKS; // \
+    // expected-error {{'no_uninitialized_checks' attribute only applies to functions and methods}}
+  void test_method() NO_UNINITIALIZED_CHECKS;
+};
+
+class NO_UNINITIALIZED_CHECKS NoanalTestClass { // \
+  // expected-error {{'no_uninitialized_checks' attribute only applies to functions and methods}}
+};
+
+void noanal_fun_params(int lvar NO_UNINITIALIZED_CHECKS); // \
+  // expected-error {{'no_uninitialized_checks' attribute only applies to functions and methods}}