Change attribute merge order [RFC]

[Meinersbur]

Summary

This is a very crude attempt to fix the diagnostic marker order ('note: previous declaration here' pointing to a source location before the main error marker) from D48100.

Approach:

• After parsing, remove all attributes from the new declaration
• Merge-in all inherited attributes (since the Decl's attribute list is empty, this should not result in any message)
• Merge-in the previously removed parsed attributes again

This ensures that

• Inherited attributes are listed before all non-inherited attributes (and corresponds textual order)
• Attribute merging happens the same way as adding a new attribute to the currently parsed declaration's attribute list

Patch is unfinished and ugly

include/clang/Sema/Sema.h

public:
   InternalLinkageAttr *mergeInternalLinkageAttr(Decl *D, SourceRange Range,
                                                 IdentifierInfo *Ident,
                                                 unsigned AttrSpellingListIndex);
   CommonAttr *mergeCommonAttr(Decl *D, SourceRange Range, IdentifierInfo *Ident,
                               unsigned AttrSpellingListIndex);
 
   void mergeDeclAttributes(NamedDecl *New, Decl *Old,
                            AvailabilityMergeKind AMK = AMK_Redeclaration);
+  void mergeDeclAttributes2(NamedDecl *New, Decl *Old,
+                            AvailabilityMergeKind AMK = AMK_Redeclaration);
+  void mergeDeclAttributes3(NamedDecl *New, const AttrVec &AttrList,
+                            bool OldIsUsed, SourceLocation OldLocation,
+                            AvailabilityMergeKind AMK = AMK_Redeclaration);
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
                             LookupResult &OldDecls);
   bool MergeFunctionDecl(FunctionDecl *New, NamedDecl *&Old, Scope *S,
                          bool MergeTypeWithOld);
   bool MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old,
                                     Scope *S, bool MergeTypeWithOld);
   void mergeObjCMethodDecls(ObjCMethodDecl *New, ObjCMethodDecl *Old);
   void MergeVarDecl(VarDecl *New, LookupResult &Previous);

lib/AST/DeclBase.cpp

 
 void Decl::dropAttrs() {
   if (!HasAttrs) return;
 
   HasAttrs = false;
   getASTContext().eraseDeclAttrs(this);
 }
 
+void Decl::addAttr(Attr *A) {
+  if (!hasAttrs()) {
+    setAttrs(AttrVec(1, A));
+    return;
+  }
+
+  getAttrs().push_back(A);
+}
+
 const AttrVec &Decl::getAttrs() const {
   assert(HasAttrs && "No attrs to get!");
   return getASTContext().getDeclAttrs(this);
 }
 
 Decl *Decl::castFromDeclContext (const DeclContext *D) {
   Decl::Kind DK = D->getDeclKind();
   switch(DK) {

lib/Sema/SemaDecl.cpp

if (OldAlignasAttr && !NewAlignasAttr &&
     Clone->setInherited(true);
     New->addAttr(Clone);
     AnyAdded = true;
   }
 
   return AnyAdded;
 }
 
+static bool mergeAlignedAttrs(Sema &S, NamedDecl *New,
+                              ArrayRef<AlignedAttr *> AlignedAttrs) {
+  // Look for alignas attributes on Old, and pick out whichever attribute
+  // specifies the strictest alignment requirement.
+  AlignedAttr *OldAlignasAttr = nullptr;
+  AlignedAttr *OldStrictestAlignAttr = nullptr;
+  unsigned OldAlign = 0;
+  for (auto *I : AlignedAttrs) {
+    // FIXME: We have no way of representing inherited dependent alignments
+    // in a case like:
+    //   template<int A, int B> struct alignas(A) X;
+    //   template<int A, int B> struct alignas(B) X {};
+    // For now, we just ignore any alignas attributes which are not on the
+    // definition in such a case.
+    if (I->isAlignmentDependent())
+      return false;
+
+    if (I->isAlignas())
+      OldAlignasAttr = I;
+
+    unsigned Align = I->getAlignment(S.Context);
+    if (Align > OldAlign) {
+      OldAlign = Align;
+      OldStrictestAlignAttr = I;
+    }
+  }
+
+  // Look for alignas attributes on New.
+  AlignedAttr *NewAlignasAttr = nullptr;
+  unsigned NewAlign = 0;
+  for (auto *I : New->specific_attrs<AlignedAttr>()) {
+    if (I->isAlignmentDependent())
+      return false;
+
+    if (I->isAlignas())
+      NewAlignasAttr = I;
+
+    unsigned Align = I->getAlignment(S.Context);
+    if (Align > NewAlign)
+      NewAlign = Align;
+  }
+
+  if (OldAlignasAttr && NewAlignasAttr && OldAlign != NewAlign) {
+    // Both declarations have 'alignas' attributes. We require them to match.
+    // C++11 [dcl.align]p6 and C11 6.7.5/7 both come close to saying this, but
+    // fall short. (If two declarations both have alignas, they must both match
+    // every definition, and so must match each other if there is a definition.)
+
+    // If either declaration only contains 'alignas(0)' specifiers, then it
+    // specifies the natural alignment for the type.
+    if (OldAlign == 0 || NewAlign == 0) {
+      QualType Ty;
+      if (ValueDecl *VD = dyn_cast<ValueDecl>(New))
+        Ty = VD->getType();
+      else
+        Ty = S.Context.getTagDeclType(cast<TagDecl>(New));
+
+      if (OldAlign == 0)
+        OldAlign = S.Context.getTypeAlign(Ty);
+      if (NewAlign == 0)
+        NewAlign = S.Context.getTypeAlign(Ty);
+    }
+
+    if (OldAlign != NewAlign) {
+      S.Diag(NewAlignasAttr->getLocation(), diag::err_alignas_mismatch)
+          << (unsigned)S.Context.toCharUnitsFromBits(OldAlign).getQuantity()
+          << (unsigned)S.Context.toCharUnitsFromBits(NewAlign).getQuantity();
+      S.Diag(OldAlignasAttr->getLocation(), diag::note_previous_declaration);
+    }
+  }
+
+  if (OldAlignasAttr && !NewAlignasAttr && isAttributeTargetADefinition(New)) {
+    // C++11 [dcl.align]p6:
+    //   if any declaration of an entity has an alignment-specifier,
+    //   every defining declaration of that entity shall specify an
+    //   equivalent alignment.
+    // C11 6.7.5/7:
+    //   If the definition of an object does not have an alignment
+    //   specifier, any other declaration of that object shall also
+    //   have no alignment specifier.
+    S.Diag(New->getLocation(), diag::err_alignas_missing_on_definition)
+        << OldAlignasAttr;
+    S.Diag(OldAlignasAttr->getLocation(), diag::note_alignas_on_declaration)
+        << OldAlignasAttr;
+  }
+
+  bool AnyAdded = false;
+
+  // Ensure we have an attribute representing the strictest alignment.
+  if (OldAlign > NewAlign) {
+    AlignedAttr *Clone = OldStrictestAlignAttr->clone(S.Context);
+    Clone->setInherited(true);
+    New->addAttr(Clone);
+    AnyAdded = true;
+  }
+
+  // Ensure we have an alignas attribute if the old declaration had one.
+  if (OldAlignasAttr && !NewAlignasAttr &&
+      !(AnyAdded && OldStrictestAlignAttr->isAlignas())) {
+    AlignedAttr *Clone = OldAlignasAttr->clone(S.Context);
+    Clone->setInherited(true);
+    New->addAttr(Clone);
+    AnyAdded = true;
+  }
+
+  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
for (unsigned I = 0, E = NewAttributes.size(); I != E;) {
     NewAttributes.erase(NewAttributes.begin() + I);
     --E;
   }
 }
 
 /// mergeDeclAttributes - Copy attributes from the Old decl to the New one.
 void Sema::mergeDeclAttributes(NamedDecl *New, Decl *Old,
                                AvailabilityMergeKind AMK) {
+  AttrVec Attrs = New->hasAttrs() ? New->getAttrs() : AttrVec();
+  bool OldIsUsed = Old->isUsed();
+  New->dropAttrs();
+  New->setAttrs(AttrVec());
+
+  // Propagate inherited attributes.
+  mergeDeclAttributes2(New, Old, AMK);
+  mergeDeclAttributes3(New, Attrs, OldIsUsed, Old->getLocation(), AMK);
+
+  if (New->getAttrs().empty())
+    New->dropAttrs();
+}
+
+template <typename T> T *getSpecificAttr(const AttrVec &AttrList) {
+  for (auto A : AttrList) {
+    if (isa<T>(A))
+      return cast<T>(A);
+  }
+  return nullptr;
+}
+
+void Sema::mergeDeclAttributes3(NamedDecl *New, const AttrVec &AttrList,
+                                bool OldIsUsed, SourceLocation OldLocation,
+                                AvailabilityMergeKind AMK) {
+  SmallVector<AlignedAttr *, 4> AlignedAttrs;
+
+  if (AsmLabelAttr *NewA = getSpecificAttr<AsmLabelAttr>(AttrList)) {
+    if (AsmLabelAttr *OldA = New->getAttr<AsmLabelAttr>()) {
+      if (OldA->getLabel() != NewA->getLabel()) {
+        // This redeclaration changes __asm__ label.
+        Diag(New->getLocation(), diag::err_different_asm_label);
+        Diag(OldA->getLocation(), diag::note_previous_declaration);
+      }
+    } else if (OldIsUsed) {
+      // This redeclaration adds an __asm__ label to a declaration that has
+      // already been ODR-used.
+      Diag(New->getLocation(), diag::err_late_asm_label_name)
+          << isa<FunctionDecl>(New) << New->getAttr<AsmLabelAttr>()->getRange();
+    }
+  }
+
+  // Re-declaration cannot add abi_tag's.
+  if (const auto *NewAbiTagAttr = getSpecificAttr<AbiTagAttr>(AttrList)) {
+    if (const auto *OldAbiTagAttr = New->getAttr<AbiTagAttr>()) {
+      for (const auto &NewTag : NewAbiTagAttr->tags()) {
+        if (std::find(OldAbiTagAttr->tags_begin(), OldAbiTagAttr->tags_end(),
+                      NewTag) == OldAbiTagAttr->tags_end()) {
+          Diag(NewAbiTagAttr->getLocation(),
+               diag::err_new_abi_tag_on_redeclaration)
+              << NewTag;
+          Diag(OldAbiTagAttr->getLocation(), diag::note_previous_declaration);
+        }
+      }
+    } else {
+      Diag(OldAbiTagAttr->getLocation(), diag::err_abi_tag_on_redeclaration);
+      Diag(OldLocation, diag::note_previous_declaration);
+    }
+  }
+
+  // This redeclaration adds a section attribute.
+  if (New->hasAttr<SectionAttr>() && !getSpecificAttr<SectionAttr>(AttrList)) {
+    if (auto *VD = dyn_cast<VarDecl>(New)) {
+      if (VD->isThisDeclarationADefinition() == VarDecl::DeclarationOnly) {
+        Diag(New->getLocation(), diag::warn_attribute_section_on_redeclaration);
+        Diag(OldLocation, diag::note_previous_declaration);
+      }
+    }
+  }
+
+  for (auto Attr : AttrList) {
+    if (!isa<InheritableAttr>(Attr)) {
+      New->addAttr(Attr);
+      continue;
+    }
+
+    if (auto AA = dyn_cast<AlignedAttr>(Attr)) {
+      AlignedAttrs.push_back(AA);
+      continue;
+    }
+
+    auto *I = cast<InheritableAttr>(Attr);
+
+    // Already handled.
+    if (isa<UsedAttr>(I))
+      continue;
+
+    // Ignore deprecated/unavailable/availability attributes if requested.
+    AvailabilityMergeKind LocalAMK = AMK_None;
+    if (isa<DeprecatedAttr>(I) || isa<UnavailableAttr>(I) ||
+        isa<AvailabilityAttr>(I)) {
+      switch (AMK) {
+      case AMK_None:
+        continue;
+
+      case AMK_Redeclaration:
+      case AMK_Override:
+      case AMK_ProtocolImplementation:
+        LocalAMK = AMK;
+        break;
+      }
+    }
+
+    mergeDeclAttribute(*this, New, I, LocalAMK);
+  }
+
+  mergeAlignedAttrs(*this, New, AlignedAttrs);
+}
+
+void Sema::mergeDeclAttributes2(NamedDecl *New, Decl *Old,
+                                AvailabilityMergeKind AMK) {
   if (UsedAttr *OldAttr = Old->getMostRecentDecl()->getAttr<UsedAttr>()) {
     UsedAttr *NewAttr = OldAttr->clone(Context);
     NewAttr->setInherited(true);
     New->addAttr(NewAttr);
   }
 
   if (!Old->hasAttrs() && !New->hasAttrs())
     return;

test/Misc/ast-dump-attr.cpp

void f() {
 
       [[gsl::suppress("on-stmt")]] {
       // CHECK: AttributedStmt
       // CHECK-NEXT: SuppressAttr{{.*}} on-stmt
       // CHECK-NEXT: CompoundStmt
         i = reinterpret_cast<int*>(7);
       }
     }
-}
+}