Add support for __declspec(thread) under -fms-extensions

[rnk]

Summary

I had to steal a bit from ScopeDepthOrObjCQuals. This prevents us from
parsing more than 63 nested function prototypes in a parameter context.
We already can't parse more than 127, which is an arbitrary limit
(PR19607). If we care about supporting this, we should add an extra
table to ASTContext for looking up large scope depth values like we do
for parameter indices greater than 254.

[rsmith]

The C++ standard recommends we support a nesting depth of at least 256 for function declarators, but... it seems unlikely anyone's going to get close to that, and as you say, we can deal with that if it ever happens.

include/clang/Basic/DiagnosticSemaKinds.td

2084–2086

Please give this a name involving 'declspec'. It's not as general as its name implies.

include/clang/Sema/DeclSpec.h

235

This looks misleading, since we're never going to see this within a DeclSpec.

lib/Sema/DeclSpec.cpp

386

This seems like it should be unreachable.

[rnk]

include/clang/Basic/DiagnosticSemaKinds.td

2084–2086

Done.

lib/Sema/DeclSpec.cpp

386

It's used for diagnostics now, and it uses DeclSpec::TSCS___declspec_thread.

Would you be in favor of a patch that removes the TSCS_* static const ints in DeclSpec and replaces them with uses of the Basic/Specifiers.h enumerators?

[rnk]

• Add docs and more semantic checks

[rnk]

• Add more tests

[rsmith]

lib/Sema/DeclSpec.cpp

386

Where is it used for diagnostics? It seems strange for someone to pass it a TSCS that didn't come from a DeclSpec, and a DeclSpec cannot have TSCS___declspec_thread.

I don't think I'd be in favor of the patch you suggested -- the DeclSpec type and the Basic type notionally mean different things (even though they have the same underlying enum), and we're just punning between them here. In particular, it does not make sense for the DeclSpec type to have a value for __declspec(thread), since a parsed decl-specifier-seq can't have such a thread storage class specifier.

Giving DeclSpec a separate enum that doesn't have a __declspec(thread) enumerator would seem more consistent with the intent here.

[rsmith]

include/clang/Basic/Attr.td

1671

ITYM ThreadDocs?

include/clang/Basic/AttrDocs.td

58 ↗

(On Diff #8945)

Typo 'compatiblity'

lib/Sema/SemaDecl.cpp

8928–8934 ↗

(On Diff #8945)

This seems really weird. We're already checking for non-constant initialization; do you have any idea what this check is for? You can check for 'any constructor' using CXXRecordDecl::hasUserDeclaredConstructor.

[rnk]

• Remove TSCS___declspec_thread in favor of hasAttr

[rnk]

lib/Sema/SemaDecl.cpp

8928–8934 ↗

(On Diff #8945)

Yeah, this was broken. I fixed the TLS_Static check below in r207675, and it works for __declspec(thread), so I nuked this code.

[rsmith]

LGTM

lib/Sema/SemaDeclAttr.cpp

3700–3717

Are you allowed to specify __declspec(thread) multiple times on the same declaration? This would appear to allow that.

3711–3714

Can you move the hasLocalStorage check into the common attribute subject checking side of things? That'd be more in line with what we do elsewhere, but might make the diagnostic less consistent with other thread specifiers, so I'm on the fence.

[rnk]

Thanks!

lib/Sema/SemaDeclAttr.cpp

3700–3717

MSVC allows it with a warning, and we accept silently. The same is true for all of our other __declspec attributes. If we want to issue a warning, IMO we should do it somewhere common where we can use the same diagnostic reporting.

3711–3714

We don't currently have anything like a GlobalVariable subject but maybe we should. I'd rather not tablegen this for now. Now that I'm looking at the code tablegen is making for us, I feel like we should try to optimize its code size a bit.

[rnk]

Closed by commit rL207734 (authored by @rnk).

include/clang/AST/Decl.h

protected:
   mutable InitType Init;
 
 private:
   class VarDeclBitfields {
     friend class VarDecl;
     friend class ASTDeclReader;
 
     unsigned SClass : 3;
-    unsigned TSCSpec : 2;
+    unsigned TSCSpec : 3;
     unsigned InitStyle : 2;
 
     /// \brief Whether this variable is the exception variable in a C++ catch
     /// or an Objective-C @catch statement.
     unsigned ExceptionVar : 1;
 
     /// \brief Whether this local variable could be allocated in the return
     /// slot of its function, enabling the named return value optimization
class VarDeclBitfields {
     /// init-capture.
     unsigned IsInitCapture : 1;
 
     /// \brief Whether this local extern variable's previous declaration was
     /// declared in the same block scope. This controls whether we should merge
     /// the type of this declaration with its previous declaration.
     unsigned PreviousDeclInSameBlockScope : 1;
   };
-  enum { NumVarDeclBits = 14 };
+  enum { NumVarDeclBits = 15 };
 
   friend class ASTDeclReader;
   friend class StmtIteratorBase;
   friend class ASTNodeImporter;
 
 protected:
   enum { NumParameterIndexBits = 8 };
 
class ParmVarDeclBitfields {
 
     /// Whether this parameter is an ObjC method parameter or not.
     unsigned IsObjCMethodParam : 1;
 
     /// If IsObjCMethodParam, a Decl::ObjCDeclQualifier.
     /// Otherwise, the number of function parameter scopes enclosing
     /// the function parameter scope in which this parameter was
     /// declared.
-    unsigned ScopeDepthOrObjCQuals : 7;
+    unsigned ScopeDepthOrObjCQuals : 6;
 
     /// The number of parameters preceding this parameter in the
     /// function parameter scope in which it was declared.
     unsigned ParameterIndex : NumParameterIndexBits;
   };
 
   union {
     unsigned AllBits;
ThreadStorageClassSpecifier getTSCSpec() const {
     return static_cast<ThreadStorageClassSpecifier>(VarDeclBits.TSCSpec);
   }
   TLSKind getTLSKind() const {
     switch (VarDeclBits.TSCSpec) {
     case TSCS_unspecified:
       return TLS_None;
     case TSCS___thread: // Fall through.
     case TSCS__Thread_local:
+    case TSCS___declspec_thread:
       return TLS_Static;
     case TSCS_thread_local:
       return TLS_Dynamic;
     }
     llvm_unreachable("Unknown thread storage class specifier!");
   }
 
   /// hasLocalStorage - Returns true if a variable with function scope

include/clang/Basic/Attr.td

 }
 
 def SelectAny : InheritableAttr {
   let Spellings = [Declspec<"selectany">];
   let LangOpts = [MicrosoftExt];
   let Documentation = [Undocumented];
 }
 
+def Thread : InheritableAttr {
+  let Spellings = [Declspec<"thread">];
+  let LangOpts = [MicrosoftExt];
+  let Documentation = [Undocumented];
+  let Subjects = SubjectList<[Var]>;
+}
+
 def Win64 : IgnoredAttr {
   let Spellings = [Keyword<"__w64">];
   let LangOpts = [MicrosoftExt];
 }
 
 def Ptr32 : TypeAttr {
   let Spellings = [Keyword<"__ptr32">];
   let Documentation = [Undocumented];

include/clang/Basic/DiagnosticSemaKinds.td

def warn_attribute_weak_on_local : Warning<
   "is not enabled">,
   InGroup<IgnoredAttributes>;
 def warn_weak_identifier_undeclared : Warning<
   "weak identifier %0 never declared">;
 def err_attribute_weak_static : Error<
   "weak declaration cannot have internal linkage">;
 def err_attribute_selectany_non_extern_data : Error<
   "'selectany' can only be applied to data items with external linkage">;
+def err_multiple_thread_specifiers : Error<
+  "__declspec(thread) applied to variable declaration which already has a "
+  "thread specifier">;
 def err_attribute_dll_not_extern : Error<
   "%q0 must have external linkage when declared %q1">;
 def warn_attribute_invalid_on_definition : Warning<
   "'%0' attribute cannot be specified on a definition">,
   InGroup<IgnoredAttributes>;
 def err_attribute_dll_redeclaration : Error<
   "redeclaration of %q0 cannot add %q1 attribute">;
 def err_attribute_dllimport_data_definition : Error<

include/clang/Basic/Specifiers.h

enum ThreadStorageClassSpecifier {
     TSCS_unspecified,
     /// GNU __thread.
     TSCS___thread,
     /// C++11 thread_local. Implies 'static' at block scope, but not at
     /// class scope.
     TSCS_thread_local,
     /// C11 _Thread_local. Must be combined with either 'static' or 'extern'
     /// if used at block scope.
-    TSCS__Thread_local
+    TSCS__Thread_local,
+    /// __declspec(thread). Equivalent to GNU __thread.
+    TSCS___declspec_thread
   };
 
   /// \brief Storage classes.
   enum StorageClass {
     // These are legal on both functions and variables.
     SC_None,
     SC_Extern,
     SC_Static,

include/clang/Sema/DeclSpec.h

enum SCS {
     SCS_mutable
   };
 
   // Import thread storage class specifier enumeration and constants.
   // These can be combined with SCS_extern and SCS_static.
   typedef ThreadStorageClassSpecifier TSCS;
   static const TSCS TSCS_unspecified = clang::TSCS_unspecified;
   static const TSCS TSCS___thread = clang::TSCS___thread;
+  static const TSCS TSCS___declspec_thread = clang::TSCS___declspec_thread;
   static const TSCS TSCS_thread_local = clang::TSCS_thread_local;
   static const TSCS TSCS__Thread_local = clang::TSCS__Thread_local;
 
   // Import type specifier width enumeration and constants.
   typedef TypeSpecifierWidth TSW;
   static const TSW TSW_unspecified = clang::TSW_unspecified;
   static const TSW TSW_short = clang::TSW_short;
   static const TSW TSW_long = clang::TSW_long;

lib/AST/DeclPrinter.cpp

void DeclPrinter::VisitVarDecl(VarDecl *D) {
   if (!Policy.SuppressSpecifiers) {
     StorageClass SC = D->getStorageClass();
     if (SC != SC_None)
       Out << VarDecl::getStorageClassSpecifierString(SC) << " ";
 
     switch (D->getTSCSpec()) {
     case TSCS_unspecified:
       break;
+    case TSCS___declspec_thread:
+      break; // Nothing, the attribute printer will do it for us.
     case TSCS___thread:
       Out << "__thread ";
       break;
     case TSCS__Thread_local:
       Out << "_Thread_local ";
       break;
     case TSCS_thread_local:
       Out << "thread_local ";

lib/Sema/DeclSpec.cpp

const char *DeclSpec::getSpecifierName(DeclSpec::SCS S) {
   }
   llvm_unreachable("Unknown typespec!");
 }
 
 const char *DeclSpec::getSpecifierName(DeclSpec::TSCS S) {
   switch (S) {
   case DeclSpec::TSCS_unspecified:   return "unspecified";
   case DeclSpec::TSCS___thread:      return "__thread";
+  case DeclSpec::TSCS___declspec_thread: return "__declspec(thread)";
   case DeclSpec::TSCS_thread_local:  return "thread_local";
   case DeclSpec::TSCS__Thread_local: return "_Thread_local";
   }
   llvm_unreachable("Unknown typespec!");
 }
 
 const char *DeclSpec::getSpecifierName(TSW W) {
   switch (W) {

lib/Sema/SemaDeclAttr.cpp

static void handleMSInheritanceAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   MSInheritanceAttr *IA = S.mergeMSInheritanceAttr(
       D, Attr.getRange(), /*BestCase=*/true,
       Attr.getAttributeSpellingListIndex(),
       (MSInheritanceAttr::Spelling)Attr.getSemanticSpelling());
   if (IA)
     D->addAttr(IA);
 }
 
+static void handleDeclspecThreadAttr(Sema &S, Decl *D,
+                                     const AttributeList &Attr) {
+  VarDecl *VD = cast<VarDecl>(D);
+  if (VD->getTSCSpec() != TSCS_unspecified) {
+    S.Diag(Attr.getLoc(), diag::err_multiple_thread_specifiers);
+    return;
+  }
+  D->addAttr(::new (S.Context) ThreadAttr(
+      Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
+  VD->setTSCSpec(TSCS___declspec_thread);
+}
+
 static void handleARMInterruptAttr(Sema &S, Decl *D,
                                    const AttributeList &Attr) {
   // Check the attribute arguments.
   if (Attr.getNumArgs() > 1) {
     S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)
       << Attr.getName() << 1;
     return;
   }
 
   StringRef Str;
   SourceLocation ArgLoc;
 
   if (Attr.getNumArgs() == 0)
     Str = "";
case AttributeList::AT_Uuid:
     handleUuidAttr(S, D, Attr);
     break;
   case AttributeList::AT_MSInheritance:
     handleMSInheritanceAttr(S, D, Attr);
     break;
   case AttributeList::AT_SelectAny:
     handleSimpleAttribute<SelectAnyAttr>(S, D, Attr);
     break;
+  case AttributeList::AT_Thread:
+    handleDeclspecThreadAttr(S, D, Attr);
+    break;
 
   // Thread safety attributes:
   case AttributeList::AT_AssertExclusiveLock:
     handleAssertExclusiveLockAttr(S, D, Attr);
     break;
   case AttributeList::AT_AssertSharedLock:
     handleAssertSharedLockAttr(S, D, Attr);
     break;

test/SemaCXX/declspec-thread.cpp

+// RUN: %clang_cc1 -fms-extensions -verify %s
+
+__thread __declspec(thread) int a; // expected-error {{already has a thread specifier}}
+__declspec(thread) __thread int b; // expected-error {{already has a thread specifier}}
+__declspec(thread) int c(); // expected-warning {{only applies to variables}}
+__declspec(thread) int d;
+int foo();
+__declspec(thread) int e = foo(); // expected-error {{must be a constant expression}}