[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

Committed in r369954




Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:130
+def ext_c11_feature : Extension<
   "%0 is a C11-specific feature">, InGroup;
 

rsmith wrote:
> 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 :)
Agreed, and will do this in a follow-up patch.



Comment at: clang/test/Sema/thread-specifier.c:127-160
+// 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}}

rsmith wrote:
> 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}}
> ```
Thank you for the suggestion, the results are *so* much cleaner than this 
original attempt!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

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.




Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:130
+def ext_c11_feature : Extension<
   "%0 is a C11-specific feature">, InGroup;
 

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 :)



Comment at: clang/test/Sema/thread-specifier.c:127-160
+// 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}}

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}}
```


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

@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!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: mclow.lists, ldionne.
aaron.ballman added a comment.
Herald added a subscriber: dexonsmith.

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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

`_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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66364/new/

https://reviews.llvm.org/D66364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added a reviewer: rsmith.
Herald added a project: clang.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D66364

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/test/PCH/thread-local.cpp
  clang/test/Sema/thread-specifier.c
  clang/test/SemaOpenCLCXX/restricted.cl

Index: clang/test/SemaOpenCLCXX/restricted.cl
===
--- clang/test/SemaOpenCLCXX/restricted.cl
+++ clang/test/SemaOpenCLCXX/restricted.cl
@@ -31,6 +31,8 @@
 // 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() {
Index: clang/test/Sema/thread-specifier.c
===
--- clang/test/Sema/thread-specifier.c
+++ clang/test/Sema/thread-specifier.c
@@ -1,9 +1,10 @@
 // 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.
@@ -30,7 +31,7 @@
 __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}}
@@ -40,7 +41,7 @@
   __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;
@@ -121,3 +122,41 @@
 #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