Diagnose use of _Thread_local as an extension when appropriate

[aaron.ballman]

Summary

We currently accept use of _Thread_local in all C and C++ modes, but we do not issue a warning about it being an extension when used outside of C11 mode. This patch adds the warning and adjusts tests for the fallout.

[rsmith]

_Thread_local is a reserved identifier; we generally don't produce extension warnings for uses of reserved identifiers. (Eg, there's no warning for _Atomic in C++ or _Bool in C89, and no warning for uses of __type_traits or __builtins.)

But I note that we *do* warn for some of these already (eg, _Generic, _Static_assert, _Alignas, and _Alignof get a warning). We should choose a rule and apply it consistently.

What's the motivation for warning on this? Maybe that can inform whether these warnings are useful in general.

[aaron.ballman]

In D66364#1633775, @rsmith wrote:

_Thread_local is a reserved identifier; we generally don't produce extension warnings for uses of reserved identifiers. (Eg, there's no warning for _Atomic in C++ or _Bool in C89, and no warning for uses of __type_traits or __builtins.)

But I note that we *do* warn for some of these already (eg, _Generic, _Static_assert, _Alignas, and _Alignof get a warning). We should choose a rule and apply it consistently.

What's the motivation for warning on this? Maybe that can inform whether these warnings are useful in general.

My motivation is for portability. _Thread_local (and all the rest) do not exist in C99 or earlier (or C++), so having some way to warn users of that is useful. I agree that we should be consistent and go with all or none, but my preference is for all (esp since this is a -pedantic warning class).

That said, I think __builtins and __type_traits are a separate question, and a somewhat interesting one. I could see some value in a pedantic warning for use of a reserved identifier which is an implementation detail outside of a system header in theory, but I'm not entirely sure what such a diagnostic would look like in practice. Is __clang__ an implementation detail? I don't think we should warn on uses of it! What about _UNICODE when working with MSVC compat? Same situation. Rather than try to answer that question, I think I draw the line with only pedantically warning on reserved identifiers in the standard. WDYT?

[rsmith]

In D66364#1633981, @aaron.ballman wrote:

My motivation is for portability. _Thread_local (and all the rest) do not exist in C99 or earlier (or C++), so having some way to warn users of that is useful. I agree that we should be consistent and go with all or none, but my preference is for all (esp since this is a -pedantic warning class).

OK, if the motivation is to catch cases where people thought they were writing portable C99 code, but they weren't then I can see this being a useful warning. And that suggests that we should warn on all C11 _Keywords when used in C99 or earlier (or in C++). And I suppose it's reasonable to split the hair between "this is code that someone might think is valid C99" (eg, use of _Static_assert) and "this is code that is using language extensions that no-one is likely to think is valid C99" (eg, use of __builtin_*).

That said, this direction will presumably mean that we start to reject (eg) libc++ when built with -Wsystem-headers -pedantic-errors (because it uses _Atomic to implement std::atomic), which doesn't seem ideal to me. Do you have any thoughts on that? Maybe we should change libc++ to use __extension__ in those instances?

[aaron.ballman]

In D66364#1635795, @rsmith wrote:

In D66364#1633981, @aaron.ballman wrote:

My motivation is for portability. _Thread_local (and all the rest) do not exist in C99 or earlier (or C++), so having some way to warn users of that is useful. I agree that we should be consistent and go with all or none, but my preference is for all (esp since this is a -pedantic warning class).

OK, if the motivation is to catch cases where people thought they were writing portable C99 code, but they weren't then I can see this being a useful warning. And that suggests that we should warn on all C11 _Keywords when used in C99 or earlier (or in C++). And I suppose it's reasonable to split the hair between "this is code that someone might think is valid C99" (eg, use of _Static_assert) and "this is code that is using language extensions that no-one is likely to think is valid C99" (eg, use of __builtin_*).

That said, this direction will presumably mean that we start to reject (eg) libc++ when built with -Wsystem-headers -pedantic-errors (because it uses _Atomic to implement std::atomic), which doesn't seem ideal to me. Do you have any thoughts on that? Maybe we should change libc++ to use __extension__ in those instances?

Adding some libc++ maintainers to see if they have opinions.

__extension__ is one option. Could we get away with push/pop disabling of the diagnostic? Or perhaps this is a situation where we should not diagnose use within a system header in the first place, because that's part of the implementation?

[ldionne]

In D66364#1635814, @aaron.ballman wrote:

[ ...]

Adding some libc++ maintainers to see if they have opinions.

__extension__ is one option. Could we get away with push/pop disabling of the diagnostic? Or perhaps this is a situation where we should not diagnose use within a system header in the first place, because that's part of the implementation?

I just learned about __extension__, but from my perspective it makes sense to mark uses of _Atomic with __extension__ (or disable the warning with a #pragma) inside libc++ if we're using something non-standard for the current dialect. I don't think Clang should bend its back for libc++ in this case.

[aaron.ballman]

In D66364#1635863, @ldionne wrote:

In D66364#1635814, @aaron.ballman wrote:

[ ...]

Adding some libc++ maintainers to see if they have opinions.

__extension__ is one option. Could we get away with push/pop disabling of the diagnostic? Or perhaps this is a situation where we should not diagnose use within a system header in the first place, because that's part of the implementation?

I just learned about __extension__, but from my perspective it makes sense to mark uses of _Atomic with __extension__ (or disable the warning with a #pragma) inside libc++ if we're using something non-standard for the current dialect. I don't think Clang should bend its back for libc++ in this case.

Okay, that's good feedback, thank you!

[ldionne]

In D66364#1637570, @aaron.ballman wrote:

In D66364#1635863, @ldionne wrote:

In D66364#1635814, @aaron.ballman wrote:

[ ...]

Adding some libc++ maintainers to see if they have opinions.

__extension__ is one option. Could we get away with push/pop disabling of the diagnostic? Or perhaps this is a situation where we should not diagnose use within a system header in the first place, because that's part of the implementation?

I just learned about __extension__, but from my perspective it makes sense to mark uses of _Atomic with __extension__ (or disable the warning with a #pragma) inside libc++ if we're using something non-standard for the current dialect. I don't think Clang should bend its back for libc++ in this case.

Okay, that's good feedback, thank you!

Please ping me directly if you expect libc++ maintainers to do something following this patch.

[aaron.ballman]

@rsmith are you fine with implementing the diagnostic for these keywords piecemeal based on the pattern from this patch, or do you want to see an omnibus patch that adds all of them at once?

In D66364#1637635, @ldionne wrote:

Please ping me directly if you expect libc++ maintainers to do something following this patch.

Will do, thanks!

[rsmith]

In D66364#1638026, @aaron.ballman wrote:

@rsmith are you fine with implementing the diagnostic for these keywords piecemeal based on the pattern from this patch, or do you want to see an omnibus patch that adds all of them at once?

I'm fine with doing it piecemeal.

clang/include/clang/Basic/DiagnosticParseKinds.td

130

Please consider rewording this to "%0 is a C11 extension" to match what we do for C++XY extensions. (This will also start looking a little silly once C20 adoption starts; features aren't C11-specific if they're also part of C20.)

As a separate change, though :)

clang/test/Sema/thread-specifier.c

127–160

Hardcoding line numbers like this makes test maintenance painful. Using a different verify prefix seems like the easiest way to handle this:

// RUN: %clang_cc1 [...] -D__thread=_Thread_local -std=c++98 -verify=expected,thread-local
[...]
__thread int t1; // thread-local-warning {{_Thread_local is a C11-specific feature}}

[aaron.ballman]

Committed in r369954

clang/include/clang/Basic/DiagnosticParseKinds.td

130

Agreed, and will do this in a follow-up patch.

clang/test/Sema/thread-specifier.c

127–160

Thank you for the suggestion, the results are *so* much cleaner than this original attempt!

clang/include/clang/Basic/DiagnosticParseKinds.td

 
 def ext_c11_generic_selection : Extension<
   "generic selections are a C11-specific feature">, InGroup<C11>;
 def err_duplicate_default_assoc : Error<
   "duplicate default generic association">;
 def note_previous_default_assoc : Note<
   "previous default generic association is here">;
 
-def ext_c11_alignment : Extension<
+def ext_c11_feature : Extension<
   "%0 is a C11-specific feature">, InGroup<C11>;
 
 def ext_c11_noreturn : Extension<
   "_Noreturn functions are a C11-specific feature">, InGroup<C11>;
 def err_c11_noreturn_misplaced : Error<
   "'_Noreturn' keyword must precede function declarator">;
 
 def ext_gnu_indirect_goto : Extension<
   "use of GNU indirect-goto extension">, InGroup<GNULabelsAsValue>;

clang/lib/Parse/ParseDecl.cpp

       isStorageClass = true;
       break;
     case tok::kw_thread_local:
       isInvalid = DS.SetStorageClassSpecThread(DeclSpec::TSCS_thread_local, Loc,
                                                PrevSpec, DiagID);
       isStorageClass = true;
       break;
     case tok::kw__Thread_local:
+      if (!getLangOpts().C11)
+        Diag(Tok, diag::ext_c11_feature) << Tok.getName();
       isInvalid = DS.SetStorageClassSpecThread(DeclSpec::TSCS__Thread_local,
                                                Loc, PrevSpec, DiagID);
       isStorageClass = true;
       break;
 
     // function-specifier
     case tok::kw_inline:
       isInvalid = DS.setFunctionSpecInline(Loc, PrevSpec, DiagID);

       if (!getLangOpts().C11)
         Diag(Loc, diag::ext_c11_noreturn);
       isInvalid = DS.setFunctionSpecNoreturn(Loc, PrevSpec, DiagID);
       break;
 
     // alignment-specifier
     case tok::kw__Alignas:
       if (!getLangOpts().C11)
-        Diag(Tok, diag::ext_c11_alignment) << Tok.getName();
+        Diag(Tok, diag::ext_c11_feature) << Tok.getName();
       ParseAlignmentSpecifier(DS.getAttributes());
       continue;
 
     // friend
     case tok::kw_friend:
       if (DSContext == DeclSpecContext::DSC_class)
         isInvalid = DS.SetFriendSpec(Loc, PrevSpec, DiagID);
       else {

clang/lib/Parse/ParseExpr.cpp

     SourceLocation SavedLoc = ConsumeToken();
     Res = ParseCastExpression(false);
     if (!Res.isInvalid())
       Res = Actions.ActOnUnaryOp(getCurScope(), SavedLoc, SavedKind, Res.get());
     return Res;
   }
   case tok::kw__Alignof:   // unary-expression: '_Alignof' '(' type-name ')'
     if (!getLangOpts().C11)
-      Diag(Tok, diag::ext_c11_alignment) << Tok.getName();
+      Diag(Tok, diag::ext_c11_feature) << Tok.getName();
     LLVM_FALLTHROUGH;
   case tok::kw_alignof:    // unary-expression: 'alignof' '(' type-id ')'
   case tok::kw___alignof:  // unary-expression: '__alignof' unary-expression
                            // unary-expression: '__alignof' '(' type-name ')'
   case tok::kw_sizeof:     // unary-expression: 'sizeof' unary-expression
                            // unary-expression: 'sizeof' '(' type-name ')'
   case tok::kw_vec_step:   // unary-expression: OpenCL 'vec_step' expression
   // unary-expression: '__builtin_omp_required_simd_align' '(' type-name ')'

clang/test/PCH/thread-local.cpp

-// RUN: %clang_cc1 -pedantic-errors -std=c++11 -triple x86_64-linux-gnu -emit-pch %s -o %t
-// RUN: %clang_cc1 -pedantic-errors -std=c++11 -triple x86_64-linux-gnu -include-pch %t -verify %s
+// RUN: %clang_cc1 -pedantic-errors -Wno-c11-extensions -std=c++11 -triple x86_64-linux-gnu -emit-pch %s -o %t
+// RUN: %clang_cc1 -pedantic-errors -Wno-c11-extensions -std=c++11 -triple x86_64-linux-gnu -include-pch %t -verify %s
 // REQUIRES: x86-registered-target
 #ifndef HEADER_INCLUDED
 
 #define HEADER_INCLUDED
 extern thread_local int a;
 extern _Thread_local int b;
 extern int c;
 

clang/test/Sema/thread-specifier.c

 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -DGNU
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DGNU -std=c++98
-// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -DC11 -D__thread=_Thread_local
-// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local -std=c++98
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -std=c11 -DC11 -D__thread=_Thread_local
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local -std=c++98 -DWARN_ON_THREAD_LOCAL
 // RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DCXX11 -D__thread=thread_local -std=c++11 -Wno-deprecated
-// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local -std=c++11 -Wno-deprecated
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic -x c++ %s -DC11 -D__thread=_Thread_local -std=c++11 -Wno-deprecated -DWARN_ON_THREAD_LOCAL
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fsyntax-only -Wno-private-extern -verify -pedantic %s -std=c99 -D__thread=_Thread_local -DWARN_ON_THREAD_LOCAL -DC99
 
 #ifdef __cplusplus
 // In C++, we define __private_extern__ to extern.
 #undef __private_extern__
 #endif
 
 __thread int t1;
 __thread extern int t2;

 #else
 // FIXME: The 'is only allowed on variable declarations' diagnostic is better here.
 // expected-error@-5 {{type name does not allow storage class to be specified}}
 #endif
 
 __thread int t6();
 #if defined(GNU)
 // expected-error@-2 {{'__thread' is only allowed on variable declarations}}
-#elif defined(C11)
+#elif defined(C11) || defined(C99)
 // expected-error@-4 {{'_Thread_local' is only allowed on variable declarations}}
 #else
 // expected-error@-6 {{'thread_local' is only allowed on variable declarations}}
 #endif
 
 int f(__thread int t7) { // expected-error {{' is only allowed on variable declarations}}
   __thread int t8;
 #if defined(GNU)
   // expected-error@-2 {{'__thread' variables must have global storage}}
-#elif defined(C11)
+#elif defined(C11) || defined(C99)
   // expected-error@-4 {{'_Thread_local' variables must have global storage}}
 #endif
   extern __thread int t9;
   static __thread int t10;
   __thread __private_extern__ int t11;
 #if __cplusplus < 201103L
   __thread auto int t12a; // expected-error-re {{cannot combine with previous '{{__thread|_Thread_local}}' declaration specifier}}
   auto __thread int t12b; // expected-error {{cannot combine with previous 'auto' declaration specifier}}

 // expected-error@-2 {{initializer for thread-local variable must be a constant expression}}
 #if __cplusplus >= 201103L
 // expected-note@-4 {{use 'thread_local' to allow this}}
 #endif
 #endif
 #endif
 
 __thread int aggregate[10] = {0};
+
+#ifdef WARN_ON_THREAD_LOCAL
+// expected-warning@14 {{_Thread_local is a C11-specific feature}}
+// expected-warning@15 {{_Thread_local is a C11-specific feature}}
+// expected-warning@16 {{_Thread_local is a C11-specific feature}}
+// expected-warning@22 {{_Thread_local is a C11-specific feature}}
+// expected-warning@23 {{_Thread_local is a C11-specific feature}}
+// expected-warning@31 {{_Thread_local is a C11-specific feature}}
+// expected-warning@40 {{_Thread_local is a C11-specific feature}}
+// expected-warning@41 {{_Thread_local is a C11-specific feature}}
+// expected-warning@47 {{_Thread_local is a C11-specific feature}}
+// expected-warning@48 {{_Thread_local is a C11-specific feature}}
+// expected-warning@49 {{_Thread_local is a C11-specific feature}}
+#if __cplusplus < 201103L
+// expected-warning@51 {{_Thread_local is a C11-specific feature}}
+// expected-warning@52 {{_Thread_local is a C11-specific feature}}
+#elif !defined(CXX11)
+// expected-warning@54 {{_Thread_local is a C11-specific feature}}
+// expected-warning@55 {{_Thread_local is a C11-specific feature}}
+#endif
+// expected-warning@57 {{_Thread_local is a C11-specific feature}}
+// expected-warning@58 {{_Thread_local is a C11-specific feature}}
+// expected-warning@61 {{_Thread_local is a C11-specific feature}}
+// expected-warning@62 {{_Thread_local is a C11-specific feature}}
+// expected-warning@65 {{_Thread_local is a C11-specific feature}}
+// expected-warning@75 {{_Thread_local is a C11-specific feature}}
+// expected-warning@88 {{_Thread_local is a C11-specific feature}}
+#ifdef __cplusplus
+// expected-warning@102 {{_Thread_local is a C11-specific feature}}
+// expected-warning@115 {{_Thread_local is a C11-specific feature}}
+#endif
+// expected-warning@124 {{_Thread_local is a C11-specific feature}}
+#endif
+#ifdef CXX11
+// expected-warning@69 {{_Thread_local is a C11-specific feature}}
+// expected-warning@70 {{_Thread_local is a C11-specific feature}}
+#endif
+

clang/test/SemaOpenCLCXX/restricted.cl

 B *test_dynamic_cast(B *p) {
   return dynamic_cast<B *>(p);
   // expected-error@-1 {{'dynamic_cast' is not supported in C++ for OpenCL}}
 }
 
 // Test storage class qualifiers.
 __constant _Thread_local int a = 1;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '_Thread_local' storage class specifier}}
+// expected-warning@-2 {{_Thread_local is a C11-specific feature}}
+
 __constant __thread int b = 2;
 // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the '__thread' storage class specifier}}
 kernel void test_storage_classes() {
   register int x;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'register' storage class specifier}}
   thread_local int y;
   // expected-error@-1 {{C++ for OpenCL version 1.0 does not support the 'thread_local' storage class specifier}}
 }