Page MenuHomePhabricator

Diagnose use of _Thread_local as an extension when appropriate
ClosedPublic

Authored by aaron.ballman on Aug 16 2019, 1:18 PM.

Details

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.

Diff Detail

Event Timeline

aaron.ballman created this revision.Aug 16 2019, 1:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 1:18 PM

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

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

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?

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?

[ ...]

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.

[ ...]

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!

[ ...]

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.

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

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

Will do, thanks!

rsmith accepted this revision.Aug 23 2019, 3:24 PM

@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}}
This revision is now accepted and ready to land.Aug 23 2019, 3:24 PM
aaron.ballman marked 2 inline comments as done.

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!