[ms] Add support for parsing uuid as a MS attribute

[thakis]

Summary

Some Windows SDK classes, for example Windows::Storage::Streams::IBufferByteAccess, use the ATL way of spelling attributes:

[uuid("....")] class IBufferByteAccess {};

To be able to use __uuidof() to grab the uuid off these types, clang needs to support uuid as a Microsoft attribute. There was already code to skip Microsoft attributes, extend that to look for uuid and parse it. Add a new "MS" attribute type for this syntax.

There's already a function that moves attributes off the declspec into an attribute list for attributes applying to the type, teach that function to also move MS attributes around. This requires that MS attributes make it to the DS in the first place, so move the call to MaybeParseMicrosoftAttributes() into ParseDeclOrFunctionDefInternal() so that it can store its results in DS.getAttributes().

[rsmith]

include/clang/Basic/Attr.td

201

Is there some better description for this than MS? __declspec is also an MS attribute. Is it fair to call this IDL or MSIDL or similar?

include/clang/Parse/Parser.h

2109–2111

Can you give this a less vague-sounding name? :)

lib/Parse/ParseDeclCXX.cpp

3950

Is anything known to fail if you accept arbitrary attributes here, or is this just the only one you /know/ you need right now? (If we can ditch the whitelist, that would seem preferable.)

[rnk]

I think these are known as "IDL attributes":
https://msdn.microsoft.com/en-us/library/8tesw2eh.aspx

Let's update the naming to use that terminology, so AS_MS should be AS_IDL, and MaybeParseMicrosoftAttributes should be MaybeParseMicrosoftIDLAttributes, etc.

Also, doesn't this introduce ambiguities into the grammar? Something like this:

void useit(int);
int main() {
  int uuid = 42;
  [uuid]() {
    useit(uuid);
  }();
}

Will we keep parsing that as a lambda after this change or not?

[thakis]

Thanks for the fast review and the good comments! I'll leave it to y'all to agree on some name if you don't like the one I picked.

Also, doesn't this introduce ambiguities into the grammar? Something like this:

void useit(int);
int main() {
  int uuid = 42;
  [uuid]() {
    useit(uuid);
  }();
}

Will we keep parsing that as a lambda after this change or not?

This change shouldn't change how things are parsed. It moved calls of MaybeParseMicrosoftAttributes(attrs) that precede ParseExternalDeclaration(attrs) into ParseExternalDeclaration(). As far as I can tell, every call of ParseExternalDeclaration() was preceded by a call to MaybeParseMicrosoftAttributes(), so this should be behavior-preserving. In your example, the lamda is not an external declaration, so it's not affected. (That means [uuid(...)] won't work for local classes, but that's probably ok.)

Now that I looked at this more, behavior changes slightly for empty external declarations. Before,

[foob];

didn't produce any output, now it produces "declaration doesn't declare anything". I don't know if that's important.

include/clang/Basic/Attr.td

201

There are many IDL compilers. MSIDL works for me, so does anything else you 3 agree on :-)

include/clang/Parse/Parser.h

2109–2111

Changed to stripTypeAttribsOffDeclSpec

lib/Parse/ParseDeclCXX.cpp

3950

These attributes can be different from decl spec args. For example, we don't have anything that parses threading("apartment"), module(name="MyLib") at the moment (I suppose ParseAttributeArgsCommon does that at a token level, but it won't know all these attributes that are allowed here).

[thakis]

comments round 1

[thakis]

Aaron's right that these are currently called "Microsoft" in the source. I agree that that's a confusing name and we should probably rename it, but on second thought that seems unrelated to this change. If y'all agree on a name, I can do the renaming in a separate change. I changed the patch to use "Microsoft" until then. I also realized that this was missing a tablegen change; now included.

[thakis]

rnk: ping

[rnk]

The code looks good and I'm happy to leave the naming as it is for now. Apparently MSVC is warning on these attributes and calling them "ATL attributes" now: https://msdn.microsoft.com/en-us/library/mt723604.aspx I'm not sure if that warning applies to all square-bracketed user defined attributes or not, but the naming is certainly confusing.

Even though this change doesn't change the behavior of the parser, I would feel more comfortable if we added a new test at test/SemaCXX/ms-square-bracket-attributes.cpp or something that exhaustively lists the ambiguities we know about and codifies how we diagnose them. That way, if someone accidentally changes behavior in this area, we'll know about it. This seems like a good starting point for the test:

void local_class() {
  // FIXME: MSVC accepts, but we reject due to ambiguity.
  // expected-error@+1 {{expected body of lambda expression}}
  [uuid("a5a7bd07-3b14-49bc-9399-de066d4d72cd")] struct Local {
    int x;
  };
}

void useit(int);

int lambda() {
  int uuid = 42;
  [uuid]() {
    useit(uuid);
  }();
}

[aaron.ballman]

You are removing a lot of instances of MaybeParseMicrosoftAttributes(), but I'm not certain I understand why. Can you describe why you need to remove those?

There are things missing from this patch as well. To wit:

• Attributes.h needs to know about this attribute syntax, and hasAttribute() needs to support it (this hooks into __has_attribute support).
• AttributeList.h should be updated to expose the parsed attribute syntax.
• Both of these changes should be threaded through ClangAttrEmitter.cpp.
• We should consider adding (perhaps as a separate patch?) __has_microsoft_attribute() to complement __has_declspec_attribute().
• This change should probably go into the release notes.

I agree with @rnk, I would like to see more comprehensive tests of what we do and do not support for the parsing of these attributes. Previously, we didn't support Microsoft attributes at all, so it was fine to just eat them whenever they appeared and not worry about whether we were actually parsing them properly or not. Now that we want to support them, I want to make sure the parsing is correct, even if uuid wouldn't show up in that particular position. I am fine if your tests show things we don't parse properly yet with FIXME comments (assuming that your patch does not regress that behavior). However, I'd prefer those tests appear in Parser rather than SemaCXX. ;-)

include/clang/Parse/Parser.h

2109–2110

Better, but how about stripTypeAttributesOffDeclSpec? We don't use "attribs" anywhere else.

include/clang/Sema/AttributeList.h

111

Please remove the trailing comma.

lib/Parse/ParseDecl.cpp

516–518

This change is not incorrect, but I think you need a custom parsing step as well. IIRC, the uuid attribute supports unquoted GUIDs, like [uuid(A-B-C-D)], and this won't handle it properly. Attr.td says UUID takes a StringArgument, which may require a bit of thought as to whether we need a flag on there to say "or... maybe not a string...for Microsoft attributes only". Perhaps other MS attributes do the same thing and we're okay for our declarative information?

1427–1428

I'd spell this out a bit more.

// Usually, `__attribute__((attrib)) class Foo {} var;` means that the attribute appertains to the declaration (var) rather than the type (Foo).

1431–1434

Is this documented somewhere, or just what you gather from experimentation?

utils/TableGen/ClangAttrEmitter.cpp

2372 ↗

(On Diff #69419)

I think (but my memory is a bit fuzzy here) that these values need to match the ones from AttributeList.h. I think it's a bug that Pragma was at 4 rather than 5, but since __has_attribute doesn't care about pragmas (or context sensitive keywords), it's likely benign.

[rnk]

In D23895#528208, @aaron.ballman wrote:

You are removing a lot of instances of MaybeParseMicrosoftAttributes(), but I'm not certain I understand why. Can you describe why you need to remove those?

That shouldn't change functionality, Nico wrote: "It moved calls of MaybeParseMicrosoftAttributes(attrs) that precede ParseExternalDeclaration(attrs) into ParseExternalDeclaration(). As far as I can tell, every call of ParseExternalDeclaration() was preceded by a call to MaybeParseMicrosoftAttributes(), so this should be behavior-preserving."

include/clang/Sema/AttributeList.h

111

IMO trailing commas in long lists are nice. There's a reason they got added to C++11.

[thakis]

There are things missing from this patch as well. To wit:

• Attributes.h needs to know about this attribute syntax, and hasAttribute() needs to support it (this hooks into __has_attribute support).
• AttributeList.h should be updated to expose the parsed attribute syntax.
• Both of these changes should be threaded through ClangAttrEmitter.cpp.

All these are already part of this patch as far as I can tell. What am I missing?

lib/Parse/ParseDecl.cpp

1427–1428

(ish)

1431–1434

Experimentation.

utils/TableGen/ClangAttrEmitter.cpp

2372 ↗

(On Diff #69419)

I can change pragma to map to one more in a separate patch if you want.

[thakis]

(Just wanted to ask if I'm missing something about these three bullets. I do need to make a few other changes: More tests, parsing for uuid(1-2-3) without quotes + tests, and I also want to add a deprecation warning with a fixit, given that msvs does that too)

[aaron.ballman]

In D23895#529275, @thakis wrote:

There are things missing from this patch as well. To wit:

• Attributes.h needs to know about this attribute syntax, and hasAttribute() needs to support it (this hooks into __has_attribute support).
• AttributeList.h should be updated to expose the parsed attribute syntax.
• Both of these changes should be threaded through ClangAttrEmitter.cpp.

All these are already part of this patch as far as I can tell. What am I missing?

This is what happens when I split my review into two sessions. I think I was just tired, because I now see that you did change Attributes.h and AttributeList.h properly. Sorry about the noise! However, I think GenerateHasAttrSpellingStringSwitch() should be updated (the Microsoft variety shouldn't work if Microsoft extensions are disabled), but otherwise ClangAttrEmitter.cpp seems reasonable.

lib/Parse/ParseDecl.cpp

1431–1434

It might be good to clarify that in the comment so we don't come along later and think it's a hard-and-fast rule.

utils/TableGen/ClangAttrEmitter.cpp

2372 ↗

(On Diff #69419)

Yeah, or I can take care of it, either way is fine by me.

[thakis]

Sorry about the delay! Now with custom parsing code that accepts uuid() without quotes. I thought this would be complicated, but ended up being not so bad in the end. See the tests for several interesting edge cases.

I figured I'd punt the deprecation warning to a (fast) follow-up.

[rnk]

lib/Parse/ParseDeclCXX.cpp

3943

This isn't so bad. :) I was expecting you'd have to change the lexer.

test/Parser/ms-square-bracket-attributes.mm

138 ↗

(On Diff #70013)

nice

[aaron.ballman]

include/clang/Parse/Parser.h

2217

Attrs instead of attrs

lib/Parse/ParseDeclCXX.cpp

3947

Should we use Twine for building this string up (since this is used in a lot of header files)?

3950

How well does this diagnose [uuid{"000000A0-0000-0000-C000-000000000049"}] struct S {};? (Note the use of curly braces in lieu of parens for the argument.)

3993

Will this properly handle an abomination involving string literal concatenation? I shudder to type this, but: [uuid({000000A0-0000-""0000-C000-000000000049})] struct S {}; MSVC rejects, but I think this code will silently accept it.

test/Parser/ms-square-bracket-attributes.mm

12 ↗

(On Diff #70013)

Can we also get a test case showing the behavior of multiple arguments in the same uuid attribute? e.g., [uuid("{000000A0-0000-0000-C000-000000000049}", "1")] struct S {};
And a test showing that we reject multiple uuid attributes on the same entity? e.g., [uuid("{000000A0-0000-0000-C000-000000000049}"), uuid("{000000A0-0000-0000-C000-000000000050}")] struct S {};

[thakis]

Address comments.

[thakis]

include/clang/Parse/Parser.h

2217

It's spelled attrs in the function above and below this one…

lib/Parse/ParseDeclCXX.cpp

3947

I thought Twine is a rvalue type…yes, from its docs:

/// A Twine is not intended for use directly and should not be stored, its
/// implementation relies on the ability to store pointers to temporary stack
/// objects which may be deallocated at the end of a statement. Twines should
/// only be used accepted as const references in arguments, when an API wishes
/// to accept possibly-concatenated strings.

I think this is should be pretty fast as-is.

3950

That's currently silently ignored since ParseMicrosoftAttributes() looks for 'uuid' '(' to call the uuid parser.

3993

This is rejected, "" is returned as a string token which becomes "" (including quotes) after getSpelling. Added a test case with this.

test/Parser/ms-square-bracket-attributes.mm

12 ↗

(On Diff #70013)

The first is a nice testcase, added, thanks.

We don't currently reject multiple uuids (also not with declspec). Fixing this is on my list of follow-ups, but since the fix will live in sema land and be independent of the attribute spelling, I didn't put it in this change. (Fun fact: cl.exe doesn't warn if you have one []-uuid and one declspec-uuid. We'll get that right once we diag on it.)

[aaron.ballman]

lib/Parse/ParseDeclCXX.cpp

3950

Silently ignoring seems like the wrong thing to do -- can we diagnose (even if it's a less-than-ideal diagnostics with a fixme)?

test/Parser/ms-square-bracket-attributes.mm

13 ↗

(On Diff #70036)

Diagnosing the second case in a follow-up patch is totally fine by me.

[rsmith]

include/clang/Basic/Attr.td

201

Given that MS have deprecated this syntax and are trying to move away from it towards __declspec, I think calling this "Microsoft" is a mistake. As MS' documentation calls these "ATL attributes", it would seem reasonable for us to do the same.

[thakis]

lib/Parse/ParseDeclCXX.cpp

3950

We still skip the majority of [] contents. Maybe the token 'uuid' can appear in some other attribute, followed by something else. So we probably shouldn't diag on 'uuid' followed by not-'(' (right?). Do you want me to add a diagnostic for 'uuid' '{'? What about 'uuid' '['?

[aaron.ballman]

lib/Parse/ParseDeclCXX.cpp

3950

The grammar for the uuid attribute shows it requires the parens (it also shows that it only accepts a string literal, so take that with a grain of salt), so I think we should diagnose in this case, especially since we're manually parsing the args. So if you see "[uuid" followed by any token other than "(", I think that's an error (and MSVC treats it as such).

[thakis]

lib/Parse/ParseDeclCXX.cpp

3950

Sure, but uuid could be preceded by other tokens, e.g.

[ someotherattrib(foobar...) ..., uuid {

someotherattrib might take an argument that's called uuid and it might be valid to have uuid followed by something not '(' there, say [identify_class_by(uuid), uuid("1-2-3")]. This is a made-up example; I don't know if this is actually the case, I'm just saying I don't know, so I think I shouldn't diag on 'uuid' followed by something not-'(' in general.

Do you want me to peephole-diag on '[' 'uuid' not-'(' in the case when uuid is the first attrib in the attrib list?

[thakis]

include/clang/Basic/Attr.td

201

As said upthread, I'm happy to rename this to whatever you all agree on (or what you decree given that you're code owner), but they are called Microsoft attributes in today's code already, and renaming that is unrelated to this CL. If you think it's important, I'm happy to rename the existing code before lading this CL and then using the new name in this CL immediately.

Re "ATL attributes": I think some of the attributes in [] are SAL attributes which as far as I know is independent of ATL. Are you sure the documentation refers to the [] syntax, or could it refer to specific attributes in that syntax?

[aaron.ballman]

include/clang/Basic/Attr.td

201

MS' documentation does not call them "ATL attributes" only. The documentation also calls them COM attributes, compiler attributes, and other names. However, the compiler diagnoses them as "usage of ATL attributes is deprecated", so it's somewhat defensible as a name.

I think "ATL" is the wrong name for them *only here*, but I'm not opposed to making a sweeping change to name them ATL attributes everywhere in the compiler (including here). However, I also don't see an issue with continuing to call them Microsoft attributes, either, because it's unlikely to cause any lasting confusion (it's caused zero confusion in the years I've been working on Clang).

lib/Parse/ParseDeclCXX.cpp

3950

I'm a bit lost. You've processed the open square bracket, so you know you're in an attribute list. Then you ignore everything until you get an identifier. If that identifier is uuid, you do special processing. Right now, you require uuid( to do the special processing. What I am saying is that if, when processing an attribute-token within an attribute list, you find uuid followed by anything other than a (, it should be diagnosed.

So, I expect [identify_class_by(uuid), uuid("1-2-3")] to be fine, but [uuid["1-2-3-"], identify_by_class(uuid), uuid*] to diagnose the uuid["1-2-3"] and uuid* as being malformed (though I'd be fine if we skipped everything past the first malformed attribute if it's better for error recovery).

[thakis]

lib/Parse/ParseDeclCXX.cpp

3950

I too feel a bit lost :-)

Right now, the parsing code doesn't look at attribute lists, only at tokens, so [identify_class_by(uuid), skips tokens until we hit 'uuid', and if I were to diag on not-'(' after 'uuid', then I'd diag here, right? Do you want me to do some more parsing to look for comma-separated entities in the attribute list (and count parens brackets braces and ignore commas there) to fix this?

Sorry, I feel we're talking past each other :-(

[aaron.ballman]

lib/Parse/ParseDeclCXX.cpp

3950

I think we're getting to the crux of it, and I'm sorry for my being obtuse -- yes, I would like this to properly parse it as a list of attributes, rather than this eat-and-pray approach. The first element must be an identifier. If we get uuid, we process it as discussed. Any other identifier, we eat until we hit a ',', ']', or eof, and continue (if not ]).

[aaron.ballman]

include/clang/Basic/Attr.td

201

It seems that the "usage of ATL attributes is deprecated" means attributes appertaining to ATL functionality are deprecated, not that the attribute syntax is "ATL attribute" syntax which is deprecated. For instance, SAL attributes can be used and no diagnostic is emitted. Based on that, I think calling them ATL attributes is definitely incorrect.

[thakis]

Require '('.

[thakis]

lib/Parse/ParseDeclCXX.cpp

3950

Aha, I had forgotten that SkipUntil() handles paren nesting intelligently, so I think just requiring a '(' after 'uuid' with the current code should work fairly well. Done, and added a test.

[aaron.ballman]

LGTM with one small test case nit.

test/Parser/ms-square-bracket-attributes.mm

43 ↗

(On Diff #70197)

For giggles, can you also add:

// expected-error@+1 {{expected ')'}}
[uuid("000000A0-0000-0000-C000-000000000049"}] struct T {};

[rsmith]

For submit, can you separate the changes to generally support [] attribute syntax and the changes to parse [uuid(...)] into distinct commits? (I'm OK with the Microsoft attribute support piece landing with no tests since the uuid patch adds the relevant testing -- and I believe that piece by itself is NFC.)

test/Parser/ms-square-bracket-attributes.mm

129–131 ↗

(On Diff #70197)

Please also test cases like

[uuid(00000000-0000-0000-0000-000000000000)] { return uuid; }
[uuid("00000000-0000-0000-0000-000000000000")] (int n) { return uuid[n]; }

(which are lambda *init-capture*s in C++14 onwards).

[thakis]

Thanks! Landed in r280574 r280575 r280576 280578.

include/clang/Basic/Attr.td

 class Spelling<string name, string variety> {
   string Name = name;
   string Variety = variety;
   bit KnownToGCC;
 }
 
 class GNU<string name> : Spelling<name, "GNU">;
 class Declspec<string name> : Spelling<name, "Declspec">;
+class MS<string name> : Spelling<name, "MS">;
 class CXX11<string namespace, string name, int version = 1>
     : Spelling<name, "CXX11"> {
   string Namespace = namespace;
   int Version = version;
 }
 class Keyword<string name> : Spelling<name, "Keyword">;
 class Pragma<string namespace, string name> : Spelling<name, "Pragma"> {
   string Namespace = namespace;

 }
 
 def Used : InheritableAttr {
   let Spellings = [GCC<"used">];
   let Documentation = [Undocumented];
 }
 
 def Uuid : InheritableAttr {
-  let Spellings = [Declspec<"uuid">];
+  let Spellings = [Declspec<"uuid">, MS<"uuid">];
   let Args = [StringArgument<"Guid">];
 //  let Subjects = SubjectList<[CXXRecord]>;
   let LangOpts = [MicrosoftExt, Borland];
   let Documentation = [Undocumented];
 }
 
 def VectorSize : TypeAttr {
   let Spellings = [GCC<"vector_size">];

include/clang/Basic/Attributes.h

 
 class IdentifierInfo;
 
 enum class AttrSyntax {
   /// Is the identifier known as a GNU-style attribute?
   GNU,
   /// Is the identifier known as a __declspec-style attribute?
   Declspec,
+  /// Is the identifier known as a [] MS-style attribute?
+  MS,
   // Is the identifier known as a C++-style attribute?
   CXX,
   // Is the identifier known as a pragma attribute?
   Pragma
 };
 
 /// \brief Return the version number associated with the attribute if we
 /// recognize and implement the attribute specified by the given information.
 int hasAttribute(AttrSyntax Syntax, const IdentifierInfo *Scope,
                  const IdentifierInfo *Attr, const TargetInfo &Target,
                  const LangOptions &LangOpts);
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_BASIC_ATTRIBUTES_H

include/clang/Parse/Parser.h

void CheckMisplacedCXX11Attribute(ParsedAttributesWithRange &Attrs,
     if ((Tok.isNot(tok::l_square) || NextToken().isNot(tok::l_square)) &&
         Tok.isNot(tok::kw_alignas))
       return;
     DiagnoseMisplacedCXX11Attribute(Attrs, CorrectLocation);
   }
   void DiagnoseMisplacedCXX11Attribute(ParsedAttributesWithRange &Attrs,
                                        SourceLocation CorrectLocation);
 
-  void handleDeclspecAlignBeforeClassKey(ParsedAttributesWithRange &Attrs,
-                                         DeclSpec &DS, Sema::TagUseKind TUK);
+  void stripTypeAttribsOffDeclSpec(ParsedAttributesWithRange &Attrs,
 
   void ProhibitAttributes(ParsedAttributesWithRange &attrs) {
     if (!attrs.Range.isValid()) return;
     DiagnoseProhibitedAttributes(attrs);
     attrs.clear();
   }
   void DiagnoseProhibitedAttributes(ParsedAttributesWithRange &attrs);
 
   // Forbid C++11 attributes that appear on certain syntactic 
private:
 
   void MaybeParseMicrosoftAttributes(ParsedAttributes &attrs,
                                      SourceLocation *endLoc = nullptr) {
     if (getLangOpts().MicrosoftExt && Tok.is(tok::l_square))
       ParseMicrosoftAttributes(attrs, endLoc);
   }
   void ParseMicrosoftAttributes(ParsedAttributes &attrs,
                                 SourceLocation *endLoc = nullptr);
   void MaybeParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
                                     SourceLocation *End = nullptr) {
     const auto &LO = getLangOpts();
     if (LO.DeclSpecKeyword && Tok.is(tok::kw___declspec))
       ParseMicrosoftDeclSpecs(Attrs, End);
   }
   void ParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
                                SourceLocation *End = nullptr);
   bool ParseMicrosoftDeclSpecArgs(IdentifierInfo *AttrName,
                                   SourceLocation AttrNameLoc,
-                                  ParsedAttributes &Attrs);
+                                  ParsedAttributes &Attrs,
+                                  AttributeList::Syntax AS);
   void ParseMicrosoftTypeAttributes(ParsedAttributes &attrs);
   void DiagnoseAndSkipExtendedMicrosoftTypeAttributes();
   SourceLocation SkipExtendedMicrosoftTypeAttributes();
   void ParseMicrosoftInheritanceClassAttributes(ParsedAttributes &attrs);
   void ParseBorlandTypeAttributes(ParsedAttributes &attrs);
   void ParseOpenCLKernelAttributes(ParsedAttributes &attrs);
   void ParseOpenCLQualifiers(ParsedAttributes &Attrs);
   /// \brief Parses opencl_unroll_hint attribute if language is OpenCL v2.0

include/clang/Sema/AttributeList.h

public:
   /// The style used to specify an attribute.
   enum Syntax {
     /// __attribute__((...))
     AS_GNU,
     /// [[...]]
     AS_CXX11,
     /// __declspec(...)
     AS_Declspec,
+    /// [uuid("...")] class Foo
+    AS_MS,
     /// __ptr16, alignas(...), etc.
     AS_Keyword,
     /// Context-sensitive version of a keyword attribute.
     AS_ContextSensitiveKeyword,
     /// #pragma ...
-    AS_Pragma
+    AS_Pragma,
   };
 
 private:
   IdentifierInfo *AttrName;
   IdentifierInfo *ScopeName;
   SourceRange AttrRange;
   SourceLocation ScopeLoc;
   SourceLocation EllipsisLoc;
public:
   }
 
   bool isAlignasAttribute() const {
     // FIXME: Use a better mechanism to determine this.
     return getKind() == AT_Aligned && isKeywordAttribute();
   }
 
   bool isDeclspecAttribute() const { return SyntaxUsed == AS_Declspec; }
+  bool isMSAttribute() const { return SyntaxUsed == AS_MS; }
   bool isCXX11Attribute() const {
     return SyntaxUsed == AS_CXX11 || isAlignasAttribute();
   }
   bool isKeywordAttribute() const {
     return SyntaxUsed == AS_Keyword || SyntaxUsed == AS_ContextSensitiveKeyword;
   }
 
   bool isContextSensitiveKeywordAttribute() const {

lib/Parse/ParseDecl.cpp

void Parser::ParseGNUAttributeArgs(IdentifierInfo *AttrName,
   }
 
   ParseAttributeArgsCommon(AttrName, AttrNameLoc, Attrs, EndLoc, ScopeName,
                            ScopeLoc, Syntax);
 }
 
 bool Parser::ParseMicrosoftDeclSpecArgs(IdentifierInfo *AttrName,
                                         SourceLocation AttrNameLoc,
-                                        ParsedAttributes &Attrs) {
+                                        ParsedAttributes &Attrs,
+                                        AttributeList::Syntax AS) {
   // If the attribute isn't known, we will not attempt to parse any
   // arguments.
   if (!hasAttribute(AttrSyntax::Declspec, nullptr, AttrName,
                     getTargetInfo(), getLangOpts())) {
     // Eat the left paren, then skip to the ending right paren.
     ConsumeParen();
     SkipUntil(tok::r_paren);
     return false;
next_property_accessor:
       Diag(Tok.getLocation(), diag::err_ms_property_expected_comma_or_rparen);
       break;
     }
 
     // Only add the property attribute if it was well-formed.
     if (!HasInvalidAccessor)
       Attrs.addNewPropertyAttr(AttrName, AttrNameLoc, nullptr, SourceLocation(),
                                AccessorNames[AK_Get], AccessorNames[AK_Put],
-                               AttributeList::AS_Declspec);
+                               AS);
     T.skipToEnd();
     return !HasInvalidAccessor;
   }
 
-  unsigned NumArgs =
-      ParseAttributeArgsCommon(AttrName, AttrNameLoc, Attrs, nullptr, nullptr,
-                               SourceLocation(), AttributeList::AS_Declspec);
+  unsigned NumArgs = ParseAttributeArgsCommon(
+      AttrName, AttrNameLoc, Attrs, nullptr, nullptr, SourceLocation(), AS);
 
   // If this attribute's args were parsed, and it was expected to have
   // arguments but none were provided, emit a diagnostic.
   const AttributeList *Attr = Attrs.getList();
   if (Attr && Attr->getMaxArgs() && !NumArgs) {
     Diag(OpenParenLoc, diag::err_attribute_requires_arguments) << AttrName;
     return false;
   }
   return true;
while (Tok.isNot(tok::r_paren)) {
         AttrName = Tok.getIdentifierInfo();
         AttrNameLoc = ConsumeToken();
       }
 
       bool AttrHandled = false;
 
       // Parse attribute arguments.
       if (Tok.is(tok::l_paren))
-        AttrHandled = ParseMicrosoftDeclSpecArgs(AttrName, AttrNameLoc, Attrs);
+        AttrHandled = ParseMicrosoftDeclSpecArgs(AttrName, AttrNameLoc, Attrs,
+                                                 AttributeList::AS_Declspec);
       else if (AttrName->getName() == "property")
         // The property attribute must have an argument list.
         Diag(Tok.getLocation(), diag::err_expected_lparen_after)
             << AttrName->getName();
 
       if (!AttrHandled)
         Attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
                      AttributeList::AS_Declspec);
for (AttributeList *Attr = Attrs.getList(); Attr; Attr = Attr->getNext()) {
     else {
       Diag(Attr->getLoc(), DiagID)
         << Attr->getName();
       Attr->setInvalid();
     }
   }
 }
 
+// Usually, `__attribute__((attrib)) class Foo {} var` means that attrib applies
+// to var, not the type Foo.
 // As an exception to the rule, __declspec(align(...)) before the
 // class-key affects the type instead of the variable.
-void Parser::handleDeclspecAlignBeforeClassKey(ParsedAttributesWithRange &Attrs,
-                                               DeclSpec &DS,
-                                               Sema::TagUseKind TUK) {
+// Also, Microsoft-style [attributes] affect the type instead of the variable.
+// This function moves attributes that should apply to the type off DS to Attrs.
+void Parser::stripTypeAttribsOffDeclSpec(ParsedAttributesWithRange &Attrs,
+                                         DeclSpec &DS, Sema::TagUseKind TUK) {
   if (TUK == Sema::TUK_Reference)
     return;
 
   ParsedAttributes &PA = DS.getAttributes();
   AttributeList *AL = PA.getList();
   AttributeList *Prev = nullptr;
   while (AL) {
     AttributeList *Next = AL->getNext();
 
-    // We only consider attributes using the appropriate '__declspec' spelling.
-    // This behavior doesn't extend to any other spellings.
-    if (AL->getKind() == AttributeList::AT_Aligned &&
-        AL->isDeclspecAttribute()) {
+    if ((AL->getKind() == AttributeList::AT_Aligned &&
+         AL->isDeclspecAttribute()) ||
+        AL->isMSAttribute()) {
       // Stitch the attribute into the tag's attribute list.
       AL->setNext(nullptr);
       Attrs.add(AL);
 
       // Remove the attribute from the variable's attribute list.
       if (Prev) {
         // Set the last variable attribute's next attribute to be the attribute
         // after the current one.
void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS,
   if (!Name && TUK != Sema::TUK_Definition) {
     Diag(Tok, diag::err_enumerator_unnamed_no_def);
 
     // Skip the rest of this declarator, up until the comma or semicolon.
     SkipUntil(tok::comma, StopAtSemi);
     return;
   }
 
-  handleDeclspecAlignBeforeClassKey(attrs, DS, TUK);
+  stripTypeAttribsOffDeclSpec(attrs, DS, TUK);
 
   Sema::SkipBodyInfo SkipBody;
   if (!Name && TUK == Sema::TUK_Definition && Tok.is(tok::l_brace) &&
       NextToken().is(tok::identifier))
     SkipBody = Actions.shouldSkipAnonEnumBody(getCurScope(),
                                               NextToken().getIdentifierInfo(),
                                               NextToken().getLocation());
 

lib/Parse/ParseDeclCXX.cpp

void Parser::ParseInnerNamespace(std::vector<SourceLocation> &IdentLoc,
                                  unsigned int index, SourceLocation &InlineLoc,
                                  ParsedAttributes &attrs,
                                  BalancedDelimiterTracker &Tracker) {
   if (index == Ident.size()) {
     while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
            Tok.isNot(tok::eof)) {
       ParsedAttributesWithRange attrs(AttrFactory);
       MaybeParseCXX11Attributes(attrs);
-      MaybeParseMicrosoftAttributes(attrs);
       ParseExternalDeclaration(attrs);
     }
 
     // The caller is what called check -- we are simply calling
     // the close for it.
     Tracker.consumeClose();
 
     return;
Decl *LinkageSpec =
       Lang.isInvalid()
           ? nullptr
           : Actions.ActOnStartLinkageSpecification(
                 getCurScope(), DS.getSourceRange().getBegin(), Lang.get(),
                 Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   ParsedAttributesWithRange attrs(AttrFactory);
   MaybeParseCXX11Attributes(attrs);
-  MaybeParseMicrosoftAttributes(attrs);
 
   if (Tok.isNot(tok::l_brace)) {
     // Reset the source range in DS, as the leading "extern"
     // does not really belong to the inner declaration ...
     DS.SetRangeStart(SourceLocation());
     DS.SetRangeEnd(SourceLocation());
     // ... but anyway remember that such an "extern" was seen.
     DS.setExternInLinkageSpec(true);
while (true) {
 
     case tok::r_brace:
       if (!NestedModules)
         break;
       // Fall through.
     default:
       ParsedAttributesWithRange attrs(AttrFactory);
       MaybeParseCXX11Attributes(attrs);
-      MaybeParseMicrosoftAttributes(attrs);
       ParseExternalDeclaration(attrs);
       continue;
     }
 
     break;
   }
 
   T.consumeClose();
if (TemplateId) {
     // Don't pass down template parameter lists if this is just a tag
     // reference.  For example, we don't need the template parameters here:
     //   template <class T> class A *makeA(T t);
     MultiTemplateParamsArg TParams;
     if (TUK != Sema::TUK_Reference && TemplateParams)
       TParams =
         MultiTemplateParamsArg(&(*TemplateParams)[0], TemplateParams->size());
 
-    handleDeclspecAlignBeforeClassKey(attrs, DS, TUK);
+    stripTypeAttribsOffDeclSpec(attrs, DS, TUK);
 
     // Declaration or definition of a class type
     TagOrTempResult = Actions.ActOnTag(getCurScope(), TagType, TUK, StartLoc,
                                        SS, Name, NameLoc, attrs.getList(), AS,
                                        DS.getModulePrivateSpecLoc(),
                                        TParams, Owned, IsDependent,
                                        SourceLocation(), false,
                                        clang::TypeResult(),

 void Parser::ParseMicrosoftAttributes(ParsedAttributes &attrs,
                                       SourceLocation *endLoc) {
   assert(Tok.is(tok::l_square) && "Not a Microsoft attribute list");
 
   do {
     // FIXME: If this is actually a C++11 attribute, parse it as one.
     BalancedDelimiterTracker T(*this, tok::l_square);
     T.consumeOpen();
-    SkipUntil(tok::r_square, StopAtSemi | StopBeforeMatch);
+
+    // Skip most ms attributes except for a whitelist.
+    while (true) {
+      SkipUntil(tok::r_square, tok::identifier, StopAtSemi | StopBeforeMatch);
+      if (Tok.isNot(tok::identifier))  // ']', but also eof
+        break;
+      IdentifierInfo *Name = Tok.getIdentifierInfo();
+      SourceLocation Loc = Tok.getLocation();
+      ConsumeToken();
+      if (Name->getName() == "uuid")
+        ParseMicrosoftDeclSpecArgs(Name, Loc, attrs, AttributeList::AS_MS);
+    }
+
     T.consumeClose();
     if (endLoc)
       *endLoc = T.getCloseLocation();
   } while (Tok.is(tok::l_square));
 }
 
 void Parser::ParseMicrosoftIfExistsClassDeclaration(DeclSpec::TST TagType,
                                                     AccessSpecifier& CurAS) {
void Parser::ParseMicrosoftIfExistsClassDeclaration(DeclSpec::TST TagType,
   }
 
   while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
     // __if_exists, __if_not_exists can nest.
     if (Tok.isOneOf(tok::kw___if_exists, tok::kw___if_not_exists)) {
       ParseMicrosoftIfExistsClassDeclaration((DeclSpec::TST)TagType, CurAS);
       continue;
     }
 
     // Check for extraneous top-level semicolon.
     if (Tok.is(tok::semi)) {
       ConsumeExtraSemi(InsideStruct, TagType);
       continue;
     }
 
     AccessSpecifier AS = getAccessSpecifierIfPresent();
     if (AS != AS_none) {

lib/Parse/ParseObjc.cpp

Parser::ParseObjCAtImplementationDeclaration(SourceLocation AtLoc) {
 
   SmallVector<Decl *, 8> DeclsInGroup;
 
   {
     ObjCImplParsingDataRAII ObjCImplParsing(*this, ObjCImpDecl);
     while (!ObjCImplParsing.isFinished() && !isEofOrEom()) {
       ParsedAttributesWithRange attrs(AttrFactory);
       MaybeParseCXX11Attributes(attrs);
-      MaybeParseMicrosoftAttributes(attrs);
       if (DeclGroupPtrTy DGP = ParseExternalDeclaration(attrs)) {
         DeclGroupRef DG = DGP.get();
         DeclsInGroup.append(DG.begin(), DG.end());
       }
     }
   }
 
   return Actions.ActOnFinishObjCImplementation(ObjCImpDecl, DeclsInGroup);

lib/Parse/ParseOpenMP.cpp

case OMPD_declare_simd: {
     DeclGroupPtrTy Ptr;
     if (Tok.is(tok::annot_pragma_openmp))
       Ptr = ParseOpenMPDeclarativeDirectiveWithExtDecl(AS, Attrs, TagType, Tag);
     else if (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
       // Here we expect to see some function declaration.
       if (AS == AS_none) {
         assert(TagType == DeclSpec::TST_unspecified);
         MaybeParseCXX11Attributes(Attrs);
-        MaybeParseMicrosoftAttributes(Attrs);
         ParsingDeclSpec PDS(*this);
         Ptr = ParseExternalDeclaration(Attrs, &PDS);
       } else {
         Ptr =
             ParseCXXClassMemberDeclarationWithPragmas(AS, Attrs, TagType, Tag);
       }
     }
     if (!Ptr) {
case OMPD_declare_target: {
     if (!Actions.ActOnStartOpenMPDeclareTargetDirective(DTLoc))
       return DeclGroupPtrTy();
 
     DKind = ParseOpenMPDirectiveKind(*this);
     while (DKind != OMPD_end_declare_target && DKind != OMPD_declare_target &&
            Tok.isNot(tok::eof) && Tok.isNot(tok::r_brace)) {
       ParsedAttributesWithRange attrs(AttrFactory);
       MaybeParseCXX11Attributes(attrs);
-      MaybeParseMicrosoftAttributes(attrs);
       ParseExternalDeclaration(attrs);
       if (Tok.isAnnotation() && Tok.is(tok::annot_pragma_openmp)) {
         TentativeParsingAction TPA(*this);
         ConsumeToken();
         DKind = ParseOpenMPDirectiveKind(*this);
         if (DKind != OMPD_end_declare_target)
           TPA.Revert();
         else

lib/Parse/Parser.cpp

case tok::eof:
     return true;
 
   default:
     break;
   }
 
   ParsedAttributesWithRange attrs(AttrFactory);
   MaybeParseCXX11Attributes(attrs);
-  MaybeParseMicrosoftAttributes(attrs);
 
   Result = ParseExternalDeclaration(attrs);
   return false;
 }
 
 /// ParseExternalDeclaration:
 ///
 ///       external-declaration: [C99 6.9], declaration: [C++ dcl.dcl]
if (getLangOpts().CPlusPlus && Tok.is(tok::equal)) {
     const Token &KW = NextToken();
     return KW.is(tok::kw_default) || KW.is(tok::kw_delete);
   }
   
   return Tok.is(tok::colon) ||         // X() : Base() {} (used for ctors)
          Tok.is(tok::kw_try);          // X() try { ... }
 }
 
-/// ParseDeclarationOrFunctionDefinition - Parse either a function-definition or
-/// a declaration.  We can't tell which we have until we read up to the
-/// compound-statement in function-definition. TemplateParams, if
-/// non-NULL, provides the template parameters when we're parsing a
-/// C++ template-declaration.
+/// Parse either a function-definition or a declaration.  We can't tell which
+/// we have until we read up to the compound-statement in function-definition.
+/// TemplateParams, if non-NULL, provides the template parameters when we're
+/// parsing a C++ template-declaration.
 ///
 ///       function-definition: [C99 6.9.1]
 ///         decl-specs      declarator declaration-list[opt] compound-statement
 /// [C90] function-definition: [C99 6.7.1] - implicit int result
 /// [C90]   decl-specs[opt] declarator declaration-list[opt] compound-statement
 ///
 ///       declaration: [C99 6.7]
 ///         declaration-specifiers init-declarator-list[opt] ';'
 /// [!C99]  init-declarator-list ';'                   [TODO: warn in c99 mode]
 /// [OMP]   threadprivate-directive                              [TODO]
 ///
 Parser::DeclGroupPtrTy
 Parser::ParseDeclOrFunctionDefInternal(ParsedAttributesWithRange &attrs,
                                        ParsingDeclSpec &DS,
                                        AccessSpecifier AS) {
+  MaybeParseMicrosoftAttributes(DS.getAttributes());
   // Parse the common declaration-specifiers piece.
   ParseDeclarationSpecifiers(DS, ParsedTemplateInfo(), AS, DSC_top_level);
 
   // If we had a free-standing type definition with a missing semicolon, we
   // may get this far before the problem becomes obvious.
   if (DS.hasTagDefinition() &&
       DiagnoseMissingSemiAfterTagDefinition(DS, AS, DSC_top_level))
     return nullptr;
Parser::ParseDeclarationOrFunctionDefinition(ParsedAttributesWithRange &attrs,
   if (DS) {
     return ParseDeclOrFunctionDefInternal(attrs, *DS, AS);
   } else {
     ParsingDeclSpec PDS(*this);
     // Must temporarily exit the objective-c container scope for
     // parsing c constructs and re-enter objc container scope
     // afterwards.
     ObjCDeclContextSwitch ObjCDC(*this);
-      
     return ParseDeclOrFunctionDefInternal(attrs, PDS, AS);
   }
 }
 
 /// ParseFunctionDefinition - We parsed and verified that the specified
 /// Declarator is well formed.  If this is a K&R-style function, read the
 /// parameters declaration-list, then start the compound-statement.
 ///
case IEB_Skip:
     return;
   }
 
   // Parse the declarations.
   // FIXME: Support module import within __if_exists?
   while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
     ParsedAttributesWithRange attrs(AttrFactory);
     MaybeParseCXX11Attributes(attrs);
-    MaybeParseMicrosoftAttributes(attrs);
     DeclGroupPtrTy Result = ParseExternalDeclaration(attrs);
     if (Result && !getCurScope()->getParent())
       Actions.getASTConsumer().HandleTopLevelDecl(Result.get());
   }
   Braces.consumeClose();
 }
 
 /// Parse a C++ Modules TS module declaration, which appears at the beginning

test/Parser/MicrosoftExtensions.cpp

typedef struct _GUID
     unsigned char  Data4[8];
 } GUID;
 
 struct __declspec(uuid(L"00000000-0000-0000-1234-000000000047")) uuid_attr_bad1 { };// expected-error {{'uuid' attribute requires a string}}
 struct __declspec(uuid(3)) uuid_attr_bad2 { };// expected-error {{'uuid' attribute requires a string}}
 struct __declspec(uuid("0000000-0000-0000-1234-0000500000047")) uuid_attr_bad3 { };// expected-error {{uuid attribute contains a malformed GUID}}
 struct __declspec(uuid("0000000-0000-0000-Z234-000000000047")) uuid_attr_bad4 { };// expected-error {{uuid attribute contains a malformed GUID}}
 struct __declspec(uuid("000000000000-0000-1234-000000000047")) uuid_attr_bad5 { };// expected-error {{uuid attribute contains a malformed GUID}}
+[uuid("000000000000-0000-1234-000000000047")] struct uuid_attr_bad6 { };// expected-error {{uuid attribute contains a malformed GUID}}
 
 __declspec(uuid("000000A0-0000-0000-C000-000000000046")) int i; // expected-warning {{'uuid' attribute only applies to classes}}
 
 struct __declspec(uuid("000000A0-0000-0000-C000-000000000046"))
 struct_with_uuid { };
 struct struct_without_uuid { };
 
 struct __declspec(uuid("000000A0-0000-0000-C000-000000000049"))
 struct_with_uuid2;
 
+[uuid("000000A0-0000-0000-C000-000000000049")] struct struct_with_uuid3;
+
 struct
 struct_with_uuid2 {} ;
 
 int uuid_sema_test()
 {
    struct_with_uuid var_with_uuid[1];
    struct_without_uuid var_without_uuid[1];
 
    __uuidof(struct_with_uuid);
    __uuidof(struct_with_uuid2);
+   __uuidof(struct_with_uuid3);
    __uuidof(struct_without_uuid); // expected-error {{cannot call operator __uuidof on a type with no GUID}}
    __uuidof(struct_with_uuid*);
    __uuidof(struct_without_uuid*); // expected-error {{cannot call operator __uuidof on a type with no GUID}}
    __uuidof(struct_with_uuid[1]);
    __uuidof(struct_with_uuid*[1]); // expected-error {{cannot call operator __uuidof on a type with no GUID}}
    __uuidof(const struct_with_uuid[1][1]);
    __uuidof(const struct_with_uuid*[1][1]); // expected-error {{cannot call operator __uuidof on a type with no GUID}}
 

test/Sema/MicrosoftExtensions.c

 
 struct D {
    int l;
    int D[];
 };
 
 struct __declspec(uuid("00000000-0000-0000-C000-000000000046")) IUnknown {}; /* expected-error {{'uuid' attribute is not supported in C}} */
 
+[uuid("00000000-0000-0000-C000-000000000046")] struct IUnknown2 {}; /* expected-error {{'uuid' attribute is not supported in C}} */
+
 typedef struct notnested {
   long bad1;
   long bad2;
 } NOTNESTED;
 
 
 typedef struct nested1 {
   long a;