[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 a comment.

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 `DeclSpecUuidDecl`s in CodeGen, replacing our current ad-hoc emission of UUID globals



================
Comment at: include/clang/Sema/ParsedAttr.h:220
 
+  StringRef StrUuid;
+
----------------
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).


================
Comment at: include/clang/Sema/ParsedAttr.h:564
+  StringRef getUuidStr() const {
+    assert(getKind() == AT_Uuid && "Not an availability attribute");
+    return StrUuid;
----------------
Copy-paste error in assert message


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


================
Comment at: include/clang/Sema/Sema.h:398
+  /// a single Expr.
+  std::map<StringRef, ExprResult> UuidExpMap;
+  std::map<const Decl*, StringRef> DeclSpecToStrUuid;
----------------
Use an `llvm::StringMap` here? But I'm also not convinced this should exist at all.


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


================
Comment at: lib/Parse/ParseDecl.cpp:588
     return !HasInvalidAccessor;
+    if (AttrName->getName() == "uuid") {
+      // Parse the uuid attribute and create a UuidDecl.
----------------
This code is dead: note that we returned on the line before.


================
Comment at: lib/Parse/ParseDecl.cpp:589
+    if (AttrName->getName() == "uuid") {
+      // Parse the uuid attribute and create a UuidDecl.
+      ConsumeParen();
----------------
This comment does not describe what this code does.


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


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5371
+  if (auto *UA = D->getAttr<UuidAttr>()) {
+    if (UA->UuidAttr::getUuidAsStr().equals_lower(Uuid->getStrUuid()))
       return nullptr;
----------------
We should be comparing to see if the declarations are the same here (use `declaresSameEntity` on the UUID decls).


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5404-5409
+  // GUID string shouldn't be a wide string.
+  if (StrRef.front() == 'L') {
+    S.Diag(LiteralLoc, diag::err_attribute_argument_type)
+      << AL.getName() << AANT_ArgumentString;
+    return;
+  }
----------------
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.)


================
Comment at: lib/Sema/SemaExprCXX.cpp:636
+  if (TD->getMostRecentDecl()->getAttr<UuidAttr>()) {
+    const Decl *dl = SemaRef.DeclSpecUuidDecls.back();
+    UuidDecl.push_back(dcl);
----------------
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.)


================
Comment at: lib/Sema/SemaExprCXX.cpp:664-670
+    FindStrUuid(*this, QT, UuidDecl);
+    if (UuidDecl.empty())
       return ExprError(Diag(TypeidLoc, diag::err_uuidof_without_guid));
-    if (UuidAttrs.size() > 1)
+    if (UuidDecl.size() > 1)
       return ExprError(Diag(TypeidLoc, diag::err_uuidof_with_multiple_guids));
-    UuidStr = UuidAttrs.back()->getGuid();
+    const Decl *dd1 = UuidDecl.back();
+    UuidStr = DeclSpecToStrUuid[dd1];
----------------
I think this should be finding the `UuidAttr` of the declaration and asking it for its `DeclSpecUuidDecl`.


================
Comment at: lib/Sema/SemaExprCXX.cpp:675-678
+  if (expr.isUnset()) {
+    uuid_expr =
+        new (Context) CXXUuidofExpr(TypeInfoType.withConst(), Operand, UuidStr,
+                                    SourceRange(TypeidLoc, RParenLoc));
----------------
We should store a pointer to the UUID declaration on a non-dependent `CXXUuidofExpr`.


================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:554-558
+  DeclSpecUuidDecl *Inst = DeclSpecUuidDecl::Create(SemaRef.Context, Owner,
+                                                    D->getLocation(),
+                                                    D->getStrUuid());
+  Owner->addDecl(Inst);
+  return Inst;
----------------
Just use `llvm_unreachable` here; `DeclSpecUuidDecl`s are global and shouldn't be instantiated.


================
Comment at: test/Parser/MicrosoftExtensions.cpp:79
 
-   __uuidof(struct_with_uuid);
+   __uuidof(struct_with_uuid); // expected-error {{non-type template argument of type 'const _GUID' is not a constant expression}}
    __uuidof(struct_with_uuid2);
----------------
Why is this error now appearing on the wrong line?


================
Comment at: test/Parser/ms-square-bracket-attributes.mm:22-23
 [uuid(u8"000000A0-0000-0000-C000-000000000049")] struct struct_with_uuid_u8;
-// expected-error at +1 {{uuid attribute contains a malformed GUID}}
+// expected-error at +1 {{attribute requires a string}}
 [uuid(L"000000A0-0000-0000-C000-000000000049")] struct struct_with_uuid_L;
 
----------------
This is a diagnostic quality regression. The attribute does not require a string. But it does not accept a string with an encoding prefix.


================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:332
+      if (type == "FunctionDecl *" || type == "NamedDecl *" ||
+	  (type == "DeclSpecUuidDecl *")) {
         OS << "    OS << \" \";\n";
----------------
Nit: no need for parens here, and bad indentation.


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

https://reviews.llvm.org/D43576