[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/Decl.h:4281
+ class DeclSpecUuidDecl : public Decl {
+   StringRef StrUuid;
+ public:
----------------
Can we store a `StringLiteral*` here instead?


================
Comment at: include/clang/Sema/Sema.h:393-394
 
+  /// List of declspec(uuid ...) for a specific uuid string.
+  SmallVector<Decl *, 1> DeclSpecUuidDecls;
+
----------------
rsmith wrote:
> Remove this? It is only used once, and only to initialize an unused variable.
Comment marked done but not done?


================
Comment at: include/clang/Sema/Sema.h:399
+  std::map<StringRef, ExprResult> UuidExpMap;
+  std::map<const Decl*, StringRef> DeclSpecToStrUuid;
+
----------------
rsmith wrote:
> This seems suspicious. If we want the uuid of a declaration, should we not be asking it for its `UuidAttr` instead?
Comment marked done but not done?


================
Comment at: lib/Parse/ParseDecl.cpp:572
+    DeclSpecUuidDecl *ArgDecl =
+      DeclSpecUuidDecl::Create(Actions.getASTContext(),
+                               Actions.getFunctionLevelDeclContext(),
----------------
zahiraam wrote:
> rsmith wrote:
> > 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 `DeclSpecUuidDecl`s.
> 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?
> 
I think you should have a map from UUID to either the most recent `DeclSpecUuidDecl` for that UUID, and form redeclaration chains for `DeclSpecUuidDecl`s 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.


================
Comment at: lib/Parse/ParseDecl.cpp:594
+      bool Invalid = false;
+      StringRef UuidStr = PP.getSpelling(Tok, UuidBuffer, &Invalid);
+
----------------
zahiraam wrote:
> rsmith wrote:
> > How does this handle the case where multiple tokens must be concatenated to form a uuid? Eg, `uuid(12345678-9abc-def0-1234-56789abcdef0)`
> 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$
> 
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.


================
Comment at: lib/Sema/SemaExprCXX.cpp:619
+ /// Finds the __declSpec uuid Decl off a type.
+ static void FindADeclOffAType(Sema &SemaRef,
+                               QualType QT,
----------------
Do we need both this and getUuidAttrOfType?


================
Comment at: lib/Sema/SemaExprCXX.cpp:635
+   if (TD->getMostRecentDecl()->getAttr<UuidAttr>()) {
+     UuidDecl.push_back(dcl);
+     return;
----------------
It would seem cleaner to collect the `UuidAttrDecl`s or `UuidAttr`s here rather than the declarations that have those attributes.


================
Comment at: lib/Sema/SemaExprCXX.cpp:647-648
+         FindADeclOffAType(SemaRef, TA.getAsType(), UuidDecl);
+       if (dd)
+         UuidDecl.push_back(dcl);
+     }
----------------
Dead code.


================
Comment at: lib/Sema/SemaExprCXX.cpp:675-681
+   if (expr.isUnset()) {
+     uuid_expr =
+       new (Context) CXXUuidofExpr(TypeInfoType.withConst(), Operand, UuidStr,
+         SourceRange(TypeidLoc, RParenLoc));
+     UuidExpMap[UuidStr] = uuid_expr;
+     return uuid_expr;
+   }
----------------
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`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43576/new/

https://reviews.llvm.org/D43576