[PATCH] Add support for __declspec(thread) under -fms-extensions

[Aaron Ballman]

On Wed, Apr 30, 2014 at 11:22 PM, Reid Kleckner <rnk at google.com> wrote:
> Thanks!
>
> ================
> Comment at: lib/Sema/SemaDeclAttr.cpp:3700-3717
> @@ -3699,1 +3699,20 @@
>
> +static void handleDeclspecThreadAttr(Sema &S, Decl *D,
> +                                     const AttributeList &Attr) {
> +  VarDecl *VD = cast<VarDecl>(D);
> +  if (!S.Context.getTargetInfo().isTLSSupported()) {
> +    S.Diag(Attr.getLoc(), diag::err_thread_unsupported);
> +    return;
> +  }
> +  if (VD->getTSCSpec() != TSCS_unspecified) {
> +    S.Diag(Attr.getLoc(), diag::err_declspec_thread_on_thread_variable);
> +    return;
> +  }
> +  if (VD->hasLocalStorage()) {
> +    S.Diag(Attr.getLoc(), diag::err_thread_non_global) << "__declspec(thread)";
> +    return;
> +  }
> +  VD->addAttr(::new (S.Context) ThreadAttr(
> +      Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
> +}
> +
> ----------------
> Richard Smith wrote:
>> Are you allowed to specify `__declspec(thread)` multiple times on the same declaration? This would appear to allow that.
> 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.

Definitely agreed.

>
> ================
> Comment at: lib/Sema/SemaDeclAttr.cpp:3711-3714
> @@ +3710,6 @@
> +  }
> +  if (VD->hasLocalStorage()) {
> +    S.Diag(Attr.getLoc(), diag::err_thread_non_global) << "__declspec(thread)";
> +    return;
> +  }
> +  VD->addAttr(::new (S.Context) ThreadAttr(
> ----------------
> Richard Smith wrote:
>> 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.
> We don't currently have anything like a GlobalVariable subject but maybe we should.

Yes we do:

def GlobalVar : SubsetSubject<Var,
                             [{S->hasGlobalStorage()}]>;


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

I agree; if I get the chance, I was intending to look into cleaning it
up a bit. But if you would like to take a crack at it, I have no
objections. :-)

~Aaron