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 ↗

(On Diff #142516)

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 ↗

(On Diff #142516)

No space after *, please.

4564 ↗

(On Diff #142516)

{ on the previous line, please.

lib/AST/DeclCXX.cpp

1727 ↗

(On Diff #142516)

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 ↗

(On Diff #142516)

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 ↗

(On Diff #142516)

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

4937–4938 ↗

(On Diff #142516)

Do we still diagnose UUID mismatches somewhere else?

[zahiraam]

lib/Parse/ParseDecl.cpp

572 ↗

(On Diff #142516)

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

4937–4938 ↗

(On Diff #142516)

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

[zahiraam]

lib/Sema/SemaDeclAttr.cpp

4937–4938 ↗

(On Diff #142516)

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

393–394 ↗

(On Diff #191371)

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

398 ↗

(On Diff #191371)

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

399 ↗

(On Diff #191371)

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

588 ↗

(On Diff #191371)

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

589 ↗

(On Diff #191371)

This comment does not describe what this code does.

594 ↗

(On Diff #191371)

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

5371 ↗

(On Diff #191371)

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

5404–5409 ↗

(On Diff #191371)

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

332 ↗

(On Diff #191371)

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

594 ↗

(On Diff #191371)

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

4281 ↗

(On Diff #193258)

Can we store a StringLiteral* here instead?

include/clang/Sema/Sema.h

393–394 ↗

(On Diff #191371)

Comment marked done but not done?

399 ↗

(On Diff #191371)

Comment marked done but not done?

lib/Parse/ParseDecl.cpp

572 ↗

(On Diff #142516)

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.

594 ↗

(On Diff #191371)

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.

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

619 ↗

(On Diff #193258)

No.

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.

[zahiraam]

And this patch actually fixes the bug. Thanks.

[zahiraam]

A review please :-) Thanks.

[rsmith]

include/clang/AST/Decl.h

4303 ↗

(On Diff #200484)

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

408 ↗

(On Diff #200484)

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

1071–1073 ↗

(On Diff #200484)

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

lib/Sema/SemaDeclAttr.cpp

5405–5408 ↗

(On Diff #200484)

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

5452–5460 ↗

(On Diff #200484)

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

5471–5474 ↗

(On Diff #200484)

Please combine these lines.

5473–5474 ↗

(On Diff #200484)

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.

5476 ↗

(On Diff #200484)

(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

4303 ↗

(On Diff #200484)

Renamed it.

lib/CodeGen/CodeGenModule.cpp

1071–1073 ↗

(On Diff #200484)

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

1071–1073 ↗

(On Diff #200484)

Removed.

clang/include/clang/AST/Decl.h

 class TagDecl;
 class TemplateArgumentList;
 class TemplateArgumentListInfo;
 class TemplateParameterList;
 class TypeAliasTemplateDecl;
 class TypeLoc;
 class UnresolvedSetImpl;
 class VarTemplateDecl;
+class DeclSpecUuidDecl;
 
 /// A container of type source information.
 ///
 /// A client can read the relevant info using TypeLoc wrappers, e.g:
 /// @code
 /// TypeLoc TL = TypeSourceInfo->getTypeLoc();
 /// TL.getBeginLoc().print(OS, SrcMgr);
 /// @endcode
public:
   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 , public Redeclarable<DeclSpecUuidDecl> {
+  StringLiteral *Uuid;
+
+public:
+  static DeclSpecUuidDecl *Create(const ASTContext &C, DeclContext *DC,
+                                  SourceLocation IdLoc, StringLiteral *uuid);
+
+  static DeclSpecUuidDecl *CreateDeserialized(ASTContext &C, unsigned ID);
+
+  static bool classof(const Decl *D) { return classofKind(D->getKind()); }
+  static bool classofKind(Kind K) { return K == DeclSpecUuid; }
+
+  DeclSpecUuidDecl(Kind DK, const ASTContext &C, DeclContext *DC,
+       SourceLocation IdLoc, StringLiteral *uuid)
+      : Decl (DK, DC, IdLoc), Uuid(uuid), redeclarable_base(C) {}
+
+  using redeclarable_base = Redeclarable<DeclSpecUuidDecl>;
+
+  DeclSpecUuidDecl *getMostRecentDecl() {
+    return cast_or_null<DeclSpecUuidDecl>(
+        static_cast<DeclSpecUuidDecl *>(this)->getMostRecentDecl());
+  }
+
+  DeclSpecUuidDecl *getPreviousDecl() {
+    return cast_or_null<DeclSpecUuidDecl>(
+        static_cast<DeclSpecUuidDecl *>(this)->getPreviousDecl());
+  }
+
+   StringLiteral *getUuid() { return Uuid; }
+ };
+
 /// 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);
   return DB;
 }

clang/include/clang/AST/RecursiveASTVisitor.h

 
 DEF_TRAVERSE_DECL(CapturedDecl, {
   TRY_TO(TraverseStmt(D->getBody()));
   ShouldVisitChildren = false;
 })
 
 DEF_TRAVERSE_DECL(EmptyDecl, {})
 
+DEF_TRAVERSE_DECL(DeclSpecUuidDecl, {})
+
 DEF_TRAVERSE_DECL(FileScopeAsmDecl,
                   { TRY_TO(TraverseStmt(D->getAsmString())); })
 
 DEF_TRAVERSE_DECL(ImportDecl, {})
 
 DEF_TRAVERSE_DECL(FriendDecl, {
   // Friend is either decl or a type.
   if (D->getFriendType())

clang/include/clang/Basic/Attr.td

class NamedArgument<string name, bit opt = 0, bit fake = 0> : Argument<name,
                                                                           opt,
                                                                           fake>;
 class TypeArgument<string name, bit opt = 0> : Argument<name, opt>;
 class UnsignedArgument<string name, bit opt = 0> : Argument<name, opt>;
 class VariadicUnsignedArgument<string name> : Argument<name, 1>;
 class VariadicExprArgument<string name> : Argument<name, 1>;
 class VariadicStringArgument<string name> : Argument<name, 1>;
 class VariadicIdentifierArgument<string name> : Argument<name, 1>;
+class DeclSpecUuidDeclArgument<string name, bit opt = 0> : Argument<name,opt>;
 
 // Like VariadicUnsignedArgument except values are ParamIdx.
 class VariadicParamIdxArgument<string name> : Argument<name, 1>;
 
 // A list of identifiers matching parameters or ParamIdx indices.
 class VariadicParamOrParamIdxArgument<string name> : Argument<name, 1>;
 
 // Like VariadicParamIdxArgument but for a single function parameter index.

 def Used : InheritableAttr {
   let Spellings = [GCC<"used">];
   let Subjects = SubjectList<[NonLocalVar, Function, ObjCMethod]>;
   let Documentation = [Undocumented];
 }
 
 def Uuid : InheritableAttr {
   let Spellings = [Declspec<"uuid">, Microsoft<"uuid">];
-  let Args = [StringArgument<"Guid">];
+  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];
 }
 
 def VectorSize : TypeAttr {

clang/include/clang/Basic/DeclNodes.td

 def Block : Decl<"blocks">, DeclContext;
 def Captured : Decl, DeclContext;
 def ClassScopeFunctionSpecialization : Decl;
 def Import : Decl;
 def OMPThreadPrivate : Decl;
 def OMPAllocate : Decl;
 def OMPRequires : Decl;
 def Empty : Decl;
-
+def DeclSpecUuid : Decl;
 No newline at end of file

clang/include/clang/Sema/Sema.h

AvailabilityAttr *mergeAvailabilityAttr(
       unsigned AttrSpellingListIndex);
   TypeVisibilityAttr *mergeTypeVisibilityAttr(Decl *D, SourceRange Range,
                                        TypeVisibilityAttr::VisibilityType Vis,
                                               unsigned AttrSpellingListIndex);
   VisibilityAttr *mergeVisibilityAttr(Decl *D, SourceRange Range,
                                       VisibilityAttr::VisibilityType Vis,
                                       unsigned AttrSpellingListIndex);
   UuidAttr *mergeUuidAttr(Decl *D, SourceRange Range,
-                          unsigned AttrSpellingListIndex, StringRef Uuid);
+                          unsigned AttrSpellingListIndex, DeclSpecUuidDecl *Uuid);
   DLLImportAttr *mergeDLLImportAttr(Decl *D, SourceRange Range,
                                     unsigned AttrSpellingListIndex);
   DLLExportAttr *mergeDLLExportAttr(Decl *D, SourceRange Range,
                                     unsigned AttrSpellingListIndex);
   MSInheritanceAttr *
   mergeMSInheritanceAttr(Decl *D, SourceRange Range, bool BestCase,
                          unsigned AttrSpellingListIndex,
                          MSInheritanceAttr::Spelling SemanticSpelling);

clang/lib/AST/Decl.cpp

 }
 
 VarDecl *VarDecl::CreateDeserialized(ASTContext &C, unsigned ID) {
   return new (C, ID)
       VarDecl(Var, C, nullptr, SourceLocation(), SourceLocation(), nullptr,
               QualType(), nullptr, SC_None);
 }
 
+ //===----------------------------------------------------------------------===//
+ // UuidDeclSpec Implementation
+ //===----------------------------------------------------------------------===//
+DeclSpecUuidDecl *DeclSpecUuidDecl::Create(const ASTContext &C, DeclContext *DC,
+                                           SourceLocation IdLoc,
+                                           StringLiteral *uuid) {
+  return new (C, DC) DeclSpecUuidDecl(DeclSpecUuid, C, DC, IdLoc, uuid);
+}
+
 void VarDecl::setStorageClass(StorageClass SC) {
   assert(isLegalForVariable(SC));
   VarDeclBits.SClass = SC;
 }
 
 VarDecl::TLSKind VarDecl::getTLSKind() const {
   switch (VarDeclBits.TSCSpec) {
   case TSCS_unspecified:

clang/lib/AST/DeclBase.cpp

switch (DeclKind) {
     case ObjCImplementation:
     case ObjCCategory:
     case ObjCCategoryImpl:
     case Import:
     case OMPThreadPrivate:
     case OMPAllocate:
     case OMPRequires:
     case OMPCapturedExpr:
+    case DeclSpecUuid:
     case Empty:
       // Never looked up by name.
       return 0;
   }
 
   llvm_unreachable("Invalid DeclKind!");
 }
 

clang/lib/AST/DeclCXX.cpp

bool CXXRecordDecl::isInterfaceLike() const {
   // Check "Special" types.
   const 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->getGuid() == "00000000-0000-0000-C000-000000000046") ||
+        Uuid->getDeclSpecUuidDecl()->getUuid()->getString() ==
+            "00000000-0000-0000-C000-000000000046") ||
        (getName() == "IDispatch" &&
-        Uuid->getGuid() == "00020400-0000-0000-C000-000000000046"))) {
+        Uuid->getDeclSpecUuidDecl()->getUuid()->getString() ==
+            "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.
 

clang/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitDecl(const Decl &D) {
   case Decl::StaticAssert: // static_assert(X, ""); [C++0x]
   case Decl::Label:        // __label__ x;
   case Decl::Import:
   case Decl::OMPThreadPrivate:
   case Decl::OMPAllocate:
   case Decl::OMPCapturedExpr:
   case Decl::OMPRequires:
   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++]

clang/lib/Sema/SemaDecl.cpp

else if (isa<AlignedAttr>(Attr))
     // such attributes on a declaration at the same time.
     NewAttr = nullptr;
   else if ((isa<DeprecatedAttr>(Attr) || isa<UnavailableAttr>(Attr)) &&
            (AMK == Sema::AMK_Override ||
             AMK == Sema::AMK_ProtocolImplementation))
     NewAttr = nullptr;
   else if (const auto *UA = dyn_cast<UuidAttr>(Attr))
     NewAttr = S.mergeUuidAttr(D, UA->getRange(), AttrSpellingListIndex,
-                              UA->getGuid());
+                              UA->getDeclSpecUuidDecl());
   else if (const auto *SLHA = dyn_cast<SpeculativeLoadHardeningAttr>(Attr))
     NewAttr = S.mergeSpeculativeLoadHardeningAttr(D, *SLHA);
   else if (const auto *SLHA = dyn_cast<NoSpeculativeLoadHardeningAttr>(Attr))
     NewAttr = S.mergeNoSpeculativeLoadHardeningAttr(D, *SLHA);
   else if (Attr->shouldInheritEvenIfAlreadyPresent() || !DeclHasAttr(D, Attr))
     NewAttr = cast<InheritableAttr>(Attr->clone(S.Context));
 
   if (NewAttr) {

clang/lib/Sema/SemaDeclAttr.cpp

D->addAttr(::new (S.Context)
                                      AL.getAttributeSpellingListIndex()));
 }
 
 //===----------------------------------------------------------------------===//
 // Microsoft specific attribute handlers.
 //===----------------------------------------------------------------------===//
 
 UuidAttr *Sema::mergeUuidAttr(Decl *D, SourceRange Range,
-                              unsigned AttrSpellingListIndex, StringRef Uuid) {
-  if (const auto *UA = D->getAttr<UuidAttr>()) {
-    if (UA->getGuid().equals_lower(Uuid))
+                              unsigned AttrSpellingListIndex,
+                              DeclSpecUuidDecl *Uuid) {
+  if (auto *UA = D->getAttr<UuidAttr>()) {
+    if (declaresSameEntity(UA->getDeclSpecUuidDecl(), Uuid))
       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);
 }
if (i == 8 || i == 13 || i == 18 || i == 23) {
         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;
     }
   }
+  StringLiteral *Uuid = StringLiteral::Create(
+      S.getASTContext(), StrRef, StringLiteral::Ascii, /*Pascal*/ false,
+      AL.getArgAsExpr(0)->getType(), LiteralLoc);
 
   // 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;
+  ArgDecl = DeclSpecUuidDecl::Create(S.getASTContext(),
+             S.getFunctionLevelDeclContext(),
+             SourceLocation(), Uuid);
+  // Chain the DeclSpecUuids that have the same Uuid.
+  if (auto *UA = D->getAttr<UuidAttr>()) {
+    ArgDecl->setPreviousDecl(UA->getDeclSpecUuidDecl());
+  }
+
   UuidAttr *UA = S.mergeUuidAttr(D, AL.getRange(),
-                                 AL.getAttributeSpellingListIndex(), StrRef);
+                                 AL.getAttributeSpellingListIndex(), ArgDecl);
   if (UA)
     D->addAttr(UA);
 }
 
 static void handleMSInheritanceAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!S.LangOpts.CPlusPlus) {
     S.Diag(AL.getLoc(), diag::err_attribute_not_supported_in_lang)
         << AL << AttributeLangSupport::C;

clang/lib/Sema/SemaExprCXX.cpp

ExprResult Sema::BuildCXXUuidof(QualType TypeInfoType,
   StringRef UuidStr;
   if (!Operand->getType()->isDependentType()) {
     llvm::SmallSetVector<const UuidAttr *, 1> UuidAttrs;
     getUuidAttrOfType(*this, Operand->getType(), UuidAttrs);
     if (UuidAttrs.empty())
       return ExprError(Diag(TypeidLoc, diag::err_uuidof_without_guid));
     if (UuidAttrs.size() > 1)
       return ExprError(Diag(TypeidLoc, diag::err_uuidof_with_multiple_guids));
-    UuidStr = UuidAttrs.back()->getGuid();
+    UuidStr = UuidAttrs.back()->getDeclSpecUuidDecl()->getUuid()->getString();
   }
 
   return new (Context) CXXUuidofExpr(TypeInfoType.withConst(), Operand, UuidStr,
                                      SourceRange(TypeidLoc, RParenLoc));
 }
 
 /// Build a Microsoft __uuidof expression with an expression operand.
 ExprResult Sema::BuildCXXUuidof(QualType TypeInfoType,
                                 SourceLocation TypeidLoc,
                                 Expr *E,
                                 SourceLocation RParenLoc) {
   StringRef UuidStr;
   if (!E->getType()->isDependentType()) {
     if (E->isNullPointerConstant(Context, Expr::NPC_ValueDependentIsNull)) {
       UuidStr = "00000000-0000-0000-0000-000000000000";
     } else {
       llvm::SmallSetVector<const UuidAttr *, 1> UuidAttrs;
       getUuidAttrOfType(*this, E->getType(), UuidAttrs);
       if (UuidAttrs.empty())
         return ExprError(Diag(TypeidLoc, diag::err_uuidof_without_guid));
       if (UuidAttrs.size() > 1)
         return ExprError(Diag(TypeidLoc, diag::err_uuidof_with_multiple_guids));
-      UuidStr = UuidAttrs.back()->getGuid();
+      UuidStr = UuidAttrs.back()->getDeclSpecUuidDecl()->getUuid()->getString();
+      if (UuidStr.startswith("{") && UuidStr.endswith("}"))
+        UuidStr = UuidStr.drop_front().drop_back();
     }
   }
 
   return new (Context) CXXUuidofExpr(TypeInfoType.withConst(), E, UuidStr,
                                      SourceRange(TypeidLoc, RParenLoc));
 }
 
 /// ActOnCXXUuidof - Parse __uuidof( type-id ) or __uuidof (expression);

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

= NamespaceAliasDecl::Create(SemaRef.Context, Owner,
                                  D->getIdentifier(),
                                  D->getQualifierLoc(),
                                  D->getTargetNameLoc(),
                                  D->getNamespace());
   Owner->addDecl(Inst);
   return Inst;
 }
 
+Decl *
+TemplateDeclInstantiator::VisitDeclSpecUuidDecl(DeclSpecUuidDecl *D) {
+   llvm_unreachable("DeclSpecUuidDecl cannot be instantiated");
+}
+
 Decl *TemplateDeclInstantiator::InstantiateTypedefNameDecl(TypedefNameDecl *D,
                                                            bool IsTypeAlias) {
   bool Invalid = false;
   TypeSourceInfo *DI = D->getTypeSourceInfo();
   if (DI->getType()->isInstantiationDependentType() ||
       DI->getType()->isVariablyModifiedType()) {
     DI = SemaRef.SubstType(DI, TemplateArgs,
                            D->getLocation(), D->getDeclName());

clang/test/Sema/ms-uuid-1.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions -fms-compatibility -std=c++14 %s
+// expected-no-diagnostics
+typedef struct _GUID {
+  int i;
+} IID;
+template <const IID *piid>
+class A {};
+
+struct
+    __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
+        S1 {};
+
+struct
+    __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
+        S2 {};
+
+struct __declspec(dllexport)
+    C1 : public A<&__uuidof(S1)> {};
+
+struct __declspec(dllexport)
+    C2 : public A<&__uuidof(S2)> {};
+int printf(const char *, ...);
+int main() {
+
+  if (&__uuidof(S1) == &__uuidof(S2))
+    printf("OK\n");
+  else
+    printf("ERROR\n");
+
+  return 0;
+}

clang/test/Sema/ms-uuid-2.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions -fms-compatibility -std=c++14  %s
+
+
+typedef struct _GUID
+{
+    unsigned long  Data1;
+    unsigned short Data2;
+    unsigned short Data3;
+    unsigned char  Data4[8];
+} GUID;
+
+// expected-error@+5 {{C++ requires a type specifier for all declarations}}
+// expected-error@+4 {{invalid digit 'a' in decimal constant}}
+// expected-error@+3 {{use of undeclared identifier 'def0'}}
+// expected-error@+2 {{invalid digit 'a' in decimal constant}}
+// expected-error@+1 {{expected ';' after top level declarator}}
+uuid(12345678-9abc-def0-1234-56789abcdef0) struct S2;

clang/utils/TableGen/ClangAttrEmitter.cpp

void writeValue(raw_ostream &OS) const override {
         // Some non-optional (comma required) identifier arguments can be the
         // empty string but are then recorded as a nullptr.
         OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
            << "()->getName() : \"\") << \"";
       else if (type == "TypeSourceInfo *")
         OS << "\" << get" << getUpperName() << "().getAsString() << \"";
       else if (type == "ParamIdx")
         OS << "\" << get" << getUpperName() << "().getSourceIndex() << \"";
-      else
+      else if (type == "DeclSpecUuidDecl *") {
+        OS << "\" << get" << getUpperName() << "() << \"";
+      } else
         OS << "\" << get" << getUpperName() << "() << \"";
     }
 
     void writeDump(raw_ostream &OS) const override {
-      if (type == "FunctionDecl *" || type == "NamedDecl *") {
+      if (type == "FunctionDecl *" || type == "NamedDecl *" ||
+          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";
createArgument(const Record &Arg, StringRef Attr,
   else if (ArgName == "VariadicParamOrParamIdxArgument")
     Ptr = llvm::make_unique<VariadicParamOrParamIdxArgument>(Arg, Attr);
   else if (ArgName == "ParamIdxArgument")
     Ptr = llvm::make_unique<SimpleArgument>(Arg, Attr, "ParamIdx");
   else if (ArgName == "VariadicIdentifierArgument")
     Ptr = llvm::make_unique<VariadicIdentifierArgument>(Arg, Attr);
   else if (ArgName == "VersionArgument")
     Ptr = llvm::make_unique<VersionArgument>(Arg, Attr);
+  else if (ArgName == "DeclSpecUuidDeclArgument")
+    Ptr = llvm::make_unique<SimpleArgument>(Arg, Attr, "DeclSpecUuidDecl *");
 
   if (!Ptr) {
     // Search in reverse order so that the most-derived type is handled first.
     ArrayRef<std::pair<Record*, SMRange>> Bases = Search->getSuperClasses();
     for (const auto &Base : llvm::reverse(Bases)) {
       if ((Ptr = createArgument(Arg, Attr, Base.first)))
         break;
     }