[clang-cl] Diagnose duplicate uuids.

[thakis]

Summary

This mostly behaves cl.exe's behavior, even though clang-cl is stricter in some corner cases and more lenient in others (see the included test).

To make the uuid declared previously here diagnostic work correctly, tweak stripTypeAttributesOffDeclSpec() to keep attributes in the right order.

[thakis]

Remove local XXXs I've since addressed.

[thakis]

On conflict, keep first uuid in ast instead of having different uuids on different redeclarations.

[rnk]

lgtm

lib/Parse/ParseDecl.cpp

1456

mmm hand rolled singly linked list code. Feels like lisp.

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

2258

uuid instead of uiid.

lib/Parse/ParseDecl.cpp

1476

Remove double space here.

lib/Sema/SemaDecl.cpp

2255

Thank you for the improved comments as a drive-by.

lib/Sema/SemaDeclAttr.cpp

4609

Can use const auto * here.

Also, don't you need to iterate over all of the UuidAttr objects attached to the declaration to see if any of them match, rather than just the first?

[majnemer]

Does uuidof walk bases to find [uuid] in cl.exe? They walk bases for the declspec spelling.

[thakis]

In D24469#540400, @majnemer wrote:

Does uuidof walk bases to find [uuid] in cl.exe? They walk bases for the declspec spelling.

As far as I can tell, they don't for either:

C:\src\chrome\src>type atltest.cc
#include <guiddef.h>

class __declspec(uuid("86759049-8B8E-47F4-81F1-AE07D3F876C8")) Foo {};
class Bar : public Foo {};

[uuid("06759049-8B8E-47F4-81F1-AE07D3F876C7")] class Foo2 {};
class Bar2 : public Foo2 {};

void f() {

__uuidof(Bar);
__uuidof(Bar2);

}

C:\src\chrome\src>cl /c atltest.cc /nologo
atltest.cc
atltest.cc(6): warning C4467: usage of ATL attributes is deprecated
atltest.cc(10): error C2787: 'Bar': no GUID has been associated with this object
atltest.cc(11): error C2787: 'Bar2': no GUID has been associated with this object

lib/Parse/ParseDecl.cpp

1476

ack '\. ' lib

lib/Sema/SemaDeclAttr.cpp

4609

From what I understand, every time a new declaration is created, it copies attributes from the previous decl (see mergeDeclAttribute() in SemaDecl.cpp), so checking the latest should be enough. I added a test case for this.

[thakis]

comments

[aaron.ballman]

LGTM!

lib/Sema/SemaDeclAttr.cpp

4609

I didn't pick up on the fact that there shouldn't be multiple UUID attributes associated with a single decl anyway. So this code is fine (with the auto nit fixed).

[thakis]

r281367, thanks!

include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_argument_vec_type_hint : Error<
   "invalid attribute argument %0 - expecting a vector or vectorizable scalar type">;
 def err_attribute_argument_out_of_bounds : Error<
   "%0 attribute parameter %1 is out of bounds">;
 def err_attribute_only_once_per_parameter : Error<
   "%0 attribute can only be applied once per parameter">;
 def err_attribute_uuid_malformed_guid : Error<
   "uuid attribute contains a malformed GUID">;
+def err_mismatched_uuid : Error<"uuid does not match previous declaration">;
+def note_previous_uuid : Note<"previous uuid specified here">;
 def warn_attribute_pointers_only : Warning<
   "%0 attribute only applies to%select{| constant}1 pointer arguments">,
   InGroup<IgnoredAttributes>;
 def err_attribute_pointers_only : Error<warn_attribute_pointers_only.Text>;
 def warn_attribute_return_pointers_only : Warning<
   "%0 attribute only applies to return values that are pointers">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_return_pointers_refs_only : Warning<

include/clang/Sema/AttributeList.h

void addAll(AttributeList *newList) {
     AttributeList *lastInNewList = newList;
     while (AttributeList *next = lastInNewList->getNext())
       lastInNewList = next;
 
     lastInNewList->setNext(list);
     list = newList;
   }
 
+  void addAllAtEnd(AttributeList *newList) {
+    if (!list) {
+      list = newList;
+      return;
+    }
+
+    AttributeList *lastInList = list;
+    while (AttributeList *next = lastInList->getNext())
+      lastInList = next;
+
+    lastInList->setNext(newList);
+  }
+
   void set(AttributeList *newList) {
     list = newList;
   }
 
   void takeAllFrom(ParsedAttributes &attrs) {
     addAll(attrs.list);
     attrs.list = nullptr;
     pool.takeAllFrom(attrs.pool);

include/clang/Sema/Sema.h

AvailabilityAttr *mergeAvailabilityAttr(NamedDecl *D, SourceRange Range,
                                           AvailabilityMergeKind AMK,
                                           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);
   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);

lib/Parse/ParseDecl.cpp

void Parser::stripTypeAttributesOffDeclSpec(ParsedAttributesWithRange &Attrs,
                                             DeclSpec &DS,
                                             Sema::TagUseKind TUK) {
   if (TUK == Sema::TUK_Reference)
     return;
 
   ParsedAttributes &PA = DS.getAttributes();
   AttributeList *AL = PA.getList();
   AttributeList *Prev = nullptr;
+  AttributeList *TypeAttrHead = nullptr;
+  AttributeList *TypeAttrTail = nullptr;
   while (AL) {
     AttributeList *Next = AL->getNext();
 
     if ((AL->getKind() == AttributeList::AT_Aligned &&
          AL->isDeclspecAttribute()) ||
         AL->isMicrosoftAttribute()) {
       // Stitch the attribute into the tag's attribute list.
-      AL->setNext(nullptr);
-      Attrs.add(AL);
+      if (TypeAttrTail)
+        TypeAttrTail->setNext(AL);
+      else
+        TypeAttrHead = AL;
+      TypeAttrTail = AL;
+      TypeAttrTail->setNext(nullptr);
 
       // Remove the attribute from the variable's attribute list.
       if (Prev) {
         // Set the last variable attribute's next attribute to be the attribute
         // after the current one.
         Prev->setNext(Next);
       } else {
         // Removing the head of the list requires us to reset the head to the
         // next attribute.
         PA.set(Next);
       }
     } else {
       Prev = AL;
     }
 
     AL = Next;
   }
+
+  // Find end of type attributes Attrs and add NewTypeAttributes in the same
+  // order they were in originally.  (Remember, in AttributeList things earlier
+  // in source order are later in the list, since new attributes are added to
+  // the front of the list.)
+  Attrs.addAllAtEnd(TypeAttrHead);
 }
 
 /// ParseDeclaration - Parse a full 'declaration', which consists of
 /// declaration-specifiers, some number of declarators, and a semicolon.
 /// 'Context' should be a Declarator::TheContext value.  This returns the
 /// location of the semicolon in DeclEnd.
 ///
 ///       declaration: [C99 6.7]

lib/Sema/SemaDecl.cpp

static bool mergeAlignedAttrs(Sema &S, NamedDecl *New, Decl *Old) {
   }
 
   return AnyAdded;
 }
 
 static bool mergeDeclAttribute(Sema &S, NamedDecl *D,
                                const InheritableAttr *Attr,
                                Sema::AvailabilityMergeKind AMK) {
+  // This function copies an attribute Attr from a previous declaration to the
+  // new declaration D if the new declaration doesn't itself have that attribute
+  // yet or if that attribute allows duplicates.
+  // If you're adding a new attribute that requires logic different from
+  // "use explicit attribute on decl if present, else use attribute from
+  // previous decl", for example if the attribute needs to be consistent
+  // between redeclarations, you need to call a custom merge function here.
   InheritableAttr *NewAttr = nullptr;
   unsigned AttrSpellingListIndex = Attr->getSpellingListIndex();
   if (const auto *AA = dyn_cast<AvailabilityAttr>(Attr))
     NewAttr = S.mergeAvailabilityAttr(D, AA->getRange(), AA->getPlatform(),
                                       AA->isImplicit(), AA->getIntroduced(),
                                       AA->getDeprecated(),
                                       AA->getObsoleted(), AA->getUnavailable(),
                                       AA->getMessage(), AA->getStrict(),
static bool mergeDeclAttribute(Sema &S, NamedDecl *D,
   else if (isa<AlignedAttr>(Attr))
     // AlignedAttrs are handled separately, because we need to handle all
     // 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());
   else if (Attr->duplicatesAllowed() || !DeclHasAttr(D, Attr))
     NewAttr = cast<InheritableAttr>(Attr->clone(S.Context));
 
   if (NewAttr) {
     NewAttr->setInherited(true);
     D->addAttr(NewAttr);
     if (isa<MSInheritanceAttr>(NewAttr))
       S.Consumer.AssignInheritanceModel(cast<CXXRecordDecl>(D));

lib/Sema/SemaDeclAttr.cpp

D->addAttr(::new (S.Context)
              ObjCPreciseLifetimeAttr(Attr.getRange(), S.Context,
                                      Attr.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() == 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);
+}
+
 static void handleUuidAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   if (!S.LangOpts.CPlusPlus) {
     S.Diag(Attr.getLoc(), diag::err_attribute_not_supported_in_lang)
       << Attr.getName() << AttributeLangSupport::C;
     return;
   }
 
   if (!isa<CXXRecordDecl>(D)) {
if (i == 8 || i == 13 || i == 18 || i == 23) {
         return;
       }
     } else if (!isHexDigit(StrRef[i])) {
       S.Diag(LiteralLoc, diag::err_attribute_uuid_malformed_guid);
       return;
     }
   }
 
-  D->addAttr(::new (S.Context) UuidAttr(Attr.getRange(), S.Context, StrRef,
-                                        Attr.getAttributeSpellingListIndex()));
+  UuidAttr *UA = S.mergeUuidAttr(D, Attr.getRange(),
+                                 Attr.getAttributeSpellingListIndex(), StrRef);
+  if (UA)
+    D->addAttr(UA);
 }
 
 static void handleMSInheritanceAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   if (!S.LangOpts.CPlusPlus) {
     S.Diag(Attr.getLoc(), diag::err_attribute_not_supported_in_lang)
       << Attr.getName() << AttributeLangSupport::C;
     return;
   }

test/SemaCXX/ms-uuid.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s
+
+typedef struct _GUID {
+  unsigned long Data1;
+  unsigned short Data2;
+  unsigned short Data3;
+  unsigned char Data4[8];
+} GUID;
+
+namespace {
+// cl.exe's behavior with merging uuid attributes is a bit erratic:
+// * In []-style attributes, a single [] list must not list a duplicate uuid
+//   (even if it's the same uuid), and only a single declaration of a class
+//   must have a uuid else the compiler errors out (even if two declarations of
+//   a class have the same uuid).
+// * For __declspec(uuid(...)), it's ok if several declarations of a class have
+//   an uuid, as long as it's the same uuid each time.  If uuids on declarations
+//   don't match, the compiler errors out.
+// * If there are several __declspec(uuid(...))s on one declaration, the
+//   compiler only warns about this and uses the last uuid.  It even warns if
+//   the uuids are the same.
+
+// clang-cl implements the following simpler (but largely compatible) behavior
+// instead:
+// * [] and __declspec uuids have the same behavior.
+// * If there are several uuids on a a class (no matter if on the same decl or
+//   on several decls), it is an error if they don't match.
+// * Having several uuids that match is ok.
+
+// Both cl and clang-cl accept this:
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049")) C1;
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049")) C1;
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049")) C1 {};
+
+// Both cl and clang-cl error out on this:
+// expected-note@+1 2{{previous uuid specified here}}
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049")) C2;
+// expected-error@+1 {{uuid does not match previous declaration}}
+class __declspec(uuid("110000A0-0000-0000-C000-000000000049")) C2;
+// expected-error@+1 {{uuid does not match previous declaration}}
+class __declspec(uuid("220000A0-0000-0000-C000-000000000049")) C2 {};
+
+// expected-note@+1 {{previous uuid specified here}}
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049")) C2_2;
+class C2_2;
+// expected-error@+1 {{uuid does not match previous declaration}}
+class __declspec(uuid("110000A0-0000-0000-C000-000000000049")) C2_2;
+
+// clang-cl accepts this, but cl errors out:
+[uuid("000000A0-0000-0000-C000-000000000049")] class C3;
+[uuid("000000A0-0000-0000-C000-000000000049")] class C3;
+[uuid("000000A0-0000-0000-C000-000000000049")] class C3 {};
+
+// Both cl and clang-cl error out on this (but for different reasons):
+// expected-note@+1 2{{previous uuid specified here}}
+[uuid("000000A0-0000-0000-C000-000000000049")] class C4;
+// expected-error@+1 {{uuid does not match previous declaration}}
+[uuid("110000A0-0000-0000-C000-000000000049")] class C4;
+// expected-error@+1 {{uuid does not match previous declaration}}
+[uuid("220000A0-0000-0000-C000-000000000049")] class C4 {};
+
+// Both cl and clang-cl error out on this:
+// expected-note@+1 {{previous uuid specified here}}
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049"))
+// expected-error@+1 {{uuid does not match previous declaration}}
+      __declspec(uuid("110000A0-0000-0000-C000-000000000049")) C5;
+
+// expected-note@+1 {{previous uuid specified here}}
+[uuid("000000A0-0000-0000-C000-000000000049"),
+// expected-error@+1 {{uuid does not match previous declaration}}
+ uuid("110000A0-0000-0000-C000-000000000049")] class C6;
+
+// cl doesn't diagnose having one uuid each as []-style attributes and as
+// __declspec, even if the uuids differ.  clang-cl errors if they differ.
+[uuid("000000A0-0000-0000-C000-000000000049")]
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049")) C7;
+
+// expected-note@+1 {{previous uuid specified here}}
+[uuid("000000A0-0000-0000-C000-000000000049")]
+// expected-error@+1 {{uuid does not match previous declaration}}
+class __declspec(uuid("110000A0-0000-0000-C000-000000000049")) C8;
+
+
+// cl warns on this, but clang-cl is fine with it (which is consistent with
+// e.g. specifying __multiple_inheritance several times, which cl accepts
+// without warning too).
+class __declspec(uuid("000000A0-0000-0000-C000-000000000049"))
+      __declspec(uuid("000000A0-0000-0000-C000-000000000049")) C9;
+
+// cl errors out on this, but clang-cl is fine with it (to be consistent with
+// the previous case).
+[uuid("000000A0-0000-0000-C000-000000000049"),
+ uuid("000000A0-0000-0000-C000-000000000049")] class C10;
+}