This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rnk on Apr 29 2014, 12:59 PM.

Details

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.

Diff Detail

Event Timeline

rnk updated this revision to Diff 8939.Apr 29 2014, 12:59 PM
rnk retitled this revision from to Add support for __declspec(thread) under -fms-extensions.
rnk added a reviewer: rsmith.
rnk updated this object.
rnk added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Apr 29 2014, 2:55 PM

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 added inline comments.Apr 29 2014, 3:05 PM
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 updated this revision to Diff 8944.Apr 29 2014, 3:05 PM
rnk edited edge metadata.
  • Add docs and more semantic checks
rnk updated this revision to Diff 8945.Apr 29 2014, 3:13 PM
  • Add more tests
rsmith added inline comments.Apr 29 2014, 6:05 PM
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 added inline comments.Apr 29 2014, 6:14 PM
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 updated this revision to Diff 8988.Apr 30 2014, 11:50 AM
  • Remove TSCS___declspec_thread in favor of hasAttr
rnk added inline comments.Apr 30 2014, 11:58 AM
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 accepted this revision.Apr 30 2014, 7:32 PM
rsmith edited edge metadata.

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.

This revision is now accepted and ready to land.Apr 30 2014, 7:32 PM
rnk added a comment.Apr 30 2014, 8:22 PM

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 this revision.May 19 2014, 7:23 AM
rnk updated this revision to Diff 9549.

Closed by commit rL207734 (authored by @rnk).