Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

[zahiraam]

Summary

In this (partial) test case:
...
struct declspec(dllexport) C1 : public A <&uuidof(S1)>
{};
struct declspec(dllexport) C2 : public A<&uuidof(S2)>
{};
...

clang should be generating a single instantiation of A. It is instead generating two of them. This happens only in the presence of uuid.

Compiling this with clang this test case (dllexport with no uuid):
template <const char* s> class A {};
char s = 'a';
struct declspec(dllexport) C1 : public A <&s>
{};
struct declspec(dllexport) C2 : public A<&s>
{};

has the same(correct) behavior than MSVC:

ksh-3.2$ dumpbin /symbols t3.o | grep "class A"
008 00000000 SECT4 notype () External | ??4?$A@$1?s@@3DA@@QEAAAEAV0@AEBV0@@Z (public: class A<&char s> & cdecl A<&char s>::operator=(class A<&char s> const &))
00D 00000000 SECT5 notype () External | ??4?$A@$1?s@@3DA@@QEAAAEAV0@$$QEAV0@@Z (public: class A<&char s> & cdecl A<&char s>::operator=(class A<&char s> &&))
ksh-3.2$

The solution proposed in this patch is the generation a new template argument for uuid.

Let me know if you agree.

[zahiraam]

[majnemer]

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

[erichkeane]

It seems to me that the test here is very much lacking. It doesn't seem to include the example you've mentioned, and has no validation to ensure that there is a single instantiation happening. I'd like to see what happens when a UUID is passed as a pack, how SFINAE works on it, etc.

include/clang/AST/RecursiveASTVisitor.h

846 ↗

(On Diff #135250)

No need for curly brackets.

891 ↗

(On Diff #135250)

Curly brackets likely not necessary here.

include/clang/AST/TemplateBase.h

214 ↗

(On Diff #135250)

This function overload should be documented.

lib/AST/ASTContext.cpp

5066 ↗

(On Diff #135250)

Not only for you here, but is there any reason why this cannot just be a fallthrough for the ones above? I suspect we'd prefer to get those 3 all combined.

lib/AST/MicrosoftMangle.cpp

1426 ↗

(On Diff #135250)

If you combine the getAsUuidExpr line and the mangleExpession line, you can get rid of curlies, and be more consistent with the surrounding code.

lib/AST/TemplateBase.cpp

581 ↗

(On Diff #135250)

Why is much of this required? Wouldn't just calling printPretty with the current policy work? Why use a separate stream rather than the 'DB' stream?

lib/Sema/SemaTemplate.cpp

4629 ↗

(On Diff #135250)

Is this section clang-formatted? It seems a little oddly newlined.

[zahiraam]

Adding test case.

[zahiraam]

In D43576#1016295, @majnemer wrote:

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

Not sure I fully understand what you are proposing?

Are you proposing that generated symbols like this:
??4?$A@$E?_GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3@@3U__s_GUID@@B@@QEAAAEAV0@AEBV0@@Z
Be de-sugared? Wouldn't that be different that what MS is doing?
Can you please give me more details about what you are thinking of?
Thanks.

[majnemer]

In D43576#1016418, @zahiraam wrote:

In D43576#1016295, @majnemer wrote:

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

Not sure I fully understand what you are proposing?

Are you proposing that generated symbols like this:
??4?$A@$E?_GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3@@3U__s_GUID@@B@@QEAAAEAV0@AEBV0@@Z
Be de-sugared? Wouldn't that be different that what MS is doing?
Can you please give me more details about what you are thinking of?
Thanks.

We create an AST such that the following:

struct _GUID;

template <const _GUID *>
class A {};

struct __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}")) S {};

struct __declspec(dllexport) C : public A<&__uuidof(S)> {};

is turned into something like:

struct _GUID;

__declspec(selectany) __s_GUID const _GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3 = {...};

template <const _GUID *>
class A {};

struct __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}")) S {};

struct __declspec(dllexport) C : public A<&_GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3> {};

[rsmith]

Do we need to also track whether the argument is a pointer or reference to a UUID (and also the cv-qualifiers)? For the Declaration case, we track this by tracking the corresponding parameter type; the same thing would presumably work here.

In D43576#1016295, @majnemer wrote:

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

This desugaring approach is not how we generally do things in Clang. The fact that MS exposes a variable that can be named from user code is, in my opinion, simply a bug in their implementation -- their implementation details are leaking -- and not part of the actual semantics here. I view this as exactly analogous to typeid (which would have exactly the same problems if its result could be used as a non-type template parameter); as with typeid, __uuidof notionally produces a global object not corresponding to any variable. If we want to model this as a declaration, we could add a new Decl subclass for these uuid objects (and eventually also for objects produced by typeid). But I don't think we should model them as variables unless that's actually part of their intended semantics.

[majnemer]

In D43576#1016512, @rsmith wrote:

Do we need to also track whether the argument is a pointer or reference to a UUID (and also the cv-qualifiers)? For the Declaration case, we track this by tracking the corresponding parameter type; the same thing would presumably work here.

In D43576#1016295, @majnemer wrote:

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

This desugaring approach is not how we generally do things in Clang. The fact that MS exposes a variable that can be named from user code is, in my opinion, simply a bug in their implementation -- their implementation details are leaking -- and not part of the actual semantics here. I view this as exactly analogous to typeid (which would have exactly the same problems if its result could be used as a non-type template parameter); as with typeid, __uuidof notionally produces a global object not corresponding to any variable. If we want to model this as a declaration, we could add a new Decl subclass for these uuid objects (and eventually also for objects produced by typeid). But I don't think we should model them as variables unless that's actually part of their intended semantics.

Here's my thinking: the __uuidof expression literally declares a variable called _GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3 of type __s_GUID which is why it behaves the way it does: https://godbolt.org/g/74FY7U

I don't think it is reasonable to invent new semantics which are different from the MSVC ones because we find the MSVC ones inelegant. The huge upside I see from matching their behavior is that the implementation is dramatically simpler, we have a considerable amount of mumbo jumbo in the template instantiation code to handle __uuidof correctly and it looks like we will need some more.

What is the relative upside to a new kind of Decl? Better AST fidelity?

[rsmith]

In D43576#1016561, @majnemer wrote:

Here's my thinking: the __uuidof expression literally declares a variable called _GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3 of type __s_GUID which is why it behaves the way it does: https://godbolt.org/g/74FY7U

This is an implementation detail leaking, though. no? Note that that is a reserved name.

I don't think it is reasonable to invent new semantics which are different from the MSVC ones because we find the MSVC ones inelegant.

I mostly agree, but my point is that this is *not* the MSVC semantics, it's merely an implementation detail that non-conforming code happens to be able to observe. Suppose that type_info objects were similarly accessible in MSVC by guessing their mangled names. Would you be arguing that we should inject variables for those too? (And note that it is *nearly* true that type_info objects work that way: https://godbolt.org/g/zByFFg -- but the parser gets confused somehow when you reference them.) The only difference I can see between these cases is that the reserved name used for the GUID case happens to not contain any ?s and @s, so happens to be utterable as an identifier.

We should not attempt to be compatible with the cases where MSVC's implementation details happen to leak into user-visible semantics.

What is the relative upside to a new kind of Decl? Better AST fidelity?

Yes, exactly. The same reason we don't desguar other things any more than we have to.

[zahiraam]

In D43576#1016588, @rsmith wrote:

In D43576#1016561, @majnemer wrote:

Here's my thinking: the __uuidof expression literally declares a variable called _GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3 of type __s_GUID which is why it behaves the way it does: https://godbolt.org/g/74FY7U

This is an implementation detail leaking, though. no? Note that that is a reserved name.

I don't think it is reasonable to invent new semantics which are different from the MSVC ones because we find the MSVC ones inelegant.

I mostly agree, but my point is that this is *not* the MSVC semantics, it's merely an implementation detail that non-conforming code happens to be able to observe. Suppose that type_info objects were similarly accessible in MSVC by guessing their mangled names. Would you be arguing that we should inject variables for those too? (And note that it is *nearly* true that type_info objects work that way: https://godbolt.org/g/zByFFg -- but the parser gets confused somehow when you reference them.) The only difference I can see between these cases is that the reserved name used for the GUID case happens to not contain any ?s and @s, so happens to be utterable as an identifier.

We should not attempt to be compatible with the cases where MSVC's implementation details happen to leak into user-visible semantics.

What is the relative upside to a new kind of Decl? Better AST fidelity?

Yes, exactly. The same reason we don't desguar other things any more than we have to.

Before I start implementing this I want to make sure that we are on the same page on this.
From your comments it looks like we do not want to create global variables for the uuid globals. I tend to agree with that. I do like the idea of creating a new Decl type for the uuid declspec. But I want to make sure that I understand what you are referring to.
Currently this declaration:
struct
__declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
S1;

a CXXRecordDecl type is generated with attributes (the uuid being an attribute). Are you proposing that instead a new type is generated for S1 say something like CXXUuidRecord? And create also a new TagType that corresponds to this new Decl?

[rsmith]

In D43576#1019703, @zahiraam wrote:

Currently this declaration:
struct
__declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
S1;

a CXXRecordDecl type is generated with attributes (the uuid being an attribute). Are you proposing that instead a new type is generated for S1 say something like CXXUuidRecord? And create also a new TagType that corresponds to this new Decl?

No. Concretely, I'd suggest we create a new UuidDecl object representing the _GUID object. An instance of UuidDecl would be created and owned by the uuid attribute, and __uuidof(X) would denote the UuidDecl owned by the uuid attribute on the CXXRecordDecl. We'd also need some way to form redeclaration chains for UuidDecls (which means we'll need a map from UUID to UuidDecl somewhere, perhaps on the ASTContext).

[zahiraam]

In D43576#1016295, @majnemer wrote:

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

Not sure I fully understand what you are proposing?

Are you proposing that generated symbols like this:
??4?$A@$E?_GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3@@3U__s_GUID@@B@@QEAAAEAV0@AEBV0@@Z
Be de-sugared? Wouldn't that be different that what MS is doing?
Can you please give me more details about what you are thinking of?
Thanks.

In D43576#1019943, @rsmith wrote:

In D43576#1019703, @zahiraam wrote:

Currently this declaration:
struct
__declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
S1;

a CXXRecordDecl type is generated with attributes (the uuid being an attribute). Are you proposing that instead a new type is generated for S1 say something like CXXUuidRecord? And create also a new TagType that corresponds to this new Decl?

No. Concretely, I'd suggest we create a new UuidDecl object representing the _GUID object. An instance of UuidDecl would be created and owned by the uuid attribute, and __uuidof(X) would denote the UuidDecl owned by the uuid attribute on the CXXRecordDecl. We'd also need some way to form redeclaration chains for UuidDecls (which means we'll need a map from UUID to UuidDecl somewhere, perhaps on the ASTContext).

In D43576#1019943, @rsmith wrote:

In D43576#1019703, @zahiraam wrote:

Currently this declaration:
struct
__declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
S1;

a CXXRecordDecl type is generated with attributes (the uuid being an attribute). Are you proposing that instead a new type is generated for S1 say something like CXXUuidRecord? And create also a new TagType that corresponds to this new Decl?

No. Concretely, I'd suggest we create a new UuidDecl object representing the _GUID object. An instance of UuidDecl would be created and owned by the uuid attribute, and __uuidof(X) would denote the UuidDecl owned by the uuid attribute on the CXXRecordDecl. We'd also need some way to form redeclaration chains for UuidDecls (which means we'll need a map from UUID to UuidDecl somewhere, perhaps on the ASTContext).

As per Richard comment. I have added a UuidDecl owned by the uuid attribute. This patch concerns only this change. It is not a patch to fix the bug yet. I want to make sure that we are in agreement with my changes first. Then I will change the code that deal with ParseCXUuid.
Comments are welcome.
Thanks.

[rsmith]

include/clang/Sema/AttributeList.h

601

You're not storing a DeclSpecUuidDecl within the attribute, so you don't need to reserve enough space for one here. In fact, this constant appears to be unused, so you can just remove it.

lib/AST/Decl.cpp

4561

No space after *, please.

4564

{ on the previous line, please.

lib/AST/DeclCXX.cpp

1727

It would be nice to have a convenience accessor on UuidAttr to get the UUID as a string. (You can add one using AdditionalMembers in Attr.td.)

lib/Parse/ParseDecl.cpp

568–583

The Parser should not create Decls (that's a layering violation). There are two ways to handle this:

1. Add a Sema method to act on a UUID attribute string, and return a pointer that you can stash in the Attrs list.
2. Just store a string in the Attrs list, and move the code to convert the string into a DeclSpecUuidDecl* and store that on the UuidAttr into Sema.

I'd strongly prefer the second option; from the point of view of the parser, we can still just treat this as an attribute that takes a string, and we can do the string -> DeclSpecUuidDecl handling entirely within the semantic analysis for the attribute (in Sema).

572

What should happen if multiple declarations (of distinct entities) use the same __declspec(uuid(...)) attribute? Should you get a redefinition error for the attribute or should they all share the same UUID entity? Either way, we'll want to do some (minimal) UUID lookup and build a redeclaration chain for DeclSpecUuidDecls.

lib/Sema/SemaDeclAttr.cpp

4932–4933

Do we still diagnose UUID mismatches somewhere else?

[zahiraam]

lib/Parse/ParseDecl.cpp

572

If we want to align with MS, we don't want to signal an error. So may be I should have a map that assigns to each StrUuid a list of DeclSpecUuid?
So for this code:
struct
__declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
S1 {};

struct
__declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
S2 {};

I will have a map from DDB47A6A-0F23-11D5-9109-00E0296B75D3 to {S1, S2}. Do you agree?

lib/Sema/SemaDeclAttr.cpp

4932–4933

Not as far as I know. I guess I should put this diag somewhere?

[zahiraam]

lib/Sema/SemaDeclAttr.cpp

4932–4933

Fixed the initial code.

[zahiraam]

Richard,
Please let me know if I have answered to all the issues you raised. Thanks.

[zahiraam]

It would be nice to have a review for this year old (updated) patch. Thanks.

[rnk]

I see! I only took a quick look, but this looks exactly like what @rsmith has been asking for in discussions for a long time now: a more explicit AST representation of uuid of uuidof in template arguments. I'll make an effort to get his attention and see if this addresses his concerns.

[rsmith]

Thanks, this looks like a good broad direction.

I think this patch does not goes far enough yet. Broad comments (partially duplicating some of the specific comments on the patch itself):

• form the DeclSpecUuidDecl earlier, when parsing the attribute argument
• store a pointer to the DeclSpecUuidDecl on a non-dependent CXXUuidofExpr
• remove the maps in Sema, and instead ask attributes and uuidof expressions for their declarations
• DeclSpecUuidDecl creation needs to either deduplicate declarations with the same UUID or form redeclaration chains for such declarations
• we are missing at least serialization and deserialization code for the new declaration kind (and likely other things too: ASTImporter support, for instance)
• we should be able to emit DeclSpecUuidDecls in CodeGen, replacing our current ad-hoc emission of UUID globals

include/clang/Sema/ParsedAttr.h

220 ↗

(On Diff #191371)

I don't really like adding new special kinds of argument representation in ParsedAttr, but... this really is a very special kind of argument, with rules different from anything else.

Have you considered forming the DeclSpecUuidDecl object earlier (when the argument is parsed) and storing it here? That'd be a closer match for how we handle other forms of attribute arguments (particularly expressions).

564 ↗

(On Diff #191371)

Copy-paste error in assert message

include/clang/Sema/Sema.h

353–354

Remove this? It is only used once, and only to initialize an unused variable.

358

Use an llvm::StringMap here? But I'm also not convinced this should exist at all.

359

This seems suspicious. If we want the uuid of a declaration, should we not be asking it for its UuidAttr instead?

lib/Parse/ParseDecl.cpp

558

This code is dead: note that we returned on the line before.

559

This comment does not describe what this code does.

564

How does this handle the case where multiple tokens must be concatenated to form a uuid? Eg, uuid(12345678-9abc-def0-1234-56789abcdef0)

lib/Sema/SemaDeclAttr.cpp

4935

We should be comparing to see if the declarations are the same here (use declaresSameEntity on the UUID decls).

4963–4968

Sema should not be dealing with lexical issues like this. This should have been handled when parsing the attribute argument. (Also this is missing handling for other encoding prefixes and suffixes.)

lib/Sema/SemaExprCXX.cpp

636 ↗

(On Diff #191371)

This variable is unused. (And it's not clear why we would assume that the most-recently-created DeclSpecUuidDecl would correspond to anything relevant here.)

664–670 ↗

(On Diff #191371)

I think this should be finding the UuidAttr of the declaration and asking it for its DeclSpecUuidDecl.

675–678 ↗

(On Diff #191371)

We should store a pointer to the UUID declaration on a non-dependent CXXUuidofExpr.

lib/Sema/SemaTemplateInstantiateDecl.cpp

554–558 ↗

(On Diff #191371)

Just use llvm_unreachable here; DeclSpecUuidDecls are global and shouldn't be instantiated.

test/Parser/MicrosoftExtensions.cpp

79 ↗

(On Diff #191371)

Why is this error now appearing on the wrong line?

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

22–23 ↗

(On Diff #191371)

This is a diagnostic quality regression. The attribute does not require a string. But it does not accept a string with an encoding prefix.

utils/TableGen/ClangAttrEmitter.cpp

333

Nit: no need for parens here, and bad indentation.

[zahiraam]

There are still a few things I haven't addressed yet. Mostly because not sure there is another solution like getting rid of the map from StringRef to expr. The other issue is not adding new kind to ParsedAttr. There may be another way of doing it, but didn't look at it yet.
Meanwhile can you please let me know if you are happy with the current status of the implementation.

lib/Parse/ParseDecl.cpp

564

ksh-3.2$ cat m3.cpp
struct declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;
ksh-3.2$ clang -c m3.cpp
m3.cpp:1:35: error: invalid digit 'a' in decimal constant
struct declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;

^

m3.cpp:1:39: error: use of undeclared identifier 'def0'
struct __declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;

^

m3.cpp:1:54: error: invalid digit 'a' in decimal constant
struct __declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;

^

3 errors generated.
ksh-3.2$

[rsmith]

include/clang/AST/Decl.h

4243

Can we store a StringLiteral* here instead?

include/clang/Sema/Sema.h

353–354

Comment marked done but not done?

359

Comment marked done but not done?

lib/Parse/ParseDecl.cpp

564

This case is accepted by MSVC and not here:

struct __declspec(uuid(R"(12345678-9abc-def0-1234-56789abcdef0)")) s;

(MSVC also accepts a u8 prefix, but no other encoding prefix. I also note that MSVC does not support string literal concatenation here, so we don't need to deal with that.)

Please use StringLiteralParser to properly handle cases like this, rather than rolling your own string literal parsing here.

572

I think you should have a map from UUID to either the most recent DeclSpecUuidDecl for that UUID, and form redeclaration chains for DeclSpecUuidDecls with the same UUID. (DeclSpecUuidDecl should derive from Redeclarable<DeclSpecUuidDecl> and you can call setPreviousDecl to connect the new declaration to the prior one.)

You'll need to make sure this case is properly handled during lazy AST deserialization.

lib/Sema/SemaExprCXX.cpp

619 ↗

(On Diff #193258)

Do we need both this and getUuidAttrOfType?

635 ↗

(On Diff #193258)

It would seem cleaner to collect the UuidAttrDecls or UuidAttrs here rather than the declarations that have those attributes.

647–648 ↗

(On Diff #193258)

Dead code.

675–681 ↗

(On Diff #193258)

Revert this change; we need to build a new CXXUuidofExpr with a correct source location and type rather than reusing one from somewhere else. Then remove UuidExpMap.

[zahiraam]

@rsmith I think I have made all the changes you have pointed out to. But please note that this new patch only impements an explicit AST representation of uuid in template arguments. It does not fix the bug for which this review was opened for.
I will take care of the bug in time. But before doing that I want to make sure that the changes required to give an explicit AST for uuid is correct.

Thanks for taking the time to review. And sorry my response is slow to come. This work is done in my "spare" time.

lib/Sema/SemaExprCXX.cpp

675–678 ↗

(On Diff #191371)

Not sure what that means.

675–678 ↗

(On Diff #191371)

However the test case is still failing. Still need to find a solution for the fail.

619 ↗

(On Diff #193258)

No.

[zahiraam]

And this patch actually fixes the bug. Thanks.

[zahiraam]

A review please :-) Thanks.

[rsmith]

include/clang/AST/Decl.h

4257

What does "STL" mean here?

include/clang/Sema/ParsedAttr.h

337–346 ↗

(On Diff #200484)

I don't think we need a custom constructor for this any more; a StringLiteral is an Expr, so can be stored as a regular argument. It's also suspicious that this constructor doesn't use its stluuid parameter -- maybe it's already unused?

822 ↗

(On Diff #200484)

(Likewise we shouldn't need this any more...)

1031 ↗

(On Diff #200484)

(... or this.)

include/clang/Sema/Sema.h

353

This map is not necessary (you can get the DeclSpecUuidDecl from the UuidAttr on the Decl) and not correct (it's not serialized and deserialized). Please remove it.

lib/CodeGen/CodeGenModule.cpp

987–989

Was this supposed to be included in this patch? It looks like this is papering over a bug elsewhere.

lib/Sema/SemaDeclAttr.cpp

4935–4938

This if should be:

if (declaresSameEntity(UA->getDeclSpecUuidDecl(), Uuid))

(Not a string comparison. We already combined UuidDecls with the same string into the same entity.)

4981–4989

You already have a suitable string literal as AL.getArgAsExpr(0). There's no need to fabricate another one here.

4990–4993

Please combine these lines.

4992–4993

You should check for a prior DeclSpecUuidDecl with the same UUID here and set it as the previous declaration of the new one, so that they're treated as declaring the same entity.

4995

(Remove this; with the other change I suggested above this should be unnecessary.)

[zahiraam]

Thanks for the review.
I think and hope that I have responded to every issue you raised. Let me know if there are still pending issues.
Happy 4th!

include/clang/AST/Decl.h

4257

Renamed it.

lib/CodeGen/CodeGenModule.cpp

987–989

This is the code that actually fixes the bug. The rest of the patch is to represent uuid in the AST.

987–989

Removed.

include/clang/AST/Decl.h

static EmptyDecl *Create(ASTContext &C, DeclContext *DC,
                            SourceLocation L);
   static EmptyDecl *CreateDeserialized(ASTContext &C, unsigned ID);
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == Empty; }
 };
 
 class DeclSpecUuidDecl : public Decl {
+private:
   StringRef StrUuid;
+  DeclSpecUuidDecl *Next;
+  DeclSpecUuidDecl *Previous;
 public:
   static DeclSpecUuidDecl *Create(const ASTContext &C, DeclContext *DC,
                                   SourceLocation IdLoc,
                                   StringRef UuidVal);
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == DeclSpecUuid; }
 
   DeclSpecUuidDecl(Kind DK, DeclContext *DC, SourceLocation IdLoc, StringRef UuidVal)
-    : Decl (DK, DC, IdLoc), StrUuid(UuidVal) {}
+    : Decl (DK, DC, IdLoc), StrUuid(UuidVal), Next(nullptr), Previous(nullptr) {}
 
   StringRef getStrUuid() { return StrUuid; }
+  void setNext(DeclSpecUuidDecl *n) { Next = n; }
+  DeclSpecUuidDecl *getNext() { return Next; }
+  void setPrevious(DeclSpecUuidDecl *n) { Previous = n; }
+  DeclSpecUuidDecl *getPrevious() { return Previous; }
 };
 
 /// Insertion operator for diagnostics.  This allows sending NamedDecl's
 /// into a diagnostic with <<.
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            const NamedDecl* ND) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(ND),
                   DiagnosticsEngine::ak_nameddecl);

include/clang/Basic/Attr.td

 def Uuid : InheritableAttr {
   let Spellings = [Declspec<"uuid">, Microsoft<"uuid">];
   let Args = [DeclSpecUuidDeclArgument<"DeclSpecUuidDecl">];
   let Subjects = SubjectList<[Record, Enum]>;
   // FIXME: Allow expressing logical AND for LangOpts. Our condition should be:
   // CPlusPlus && (MicrosoftExt || Borland)
   let LangOpts = [MicrosoftExt, Borland];
   let Documentation = [Undocumented];
+  let AdditionalMembers = [{
+    StringRef getUuidAsStr() { return getDeclSpecUuidDecl()->getStrUuid(); }
+  }];
+
 }
 
 def VectorSize : TypeAttr {
   let Spellings = [GCC<"vector_size">];
   let Args = [ExprArgument<"NumBytes">];
   let Documentation = [Undocumented];
 }
 

include/clang/Sema/AttributeList.h

private:
   mutable unsigned ProcessingCache : 8;
 
   /// \brief The location of the 'unavailable' keyword in an
   /// availability attribute.
   SourceLocation UnavailableLoc;
   
   const Expr *MessageExpr;
 
-  DeclSpecUuidDecl *UuidDecl;
+  StringRef StrUuid;
 
   /// The next attribute in the current position.
   AttributeList *NextInPosition = nullptr;
 
   /// The next attribute allocated in the current Pool.
   AttributeList *NextInPool = nullptr;
 
   /// Arguments, if any, are stored immediately following the object.
AttributeList(IdentifierInfo *attrName, SourceRange attrRange,
         HasProcessingCache(false) {
     new (&getPropertyDataBuffer()) PropertyData(getterId, setterId);
     AttrKind = getKind(getName(), getScopeName(), syntaxUsed);
   }
 
   /// Constructor for __declspec(uuid) attribute.
   AttributeList(IdentifierInfo *attrName, SourceRange attrRange,
     IdentifierInfo *scopeName, SourceLocation scopeLoc,
-    DeclSpecUuidDecl *uuid, AttributeList::Syntax syntaxUsed)
+    StringRef struuid, AttributeList::Syntax syntaxUsed)
     : AttrName(attrName), ScopeName(scopeName), AttrRange(attrRange),
       ScopeLoc(scopeLoc), Invalid(false), HasParsedType(false),
-      SyntaxUsed(syntaxUsed), NumArgs(1), UuidDecl(uuid) {
+      SyntaxUsed(syntaxUsed), NumArgs(1), StrUuid(struuid) {
     AttrKind = getKind(getName(), getScopeName(), syntaxUsed);
   }
 
   /// Type tag information is stored immediately following the arguments, if
   /// any, at the end of the object.  They are mutually exclusive with
   /// availability slots.
   TypeTagForDatatypeData &getTypeTagForDatatypeDataSlot() {
     return *reinterpret_cast<TypeTagForDatatypeData*>(getArgsBuffer()+NumArgs);
const ParsedType &getTypeArg() const {
     return getTypeBuffer();
   }
 
   const PropertyData &getPropertyData() const {
     assert(isDeclspecPropertyAttribute() && "Not a __delcspec(property) attribute");
     return getPropertyDataBuffer();
   }
 
-  DeclSpecUuidDecl *getUuidDecl() const {
-    assert(getKind() == AT_Uuid && "Not an availability attribute");
-    return UuidDecl;
+  StringRef getUuidStr() const {
+   return StrUuid;
   }
 
   /// \brief Get an index into the attribute spelling list
   /// defined in Attr.td. This index is used by an attribute
   /// to pretty print itself.
   unsigned getAttributeSpellingListIndex() const;
 
   bool isTargetSpecificAttr() const;
enum {
     TypeTagForDatatypeAllocSize =
       sizeof(AttributeList)
       + (sizeof(AttributeList::TypeTagForDatatypeData) + sizeof(void *) +
          sizeof(ArgsUnion) - 1)
         / sizeof(void*) * sizeof(void*),
     PropertyAllocSize =
       sizeof(AttributeList)
       + (sizeof(AttributeList::PropertyData) + sizeof(void *) - 1)
-        / sizeof(void*) * sizeof(void*),
-    UuidDeclSpecSize =
-      sizeof(AttributeList)
-      + (sizeof(DeclSpecUuidDecl) + sizeof(void *) + sizeof(ArgsUnion) - 1)
         / sizeof(void*) * sizeof(void*)
   };
 
 private:
   enum {
     /// The number of free lists we want to be sure to support
     /// inline.  This is just enough that availability attributes
     /// don't surpass it.  It's actually very unlikely we'll see an
     /// attribute that needs more than that; on x86-64 you'd need 10
     /// expression arguments, and on i386 you'd need 19.
     InlineFreeListsCapacity =
       1 + (AvailabilityAllocSize - sizeof(AttributeList)) / sizeof(void*)
return add(new (memory) AttributeList(attrName, attrRange,
                                           scopeName, scopeLoc,
                                           getterId, setterId,
                                           syntaxUsed));
   }
 
   AttributeList *
   createUuidDeclSpecAttribute(IdentifierInfo *attrName, SourceRange attrRange,
                               IdentifierInfo *scopeName, SourceLocation scopeLoc,
-                              DeclSpecUuidDecl *ArgDecl, AttributeList::Syntax syntaxUsed) {
+                              StringRef struuid, AttributeList::Syntax syntaxUsed) {
     void *memory = allocate(sizeof(AttributeList));
     return add(new (memory) AttributeList(attrName, attrRange,
-                                          scopeName, scopeLoc, ArgDecl, syntaxUsed));
+                                          scopeName, scopeLoc, struuid, syntaxUsed));
   }
 };
 
 /// ParsedAttributes - A collection of parsed attributes.  Currently
 /// we don't differentiate between the various attribute syntaxes,
 /// which is basically silly.
 ///
 /// Right now this is a very lightweight container, but the expectation
addNewPropertyAttr(IdentifierInfo *attrName, SourceRange attrRange,
     add(attr);
     return attr;
   }
 
   /// Add microsoft __delspec(uuid) attribute.
   AttributeList *
   addNew(IdentifierInfo *attrName, SourceRange attrRange,
    IdentifierInfo *scopeName, SourceLocation scopeLoc,
-   DeclSpecUuidDecl *ArgDecl, AttributeList::Syntax syntaxUsed) {
+   StringRef struuid, AttributeList::Syntax syntaxUsed) {
     AttributeList *attr =
       pool.createUuidDeclSpecAttribute(attrName, attrRange, scopeName,
-               scopeLoc, ArgDecl, syntaxUsed);
+               scopeLoc, struuid, syntaxUsed);
     add(attr);
     return attr;
   }
 
 private:
   mutable AttributePool pool;
   AttributeList *list = nullptr;
 };

include/clang/Sema/Sema.h

LangOptions::PragmaMSPointersToMembersKind
       MSPointerToMemberRepresentationMethod;
 
   /// Stack of active SEH __finally scopes.  Can be empty.
   SmallVector<Scope*, 2> CurrentSEHFinally;
 
   /// \brief Source location for newly created implicit MSInheritanceAttrs
   SourceLocation ImplicitMSInheritanceAttrLoc;
 
+  std::map<StringRef, DeclSpecUuidDecl*> UuidDeclSpecMap;
+
   /// \brief pragma clang section kind
   enum PragmaClangSectionKind {
     PCSK_Invalid      = 0,
     PCSK_BSS          = 1,
     PCSK_Data         = 2,
     PCSK_Rodata       = 3,
     PCSK_Text         = 4
    };
 
   enum PragmaClangSectionAction {
     PCSA_Set     = 0,
     PCSA_Clear   = 1
   };

lib/AST/Decl.cpp

 ExportDecl *ExportDecl::CreateDeserialized(ASTContext &C, unsigned ID) {
   return new (C, ID) ExportDecl(nullptr, SourceLocation());
 }
 
 //===----------------------------------------------------------------------===//
 // UuidDeclSpec Implementation
 //===----------------------------------------------------------------------===//
 
-DeclSpecUuidDecl * DeclSpecUuidDecl::Create(const ASTContext &C, DeclContext *DC,
-                                            SourceLocation IdLoc,
-                                            StringRef UuidVal)
-{
+DeclSpecUuidDecl *DeclSpecUuidDecl::Create(const ASTContext &C, DeclContext *DC,
+                                           StringRef UuidVal) {
   return new (C, DC) DeclSpecUuidDecl(DeclSpecUuid, DC, IdLoc, UuidVal);
 }

lib/AST/DeclCXX.cpp

if (isLambda() || hasUserDeclaredConstructor() ||
     return false;
 
   // No interface-like type can have a method with a definition.
   for (const auto *const Method : methods())
     if (Method->isDefined() && !Method->isImplicit())
       return false;
 
   // Check "Special" types.
-  const auto *Uuid = getAttr<UuidAttr>();
+  auto *Uuid = getAttr<UuidAttr>();
   // MS SDK declares IUnknown/IDispatch both in the root of a TU, or in an
   // extern C++ block directly in the TU.  These are only valid if in one
   // of these two situations.
   if (Uuid && isStruct() && !getDeclContext()->isExternCContext() &&
       !isDeclContextInNamespace(getDeclContext()) &&
       ((getName() == "IUnknown" &&
-        Uuid->getDeclSpecUuidDecl()->getStrUuid() == "00000000-0000-0000-C000-000000000046") ||
+  Uuid->UuidAttr::getUuidAsStr() == "00000000-0000-0000-C000-000000000046") ||
        (getName() == "IDispatch" &&
-        Uuid->getDeclSpecUuidDecl()->getStrUuid() == "00020400-0000-0000-C000-000000000046"))) {
+  Uuid->UuidAttr::getUuidAsStr() == "00020400-0000-0000-C000-000000000046"))) {
     if (getNumBases() > 0)
       return false;
     return true;
   }
 
   // FIXME: Any access specifiers is supposed to make this no longer interface
   // like.
 

lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitDecl(const Decl &D) {
   case Decl::EnumConstant: // enum ? { X = ? }
   case Decl::CXXRecord: // struct/union/class X; [C++]
   case Decl::StaticAssert: // static_assert(X, ""); [C++0x]
   case Decl::Label:        // __label__ x;
   case Decl::Import:
   case Decl::OMPThreadPrivate:
   case Decl::OMPCapturedExpr:
   case Decl::Empty:
+  case Decl::DeclSpecUuid:
     // None of these decls require codegen support.
     return;
 
   case Decl::NamespaceAlias:
     if (CGDebugInfo *DI = getDebugInfo())
         DI->EmitNamespaceAlias(cast<NamespaceAliasDecl>(D));
     return;
   case Decl::Using:          // using X; [C++]

lib/CodeGen/CodeGenModule.cpp

if (!getTarget().getCXXABI().hasConstructorVariants()) {
         CanonicalGD = GlobalDecl(CD, Ctor_Complete);
     }
   }
 
   auto FoundName = MangledDeclNames.find(CanonicalGD);
   if (FoundName != MangledDeclNames.end())
     return FoundName->second;
 
-
   // Keep the first result in the case of a mangling collision.
   const auto *ND = cast<NamedDecl>(GD.getDecl());
   auto Result =
       Manglings.insert(std::make_pair(getMangledNameImpl(*this, GD, ND), GD));
   return MangledDeclNames[CanonicalGD] = Result.first->first();
 }
 
 StringRef CodeGenModule::getBlockMangledName(GlobalDecl GD,
                                              const BlockDecl *BD) {
   MangleContext &MangleCtx = getCXXABI().getMangleContext();
   const Decl *D = GD.getDecl();
 
   SmallString<256> Buffer;

lib/Parse/ParseDecl.cpp

if (AttrName->getName() == "property") {
 
     // 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);
     T.skipToEnd();
     return !HasInvalidAccessor;
   }
   if (AttrName->getName() == "uuid") {
     // Parse the uuid attribute and create a UuidDecl.
     ConsumeParen();
     assert(Tok.is(tok::string_literal) && "uuid not followed by string literal '('");
-    int sz = Tok.getLength();
     SmallString<8> UuidBuffer;
     bool Invalid = false;
     StringRef UuidStr = PP.getSpelling(Tok, UuidBuffer, &Invalid);
 
     // Clean up the string from the "\" at begining and at end.
     StringRef UuidStr1 = UuidStr.ltrim('\"');
     StringRef TrimmedUuidStr = UuidStr1.rtrim('\"');
-    DeclSpecUuidDecl *ArgDecl =
-      DeclSpecUuidDecl::Create(Actions.getASTContext(),
-                               Actions.getFunctionLevelDeclContext(),
-                               SourceLocation(),
-                               TrimmedUuidStr);
 
     // Advance to next token. Should be a r-paren.
     PP.Lex(Tok);
     SourceLocation RParen = Tok.getLocation();
     SourceRange attrRange = SourceRange(AttrNameLoc, RParen);
     if (!ExpectAndConsume(tok::r_paren))
-      Attrs.addNew(AttrName, attrRange, nullptr, AttrNameLoc, ArgDecl, AttributeList::AS_Declspec);
+      Attrs.addNew(AttrName, attrRange, nullptr, AttrNameLoc, TrimmedUuidStr, AttributeList::AS_Declspec);
     return true;
   }
   unsigned NumArgs =
       ParseAttributeArgsCommon(AttrName, AttrNameLoc, Attrs, nullptr, nullptr,
                                SourceLocation(), AttributeList::AS_Declspec);
 
   // 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;
 }
void Parser::ParseAvailabilityAttribute(IdentifierInfo &Availability,
                                         ParsedAttributes &attrs,
                                         SourceLocation *endLoc,
                                         IdentifierInfo *ScopeName,
                                         SourceLocation ScopeLoc,
                                         AttributeList::Syntax Syntax) {
   enum { Introduced, Deprecated, Obsoleted, Unknown };
   AvailabilityChange Changes[Unknown];
   ExprResult MessageExpr, ReplacementExpr;
-  Decl *DeclSpecUuidDecl = nullptr;
 
   // Opening '('.
   BalancedDelimiterTracker T(*this, tok::l_paren);
   if (T.consumeOpen()) {
     Diag(Tok, diag::err_expected) << tok::l_paren;
     return;
   }
 

lib/Sema/SemaDeclAttr.cpp

D->addAttr(::new (S.Context)
              ObjCPreciseLifetimeAttr(AL.getRange(), S.Context,
                                      AL.getAttributeSpellingListIndex()));
 }
 
 //===----------------------------------------------------------------------===//
 // Microsoft specific attribute handlers.
 //===----------------------------------------------------------------------===//
 
 UuidAttr *Sema::mergeUuidAttr(Decl *D, SourceRange Range,
                               unsigned AttrSpellingListIndex,
                               DeclSpecUuidDecl *Uuid) {
-  if (D->getAttr<UuidAttr>())
+  if (const auto *UA = D->getAttr<UuidAttr>()) {
+    if (UA->getDeclSpecUuidDecl()->getStrUuid().equals_lower(Uuid->getStrUuid()))
       return nullptr;
+    Diag(UA->getLocation(), diag::err_mismatched_uuid);
+    Diag(Range.getBegin(), diag::note_previous_uuid);
+    D->dropAttr<UuidAttr>();
+  }
 
   return ::new (Context) UuidAttr(Range, Context, Uuid, AttrSpellingListIndex);
 }
 
 static void handleUuidAttr(Sema &S, Decl *D, const AttributeList &AL) {
   if (!S.LangOpts.CPlusPlus) {
     S.Diag(AL.getLoc(), diag::err_attribute_not_supported_in_lang)
       << AL.getName() << AttributeLangSupport::C;
     return;
   }
 
-  StringRef  StrRef = AL.getUuidDecl()->getStrUuid();
+  StringRef  StrRef = AL.getUuidStr();
   SourceLocation LiteralLoc = AL.getLoc();
   if (StrRef.empty())
     return;
 
   // GUID format is "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX" or
   // "{XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX}", normalize to the former.
   if (StrRef.size() == 38 && StrRef.front() == '{' && StrRef.back() == '}')
     StrRef = StrRef.drop_front().drop_back();
 
   // Validate GUID length.
   if (StrRef.size() != 36) {
     S.Diag(LiteralLoc, diag::err_attribute_uuid_malformed_guid);
     return;
   }
 
   for (unsigned i = 0; i < 36; ++i) {
     if (i == 8 || i == 13 || i == 18 || i == 23) {
       if (StrRef[i] != '-') {
         S.Diag(LiteralLoc, diag::err_attribute_uuid_malformed_guid);
         return;
       }
     } else if (!isHexDigit(StrRef[i])) {
       S.Diag(LiteralLoc, diag::err_attribute_uuid_malformed_guid);
       return;
     }
   }
 
   // FIXME: It'd be nice to also emit a fixit removing uuid(...) (and, if it's
   // the only thing in the [] list, the [] too), and add an insertion of
   // __declspec(uuid(...)).  But sadly, neither the SourceLocs of the commas
   // separating attributes nor of the [ and the ] are in the AST.
   // Cf "SourceLocations of attribute list delimiters - [[ ... , ... ]] etc"
   // on cfe-dev.
   if (AL.isMicrosoftAttribute()) // Check for [uuid(...)] spelling.
     S.Diag(AL.getLoc(), diag::warn_atl_uuid_deprecated);
 
+  DeclSpecUuidDecl *ArgDecl, *PreviousArgDecl;
+  ArgDecl = DeclSpecUuidDecl::Create(S.getASTContext(),
+             S.getFunctionLevelDeclContext(),
+             SourceLocation(),
+             StrRef);
+
+  // Do a lookup of the declspec.
+  auto DSU = S.UuidDeclSpecMap.find(StrRef);
+  if (DSU != S.UuidDeclSpecMap.end()) {
+    PreviousArgDecl  = DSU->second;
+    PreviousArgDecl->setNext(ArgDecl);
+    ArgDecl->setPrevious(PreviousArgDecl);
+  }
+  S.UuidDeclSpecMap[StrRef] = ArgDecl;
+
   UuidAttr *UA = S.mergeUuidAttr(D, AL.getRange(),
          AL.getAttributeSpellingListIndex(),
-         AL.getUuidDecl());
+         ArgDecl);
 
   if (UA)
     D->addAttr(UA);
+
 }
 
 static void handleMSInheritanceAttr(Sema &S, Decl *D, const AttributeList &AL) {
   if (!S.LangOpts.CPlusPlus) {
     S.Diag(AL.getLoc(), diag::err_attribute_not_supported_in_lang)
       << AL.getName() << AttributeLangSupport::C;
     return;
   }

lib/Serialization/ASTCommon.cpp

bool serialization::isRedeclarableDeclKind(unsigned Kind) {
   case Decl::Var:
   case Decl::FunctionTemplate:
   case Decl::ClassTemplate:
   case Decl::VarTemplate:
   case Decl::TypeAliasTemplate:
   case Decl::ObjCProtocol:
   case Decl::ObjCInterface:
   case Decl::Empty:
+  case Decl::DeclSpecUuid:
     return true;
 
   // Never redeclarable.
   case Decl::UsingDirective:
   case Decl::Label:
   case Decl::UnresolvedUsingTypename:
   case Decl::TemplateTypeParm:
   case Decl::EnumConstant:

utils/TableGen/ClangAttrEmitter.cpp

void writeValue(raw_ostream &OS) const override {
       else if (type == "DeclSpecUuidDecl *") {
         OS << "\" << get" << getUpperName() << "() << \"";
   } else
         OS << "\" << get" << getUpperName() << "() << \"";
     }
 
     void writeDump(raw_ostream &OS) const override {
       if (type == "FunctionDecl *" || type == "NamedDecl *" ||
-    (type == "DeclSpecUuidDecl *")) {
+          type == "DeclSpecUuidDecl *") {
         OS << "    OS << \" \";\n";
         OS << "    dumpBareDeclRef(SA->get" << getUpperName() << "());\n"; 
       } else if (type == "IdentifierInfo *") {
         // Some non-optional (comma required) identifier arguments can be the
         // empty string but are then recorded as a nullptr.
         OS << "    if (SA->get" << getUpperName() << "())\n"
            << "      OS << \" \" << SA->get" << getUpperName()
            << "()->getName();\n";