[ODRHash] Hash attributes on declarations.

[vsapsai]

Summary

Arguments not for all attributes are covered. More can be added later as
needed.

rdar://100482330

[vsapsai]

clang/lib/AST/ODRHash.cpp

479–480

I'm not sure isImplicit is the best indicator of attributes to check, so suggestions in this area are welcome. I think we can start strict and relax some of the checks if needed.

If people have strong opinions that some attributes shouldn't be ignored, we can add them to the tests to avoid regressions. Personally, I believe that alignment and packed attributes should never be silently ignored.

clang/test/Modules/odr_hash.cpp

3640

As we land hashing for C and Objective-C, we can move these tests to their own file. But for now I think it makes sense to keep everything in odr_hash.cpp. Though I don't have a strong preference.

[ChuanqiXu]

I guess the reason why we didn't check odr violation for attributes is that the attributes was not a part of declarations in C++ before. But it is odd to skip the check for attributes from the perspective of users. So I think this one should be good. The only concern is that it misses too many checks for attributes. Do you plan to add it in near future or in longer future? And I guess we need to mention this in ReleaseNotes in Potential Breaking Changes section.

clang/lib/AST/ODRDiagsEmitter.cpp

337–354

I feel like we can merge these two statements.

clang/lib/AST/ODRHash.cpp

479–480

Agreed. I feel isImplicit is enough for now.

[aaron.ballman]

clang/include/clang/AST/ODRDiagsEmitter.h

119

clang/lib/AST/ODRDiagsEmitter.cpp

159

Typedefs can have attributes, so should this be updated as well? (alignment, ext vector types, mode attributes, etc all come to mind)

302

Do we want to have more control over the printing policy here? e.g., do we really want to claim an ODR difference if one module's printing policy specifies indentation of 8 and another specifies indentation of 4? Or if one module prints restrict while another prints __restrict, etc?

311

You can probably drop the getIdentifier() here as the diagnostics engine knows how to print named declarations properly already.

318

Same here.

clang/lib/AST/ODRHash.cpp

479–480

The tricky part is -- sometimes certain attributes add additional implicit attributes and those implicit attributes matter (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L9380). And some attributes seem to just do the wrong thing entirely: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L7344

So I think isImplicit() is a good approximation, but I'm more wondering what the principle is for whether an attribute should or should not be considered part of the ODR hash. Type attributes, attributes that impact object layout, etc all seem like they should almost certainly be part of ODR hashing. Others are a bit more questionable though.

I think this is something that may need a per-attribute flag in Attr.td so attributes can opt in or out of it because I'm not certain what ODR issues could stem from [[maybe_unused]] or [[deprecated]] disagreements across module boundaries.

494

100% agreed.

[erichkeane]

Most of my concerns change based on the feedback others have given, so after those are dealt with, I'll do another depever view.

clang/include/clang/Basic/DiagnosticASTKinds.td

852

Wowzer these are tough to read... can you provide a magic-decoder ring for me of some sort?

clang/lib/AST/ODRHash.cpp

479–480

I don't think 'isImplicit' is particularly good. I think the idea of auto-adding 'type' attributes and having the 'rest' be analyzed to figure out which are important.

Alternatively, I wonder if we're better off just adding ALL attributes and seeing what the fallout is. We can later decide when we don't want them to be ODR significant (which, might be OTHERWISE meaningful later!).

[vsapsai]

In D135472#3844688, @ChuanqiXu wrote:

I guess the reason why we didn't check odr violation for attributes is that the attributes was not a part of declarations in C++ before. But it is odd to skip the check for attributes from the perspective of users. So I think this one should be good.

I think we haven't seen many problems with attribute mismatches because in existing code attributes are often hidden behind macros. And we have sugar MacroQualifiedType that causes the definitions

#define NODEREF __attribute__((noderef))
struct StructWithField {
  int NODEREF *i_ptr;
};

struct StructWithField {
  int __attribute__((noderef)) *i_ptr;
};

to be treated as incompatible.

But the keyword alignas is used without macros and more attributes can be used in multiple compilers without macros. That's why I think it is useful to detect and to diagnose mismatches in attributes.

In D135472#3844688, @ChuanqiXu wrote:

The only concern is that it misses too many checks for attributes. Do you plan to add it in near future or in longer future?

In the near future I was planning to add various Objective-C and Swift attributes. For other attributes I don't know which are high-value. I definitely want to check attributes affecting the memory layout (alignment, packing) and believe I've addressed them (totally could have missed something).

In D135472#3844688, @ChuanqiXu wrote:

And I guess we need to mention this in ReleaseNotes in Potential Breaking Changes section.

Good idea, will do.

clang/lib/AST/ODRDiagsEmitter.cpp

337–354

Sorry, I don't really get what two statements you are talking about. Is it

• !LHS || !RHS || LHS->getKind() != RHS->getKind()
• ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)

?

[vsapsai]

clang/include/clang/Basic/DiagnosticASTKinds.td

852

Guess I went too far with avoiding repetition (was feeling soo smart doing it). I'll go back to the messages with a little bit more repetition but which should be easier to read and understand.

clang/lib/AST/ODRDiagsEmitter.cpp

159

It should be tested for sure, thanks for pointing it out.

302

FullAttributeText is used for diagnostics and not for the mismatch detection, so we shouldn't complain about restrict/__ restrict mismatches (DifferentAlignmentKeywords tests that __attribute__((aligned(8))) and alignas(8) are not a mismatch).

We use the same ASTContext to print both attributes, so that shouldn't be confusing. Diagnostic can be unstable across clang versions and probably across language modes. But I think that can happen to other diagnostic messages too, so think that's acceptable.

311

Thanks for the hint, will do.

clang/lib/AST/ODRHash.cpp

479–480

One criteria to decide which attributes should be hashed is if they affect IRGen. But that's not exhaustive and I'm not sure how practical it is.

The rule I'm trying to follow right now is if declarations with different attributes can be substituted instead of each other. For example, structs with different memory layout cannot be interchanged, so it is reasonable to reject them.

But maybe we should review attributes on case-by-case basis. For example, for [[deprecated]] I think the best for developers is not to complain about it but merge the attributes and then have regular diagnostic about using a deprecated entity.

479–480

One option was to hash isa<InheritedAttr> attributes as they are "sticky" and should be more significant, so shouldn't be ignored. But I don't think this argument is particularly convincing.

At this stage I think we can still afford to add all attributes and exclude some as-needed because modules adoption is still limited. Though I don't have strong feelings about this approach, just getting more restrictive later can be hard.

[ChuanqiXu]

In the near future I was planning to add various Objective-C and Swift attributes. For other attributes I don't know which are high-value. I definitely want to check attributes affecting the memory layout (alignment, packing) and believe I've addressed them (totally could have missed something).

It looks like you're planning to add them one by one, or do I misunderstand? It looks not so good to add them one by one. Maybe it'll be a better idea to add them in the Attr.td?

clang/lib/AST/ODRDiagsEmitter.cpp

337–354

Sorry for the ambiguity. Since LHS->getKind() != RHS->getKind() is covered by ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS). I feel like it is reasonable to:

if (!LHS || !RHS || ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)) {
    DiagError();
    DiagNote();
    DiagNote();
    DiagNoteAttrLoc();
    return true;
}

clang/lib/AST/ODRHash.cpp

479–480

It looks not a bad idea to add all attributes as experiments, @vsapsai how do you feel about this?

[aaron.ballman]

clang/lib/AST/ODRDiagsEmitter.cpp

302

Oh, that's great, thank you!

clang/lib/AST/ODRHash.cpp

479–480

My intuition is that we're going to want this to be controlled from Attr.td on a case-by-case basis and to automatically generate the ODR hash code for attribute arguments.

We can either force every attribute to decide explicitly (this seems pretty disruptive as a final design but would be a really good way to ensure we audit all attributes if done as an experiment), or we can pick a default behavior to ODR hash/not. I suspect we're going to want to pick a default behavior based on whatever the audit tells us the most common situation is.

I think we're going to need something a bit more nuanced than "this attribute matters for ODR" because there are attribute arguments that don't always contribute to the entity (for example, we have "fake" arguments that are used to give the semantic attribute more information, there may also be cases where one argument matter and another argument does not such as enable_if where the condition matters greatly but the message doesn't matter much). So we might need a marking on the attribute level *and* on the parameter level to determine what all factors into attribute identity for generating the ODR hashing code. Hopefully we can avoid needing more granularity than that.

[ChuanqiXu]

clang/lib/AST/ODRHash.cpp

479–480

I feel the default behavior would be to do ODR hashes. I just took a quick look in Attr.td. And I feel most of them affecting the code generation. For example, there're a lot of attributes which is hardware related.

because there are attribute arguments that don't always contribute to the entity

When you're talking about "entities", I'm not sure if you're talking the "entities" in the language spec or a general abstract ones. I mean, for example, the always_inline attribute doesn't contribute to the declaration from the language perspective. But it'll be super odd if we lost it. So I feel it may be better to change the requirement to be "affecting code generation"

Also, to be honest, I am even not sure if we should take "affecting code generation " as the requirement. For example, for the preferred_name attribute, this is used for better printing names. It doesn't affect code generation nor the entity you mentioned. But if we drop it, then the printing name may not be what users want. I'm not sure if this is desired.

[aaron.ballman]

clang/lib/AST/ODRHash.cpp

479–480

I feel the default behavior would be to do ODR hashes. I just took a quick look in Attr.td. And I feel most of them affecting the code generation. For example, there're a lot of attributes which is hardware related.

Hmmm, I'm seeing a reasonably even split. We definitely have a ton of attributes like multiversioning, interrupts, calling conventions, availability, etc that I think all should be part of an ODR hash. We also have a ton of other ones that don't seem like they should matter for ODR hashing like hot/cold, consumed/returns retained, constructor/destructor, deprecated, nodiscard, maybe_unused, etc.

Then we have the really fun ones where I think we'll want them to be in the ODR hash but maybe we don't, like [[clang::annotate]], restrict, etc.

So I think we really do need an actual audit of the attributes to decide what the default should be (if there should even be one).

When you're talking about "entities", I'm not sure if you're talking the "entities" in the language spec or a general abstract ones. I mean, for example, the always_inline attribute doesn't contribute to the declaration from the language perspective. But it'll be super odd if we lost it. So I feel it may be better to change the requirement to be "affecting code generation"

Not in a language spec meaning -- I meant mostly that there's some notion of identify for the one definition rule and not all attribute arguments contribute to that identity the same way.

I don't know that "affecting code gen" really captures the whole of it. For example, whether a function is/is not marked hot or cold affects codegen, but really has nothing to do with the identity of the function (it's the same function whether the hint is there or not). preferred_name is another example -- do we really think a structure with that attribute is fundamentally a different thing than one without that attribute? I don't think so.

To me, it's more about what cross-TU behaviors you'll get if the attribute is only present on one side. Things like ABI tags, calling conventions, attributes that behave like qualifiers, etc all contribute to the identity of something where a mismatch will impact the correctness of the program. But things like optimization hints and diagnostic hints don't seem like they contribute to the identity of something because they don't impact correctness.

[vsapsai]

• Small fixes to address review comments.
• Try to make diagnostics more understandable.
• Check attributes on typedefs.

[vsapsai]

Given all the discussion about which attributes should be added to ODR hash, I think it is useful at this point to have TableGen infrastructure to get this information from Attr.td. So I'll work on that.

clang/lib/AST/ODRDiagsEmitter.cpp

159

Typedefs are done. But while adding support for them I've realized we don't check method parameters. So it requires a little bit more work.

337–354

There are 2 separate cases to improve the diagnostic. In the first case we'd have

... first difference is definition in module 'FirstModule' found attribute 'enum_extensibility'
attribute specified here
but in 'SecondModule' found no attribute

And if we reach the second ones, it implies the kind of attributes is the same and the only difference is attribute arguments, so the diagnostics are like

first difference is definition in module 'FirstModule' found attribute ' __attribute__((enum_extensibility("open")))'
attribute specified here
but in 'SecondModule' found different attribute argument ' __attribute__((enum_extensibility("closed")))'
attribute specified here

From my limited experience, it is actually useful to have more details than trying to figure out the difference in attributes.

[vsapsai]

Fix the starting point of the diff.

[vsapsai]

clang/include/clang/Basic/DiagnosticASTKinds.td

852

It should be easier to understand now (easier doesn't mean easy).

[ChuanqiXu]

LGTM basically. Our discussion is mainly about the future work in Attr.td and not this patch. So I think they are not blocking issues.

clang/lib/AST/ODRDiagsEmitter.cpp

337–354

Agreed. I just wanted to say the codes can be further be shorten by making the diagnostic kinds contains more cases:

%select {
|
|
...
}

We have some such examples before. But this is not required. Since it actually moves the complexity from the source codes to the table gen. And the implementation doesn't look bad.

clang/lib/AST/ODRHash.cpp

479–480

I got your point. But from my developing experience, I want to say the optimization hints matters too. And I am wondering how about checking the ODR for all attributes. It would emit error if the attributes that affecting the correctness mismatches. And if the other attributes mismatches, a warning enough. (If we worry about the compile time performance, I feel it won't take too long. And we can have an option to control it actually.)

[vsapsai]

To fix pre-merge errors like

error: 'error' diagnostics seen but not expected:
  File .../clang/test/Modules/Output/odr_hash.cpp.tmp/Inputs/first.h Line 3797: ... found 'AlignedDoublePtr' with attribute ' __attribute__((align_value(0xb7dc9b8)))'

posted D135931.

[aaron.ballman]

In D135472#3856596, @vsapsai wrote:

Given all the discussion about which attributes should be added to ODR hash, I think it is useful at this point to have TableGen infrastructure to get this information from Attr.td. So I'll work on that.

Thank you, I think that's a good approach for the moment. Once you have the basic framework in place, we can opt in the uncontroversial attributes (like, I think anything inheriting from TypeAttr or DeclOrTypeAttr should be automatically opted in and any Argument whose Fake flag is set should not contribute to the ODR hash) and then we can do a follow-up to figure out how to distinguish "definitely an ODR violation" attributes from "not an ODR violation but still something we want to find a way to alert users about".

clang/lib/AST/ODRHash.cpp

479–480

I got your point. But from my developing experience, I want to say the optimization hints matters too.

That's why I'd like to have an idea about what our policy is. To me, ODR hashing should be restricted to finding ODR violations. It sounds like you'd like ODR hashing to also optionally find other kinds of differences that aren't necessarily ODR violations but still surprises nonetheless. I think that's reasonable, but I think we should ensure the diagnostics distinguish between "this is definitely UB" and "this might not have the optimization or diagnostic properties you expect but is still correct".

And I am wondering how about checking the ODR for all attributes. It would emit error if the attributes that affecting the correctness mismatches. And if the other attributes mismatches, a warning enough. (If we worry about the compile time performance, I feel it won't take too long. And we can have an option to control it actually.)

I don't think we should ODR-check all attributes blindly. There's no ODR violation for one function having [[nodiscard]] in a TU and another function not having it. In fact, attributes are sometimes used additively so that you can put extra constraints locally that aren't there globally. e.g., I've seen real code doing:

// Header.h
int some_func();

// Source.cpp
#include "Header.h"

// We've had too many bugs from people ignoring the results,
// which really matters in this particular file. Redeclare the API
// with extra diagnostic checking for this TU.
[[nodiscard]] int some_func();

...

It shouldn't cause ODR violation diagnostics if some_func() is defined in another source file without the attribute.

[ChuanqiXu]

clang/lib/AST/ODRHash.cpp

479–480

Oh, yeah, you're right. Let's try to make the policy clearly when we started to it further.

[vsapsai]

Rebase and diagnose attribute mismatch for function parameters.

clang/include/clang/AST/ODRDiagsEmitter.h

bool diagnoseSubMismatchTypedef(const NamedDecl *FirstRecord,
                                   const TypedefNameDecl *SecondTD,
                                   bool IsTypeAlias) const;
 
   bool diagnoseSubMismatchVar(const NamedDecl *FirstRecord,
                               StringRef FirstModule, StringRef SecondModule,
                               const VarDecl *FirstVD,
                               const VarDecl *SecondVD) const;
 
+  /// Diagnose ODR mismatch in attributes between 2 Decl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it. If a diagnosed entity
+  /// \p FirstDecl is nested, like a field in a struct, \p FirstContainer should
+  /// point to the parent. Otherwise, for top-level decls \p FirstContainer can
+  /// be null.
+  bool diagnoseSubMismatchAttr(const NamedDecl *FirstContainer,
+                               StringRef FirstModule, StringRef SecondModule,
+                               const NamedDecl *FirstDecl,
+                               const NamedDecl *SecondDecl) const;
+
 private:
   DiagnosticsEngine &Diags;
   const ASTContext &Context;
   const LangOptions &LangOpts;
 };
 
 } // namespace clang
 
 #endif

clang/include/clang/AST/ODRHash.h

 #include "clang/AST/TemplateBase.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
 
 namespace clang {
 
+class Attr;
 class Decl;
 class IdentifierInfo;
 class NestedNameSpecifier;
 class Stmt;
 class TemplateParameterList;
 
 // ODRHash is used to calculate a hash based on AST node contents that
 // does not rely on pointer addresses.  This allows the hash to not vary
public:
   void AddStmt(const Stmt *S);
   void AddIdentifierInfo(const IdentifierInfo *II);
   void AddNestedNameSpecifier(const NestedNameSpecifier *NNS);
   void AddTemplateName(TemplateName Name);
   void AddDeclarationName(DeclarationName Name, bool TreatAsDecl = false);
   void AddTemplateArgument(TemplateArgument TA);
   void AddTemplateParameterList(const TemplateParameterList *TPL);
 
+  // Process attributes attached to a decl that is being hashed.
+  void AddAttrs(const Decl *D);
+  void AddAttr(const Attr *A);
+
   // Save booleans until the end to lower the size of data to process.
   void AddBoolean(bool value);
 
   static bool isDeclToBeProcessed(const Decl* D, const DeclContext *Parent);
 
+  static void getHashableAttrs(const Decl *D,
+                               SmallVectorImpl<const Attr *> &HashableAttrs);
+
 private:
   void AddDeclarationNameImpl(DeclarationName Name);
 };
 
 }  // end namespace clang
 
 #endif

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_module_odr_violation_enum : Note<"but in '%0' found "
   "enum %select{without|with}2 specified type|"
   "enum with specified type %2|"
   "enum with %2 element%s2|"
   "%ordinal2 element has name %3|"
   "%ordinal2 element %3 %select{has|does not have}4 an initializer|"
   "%ordinal2 element %3 has different initializer|"
   "}1">;
 
+def err_module_odr_violation_attribute : Error<
+  "%q0 has different definitions in different modules; first difference is "
+  "%select{definition in module '%2'|defined here}1 found"
+  "%select{| %4 with}3 "
+  "%select{"
+  "%select{no attribute|attribute %7}6|"
+  "attribute '%6'"
+  "}5">;
+def note_module_odr_violation_attribute : Note<
+  "but in '%0' found"
+  "%select{| %2 with}1 "
+  "%select{"
+  "%select{no attribute|attribute %5}4|"
+  "different attribute argument '%4'"
+  "}3">;
+
+def note_attribute_specified_here : Note<"attribute specified here">;
+
 def err_module_odr_violation_mismatch_decl_unknown : Error<
   "%q0 %select{with definition in module '%2'|defined here}1 has different "
   "definitions in different modules; first difference is this "
   "%select{||||static assert|field|method|type alias|typedef|data member|"
   "friend declaration|function template|"
   "unexpected decl}3">;
 def note_module_odr_violation_mismatch_decl_unknown : Note<
   "but in '%0' found "

clang/lib/AST/ODRDiagsEmitter.cpp

 //===-- ODRDiagsEmitter.cpp - Diagnostics for ODR mismatches ----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ODRDiagsEmitter.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ODRHash.h"
 #include "clang/Basic/DiagnosticAST.h"
 #include "clang/Basic/Module.h"
 
 using namespace clang;
 
if (FirstInitHash != SecondInitHash) {
       DiagError(FieldDifferentInitializers)
           << FirstII << FirstInitializer->getSourceRange();
       DiagNote(FieldDifferentInitializers)
           << SecondII << SecondInitializer->getSourceRange();
       return true;
     }
   }
 
+  if (diagnoseSubMismatchAttr(FirstRecord, FirstModule, SecondModule,
+                              FirstField, SecondField))
+    return true;
+
   return false;
 }
 
 bool ODRDiagsEmitter::diagnoseSubMismatchTypedef(
     const NamedDecl *FirstRecord, StringRef FirstModule, StringRef SecondModule,
     const TypedefNameDecl *FirstTD, const TypedefNameDecl *SecondTD,
     bool IsTypeAlias) const {
   enum ODRTypedefDifference {
     TypedefName,
     TypedefType,
   };
 
bool ODRDiagsEmitter::diagnoseSubMismatchVar(const NamedDecl *FirstRecord,
 
   const bool FirstIsConstexpr = FirstVD->isConstexpr();
   const bool SecondIsConstexpr = SecondVD->isConstexpr();
   if (FirstIsConstexpr != SecondIsConstexpr) {
     DiagError(VarConstexpr) << FirstName << FirstIsConstexpr;
     DiagNote(VarConstexpr) << SecondName << SecondIsConstexpr;
     return true;
   }
+
+  if (diagnoseSubMismatchAttr(FirstRecord, FirstModule, SecondModule, FirstVD,
+                              SecondVD))
+    return true;
+  return false;
+}
+
+bool ODRDiagsEmitter::diagnoseSubMismatchAttr(
+    const NamedDecl *FirstContainer, StringRef FirstModule,
+    StringRef SecondModule, const NamedDecl *FirstDecl,
+    const NamedDecl *SecondDecl) const {
+  llvm::SmallVector<const Attr *, 2> FirstAttrs;
+  llvm::SmallVector<const Attr *, 2> SecondAttrs;
+  ODRHash::getHashableAttrs(FirstDecl, FirstAttrs);
+  ODRHash::getHashableAttrs(SecondDecl, SecondAttrs);
+  if (FirstAttrs.empty() && SecondAttrs.empty())
+    return false;
+
+  enum ODRAttributeDifference {
+    AttributeKind,
+    AttributeArguments,
+  };
+
+  auto ComputeAttrODRHash = [](const Attr *A) {
+    ODRHash Hasher;
+    Hasher.AddAttr(A);
+    return Hasher.CalculateHash();
+  };
+  auto FullAttributeText = [](const Attr *A, const ASTContext &Ctx) {
+    std::string FullText;
+    llvm::raw_string_ostream OutputStream(FullText);
+    A->printPretty(OutputStream, Ctx.getPrintingPolicy());
+    return OutputStream.str();
+  };
+  auto DiagError = [FirstContainer, FirstDecl, FirstModule,
+                    this](ODRAttributeDifference DiffType) {
+    return Diag(FirstDecl->getLocation(),
+                diag::err_module_odr_violation_attribute)
+           << (FirstContainer ? FirstContainer : FirstDecl)
+           << FirstModule.empty() << FirstModule << (FirstContainer != nullptr)
+           << FirstDecl->getIdentifier() << DiffType;
+  };
+  auto DiagNote = [FirstContainer, SecondDecl, SecondModule,
+                   this](ODRAttributeDifference DiffType) {
+    return Diag(SecondDecl->getLocation(),
+                diag::note_module_odr_violation_attribute)
+           << SecondModule << (FirstContainer != nullptr)
+           << SecondDecl->getIdentifier() << DiffType;
+  };
+  auto DiagNoteAttrLoc = [this](const Attr *A) {
+    if (A)
+      Diag(A->getLocation(), diag::note_attribute_specified_here)
+          << A->getRange();
+  };
+
+  const Attr *LHS = nullptr;
+  const Attr *RHS = nullptr;
+  unsigned NumFirstAttrs = FirstAttrs.size();
+  unsigned NumSecondAttrs = SecondAttrs.size();
+  unsigned MaxNumAttrs = std::max(NumFirstAttrs, NumSecondAttrs);
+  for (unsigned I = 0; I < MaxNumAttrs; ++I) {
+    if (I < NumFirstAttrs)
+      LHS = FirstAttrs[I];
+    if (I < NumSecondAttrs)
+      RHS = SecondAttrs[I];
+
+    if (!LHS || !RHS || LHS->getKind() != RHS->getKind()) {
+      DiagError(AttributeKind)
+          << (LHS != nullptr) << LHS << FirstDecl->getSourceRange();
+      DiagNoteAttrLoc(LHS);
+      DiagNote(AttributeKind)
+          << (RHS != nullptr) << RHS << SecondDecl->getSourceRange();
+      DiagNoteAttrLoc(RHS);
+      return true;
+    }
+    if (ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)) {
+      DiagError(AttributeArguments)
+          << FullAttributeText(LHS, Context) << LHS->getRange();
+      DiagNoteAttrLoc(LHS);
+      DiagNote(AttributeArguments)
+          << FullAttributeText(RHS, Context) << RHS->getRange();
+      DiagNoteAttrLoc(RHS);
+      return true;
+    }
+  }
+
   return false;
 }
 
 ODRDiagsEmitter::DiffResult
 ODRDiagsEmitter::FindTypeDiffs(DeclHashes &FirstHashes,
                                DeclHashes &SecondHashes) {
   auto DifferenceSelector = [](const Decl *D) {
     assert(D && "valid Decl required");
for (auto Pair : llvm::zip(FirstTemplateParams, SecondTemplateParams)) {
       Diag(SecondDecl->getLocation(),
            diag::note_module_odr_violation_template_parameter)
           << SecondModule << SecondDecl->getSourceRange() << NoteDiffType
           << hasSecondArg << SecondName;
       return true;
     }
   }
 
+  if (diagnoseSubMismatchAttr(/*FirstContainer=*/nullptr, FirstModule,
+                              SecondModule, FirstRecord, SecondRecord))
+    return true;
+
   auto PopulateHashes = [](DeclHashes &Hashes, const RecordDecl *Record,
                            const DeclContext *DC) {
     for (const Decl *D : Record->decls()) {
       if (!ODRHash::isDeclToBeProcessed(D, DC))
         continue;
       Hashes.emplace_back(D, computeODRHash(D));
     }
   };
if (FirstTemplateArgs && SecondTemplateArgs) {
           continue;
 
         DiagMethodError(MethodDifferentTemplateArgument) << FirstTA << i + 1;
         DiagMethodNote(MethodDifferentTemplateArgument) << SecondTA << i + 1;
         return true;
       }
     }
 
+    if (diagnoseSubMismatchAttr(FirstRecord, FirstModule, SecondModule,
+                                FirstMethod, SecondMethod))
+      return true;
+
     // Compute the hash of the method as if it has no body.
     auto ComputeCXXMethodODRHash = [](const CXXMethodDecl *D) {
       ODRHash Hasher;
       Hasher.AddFunctionDecl(D, true /*SkipBody*/);
       return Hasher.CalculateHash();
     };
 
     // Compare the hash generated to the hash stored.  A difference means
if (FirstInit && SecondInit &&
           << (I + 1) << SecondInit->getSourceRange();
       return true;
     }
 
     assert(computeODRHash(FirstParam) == computeODRHash(SecondParam) &&
            "Undiagnosed parameter difference.");
   }
 
+  if (diagnoseSubMismatchAttr(/*FirstContainer=*/nullptr, FirstModule,
+                              SecondModule, FirstFunction, SecondFunction))
+    return true;
+
   // If no error has been generated before now, assume the problem is in
   // the body and generate a message.
   DiagError(FirstFunction->getLocation(), FirstFunction->getSourceRange(),
             FunctionBody);
   DiagNote(SecondFunction->getLocation(), SecondFunction->getSourceRange(),
            FunctionBody);
   return true;
 }
if (!FirstUnderlyingType.isNull() && !SecondUnderlyingType.isNull()) {
     if (computeODRHash(FirstUnderlyingType) !=
         computeODRHash(SecondUnderlyingType)) {
       DiagError(FirstEnum, DifferentSpecifiedTypes) << FirstUnderlyingType;
       DiagNote(SecondEnum, DifferentSpecifiedTypes) << SecondUnderlyingType;
       return true;
     }
   }
 
+  if (diagnoseSubMismatchAttr(/*FirstContainer=*/nullptr, FirstModule,
+                              SecondModule, FirstEnum, SecondEnum))
+    return true;
+
   // Compare enum constants.
   using DeclHashes =
       llvm::SmallVector<std::pair<const EnumConstantDecl *, unsigned>, 4>;
   auto PopulateHashes = [FirstEnum](DeclHashes &Hashes, const EnumDecl *Enum) {
     for (const Decl *D : Enum->decls()) {
       // Due to decl merging, the first EnumDecl is the parent of
       // Decls in both records.
       if (!ODRHash::isDeclToBeProcessed(D, FirstEnum))

clang/lib/AST/ODRHash.cpp

 //===-- ODRHash.cpp - Hashing to diagnose ODR failures ----------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file implements the ODRHash class, which calculates a hash based
 /// on AST nodes, which is stable across different runs.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ODRHash.h"
 
+#include "clang/AST/Attr.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/TypeVisitor.h"
 
 using namespace clang;
 
 void ODRHash::AddStmt(const Stmt *S) {
void AddTemplateArgument(TemplateArgument TA) {
     Hash.AddTemplateArgument(TA);
   }
 
   void Visit(const Decl *D) {
     ID.AddInteger(D->getKind());
     Inherited::Visit(D);
   }
 
+  void VisitDecl(const Decl *D) {
+    Hash.AddAttrs(D);
+    Inherited::VisitDecl(D);
+  }
+
   void VisitNamedDecl(const NamedDecl *D) {
     Hash.AddDeclarationName(D->getDeclName());
     Inherited::VisitNamedDecl(D);
   }
 
   void VisitValueDecl(const ValueDecl *D) {
     if (!isa<FunctionDecl>(D)) {
       AddQualType(D->getType());
switch (D->getKind()) {
     case Decl::StaticAssert:
     case Decl::TypeAlias:
     case Decl::Typedef:
     case Decl::Var:
       return true;
   }
 }
 
+void ODRHash::getHashableAttrs(const Decl *D,
+                               SmallVectorImpl<const Attr *> &HashableAttrs) {
+  HashableAttrs.clear();
+  if (!D->hasAttrs())
+    return;
+
+  llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs),
+                [](const Attr *A) { return !A->isImplicit(); });
+}
+
+void ODRHash::AddAttrs(const Decl *D) {
+  llvm::SmallVector<const Attr *, 2> Attrs;
+  getHashableAttrs(D, Attrs);
+  ID.AddInteger(Attrs.size());
+  for (const Attr *A : Attrs)
+    AddAttr(A);
+}
+
+void ODRHash::AddAttr(const Attr *A) {
+  ID.AddInteger(A->getKind());
+
+  // FIXME: This should be auto-generated as part of Attr.td
+  switch (A->getKind()) {
+  case attr::Aligned: {
+    auto *M = cast<AlignedAttr>(A);
+    ID.AddBoolean(M->isAlignmentExpr());
+    if (M->isAlignmentExpr()) {
+      Expr *AlignmentExpr = M->getAlignmentExpr();
+      ID.AddBoolean(AlignmentExpr);
+      if (AlignmentExpr)
+        AddStmt(AlignmentExpr);
+    } else {
+      ID.AddString(M->getAlignmentType()->getType().getAsString());
+    }
+    break;
+  }
+  case attr::EnumExtensibility: {
+    auto *M = cast<EnumExtensibilityAttr>(A);
+    ID.AddInteger(M->getExtensibility());
+    break;
+  }
+  default:
+    break;
+  }
+}
+
 void ODRHash::AddSubDecl(const Decl *D) {
   assert(D && "Expecting non-null pointer.");
 
   ODRDeclVisitor(ID, *this).Visit(D);
 }
 
 void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
   assert(Record && Record->hasDefinition() &&
const auto *Specialization =
             dyn_cast<ClassTemplateSpecializationDecl>(D);
   AddBoolean(Specialization);
   if (Specialization) {
     const TemplateArgumentList &List = Specialization->getTemplateArgs();
     ID.AddInteger(List.size());
     for (const TemplateArgument &TA : List.asArray())
       AddTemplateArgument(TA);
   }
+
+  AddAttrs(D);
 }
 
 namespace {
 // Process a Type pointer.  Add* methods call back into ODRHash while Visit*
 // methods process the relevant parts of the Type.
 class ODRTypeVisitor : public TypeVisitor<ODRTypeVisitor> {
   typedef TypeVisitor<ODRTypeVisitor> Inherited;
   llvm::FoldingSetNodeID &ID;

clang/test/Modules/odr_hash.cpp

class S {
   static auto Function2 = invalid2<Num>;
   // expected-error@first.h:* {{'Types::DependentSizedExtVector::invalid2' has different definitions in different modules; definition in module 'FirstModule' first difference is function body}}
   // expected-note@second.h:* {{but in 'SecondModule' found a different body}}
   static auto Function3 = valid<Num>;
 };
 #endif
 }  // namespace DependentSizedExtVector
 
+namespace Attributes {
+#if defined(FIRST)
+struct __attribute__((packed)) PackingPresence {
+  char x;
+  long y;
+};
+#elif defined(SECOND)
+struct PackingPresence {
+  char x;
+  long y;
+};
+#else
+PackingPresence testPackingAttributePresence;
+// expected-error@first.h:* {{'Types::Attributes::PackingPresence' has different definitions in different modules; first difference is definition in module 'FirstModule' found attribute 'packed'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found no attribute}}
+#endif
+
+#if defined(FIRST)
+struct AlignmentPresence {
+  char x;
+  alignas(8) char y;
+};
+struct DifferentAlignmentKeywords {
+  char x;
+  char __attribute__((aligned(8))) y;
+};
+struct DifferentAlignmentValues {
+  char x;
+  alignas(4) char y;
+};
+struct AlignmentWithTypedefType {
+  char x;
+  alignas(float) char y;
+};
+#elif defined(SECOND)
+struct AlignmentPresence {
+  char x;
+  char y;
+};
+struct DifferentAlignmentKeywords {
+  char x;
+  alignas(8) char y;
+};
+struct DifferentAlignmentValues {
+  char x;
+  alignas(8) char y;
+};
+typedef float MyFloat;
+struct AlignmentWithTypedefType {
+  char x;
+  alignas(MyFloat) char y;
+};
+#else
+AlignmentPresence testAlignmentAttributePresence;
+// expected-error@first.h:* {{'Types::Attributes::AlignmentPresence' has different definitions in different modules; first difference is definition in module 'FirstModule' found 'y' with attribute 'alignas'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found 'y' with no attribute}}
+DifferentAlignmentKeywords testDifferentAlignmentKeywords; // expected no errors as different keywords have the same meaning
+DifferentAlignmentValues testDifferentAlignments;
+// expected-error@first.h:* {{'Types::Attributes::DifferentAlignmentValues' has different definitions in different modules; first difference is definition in module 'FirstModule' found 'y' with attribute ' alignas(4)'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found 'y' with different attribute argument ' alignas(8)'}}
+// expected-note@second.h:* {{attribute specified here}}
+AlignmentWithTypedefType testAligningAsTheSameTypeButDifferentName;
+// expected-error@first.h:* {{'Types::Attributes::AlignmentWithTypedefType' has different definitions in different modules; first difference is definition in module 'FirstModule' found 'y' with attribute ' alignas(alignof(float))'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found 'y' with different attribute argument ' alignas(alignof(MyFloat))'}}
+// expected-note@second.h:* {{attribute specified here}}
+#endif
+
+#if defined(FIRST)
+struct AttributeOnMethod {
+  int test(int x, int y);
+};
+struct AttributeOnStaticField {
+  __attribute__((unused)) static int x;
+};
+#elif defined(SECOND)
+struct AttributeOnMethod {
+  __attribute__((optnone)) int test(int x, int y);
+};
+struct AttributeOnStaticField {
+  static int x;
+};
+#else
+AttributeOnMethod testAttributeDifferenceOnMethods;
+// expected-error@first.h:* {{'Types::Attributes::AttributeOnMethod' has different definitions in different modules; first difference is definition in module 'FirstModule' found 'test' with no attribute}}
+// expected-note@second.h:* {{but in 'SecondModule' found 'test' with attribute 'optnone'}}
+// expected-note@second.h:* {{attribute specified here}}
+AttributeOnStaticField testAttributeDifferenceOnStaticFields;
+// expected-error@first.h:* {{'Types::Attributes::AttributeOnStaticField' has different definitions in different modules; first difference is definition in module 'FirstModule' found 'x' with attribute 'unused'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found 'x' with no attribute}}
+#endif
+
+#if defined(FIRST)
+struct __attribute__((packed)) __attribute__((aligned(8))) AttributeOrder {
+  char x;
+  long y;
+};
+#elif defined(SECOND)
+struct __attribute__((aligned(8))) __attribute__((packed)) AttributeOrder {
+  char x;
+  long y;
+};
+#else
+AttributeOrder testOrderOfAttributes;
+// expected-error@first.h:* {{'Types::Attributes::AttributeOrder' has different definitions in different modules; first difference is definition in module 'FirstModule' found attribute 'packed'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found attribute 'aligned'}}
+// expected-note@second.h:* {{attribute specified here}}
+#endif
+
+#if defined(FIRST)
+__attribute__((naked)) void testFunctionAttr(void) {}
+#elif defined(SECOND)
+void testFunctionAttr(void) {}
+#else
+auto testFunctionAttrVar = testFunctionAttr;
+// expected-error@second.h:* {{'Types::Attributes::testFunctionAttr' has different definitions in different modules; first difference is definition in module 'SecondModule' found no attribute}}
+// expected-note@first.h:* {{but in 'FirstModule' found attribute 'naked'}}
+// expected-note@first.h:* {{attribute specified here}}
+#endif
+
+#if defined(FIRST)
+enum __attribute__((enum_extensibility(open))) EnumExtensibilityPresence {
+  kEnumExtensibilityPresence0,
+};
+enum __attribute__((enum_extensibility(open))) EnumExtensibilityValues {
+  kEnumExtensibilityValues0,
+};
+#elif defined(SECOND)
+enum EnumExtensibilityPresence {
+  kEnumExtensibilityPresence0,
+};
+enum __attribute__((enum_extensibility(closed))) EnumExtensibilityValues {
+  kEnumExtensibilityValues0,
+};
+#else
+EnumExtensibilityPresence testEnumAttrPresence;
+// expected-error@first.h:* {{'Types::Attributes::EnumExtensibilityPresence' has different definitions in different modules; first difference is definition in module 'FirstModule' found attribute 'enum_extensibility'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found no attribute}}
+EnumExtensibilityValues testEnumAttrValue;
+// expected-error@first.h:* {{'Types::Attributes::EnumExtensibilityValues' has different definitions in different modules; first difference is definition in module 'FirstModule' found attribute ' __attribute__((enum_extensibility("open")))'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found different attribute argument ' __attribute__((enum_extensibility("closed")))'}}
+// expected-note@second.h:* {{attribute specified here}}
+#endif
+
+#if defined(FIRST)
+#define PACKED __attribute__((packed))
+struct PACKED AttributeInMacro {
+  char x;
+  long y;
+};
+
+#pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(record(unless(is_union))))
+struct AttributeInPragma { char x; };
+#pragma clang attribute pop
+
+struct __attribute__((packed)) AttributeInherited;
+struct AttributeInherited {
+  char x;
+  long y;
+};
+#elif defined(SECOND)
+struct AttributeInMacro {
+  char x;
+  long y;
+};
+
+struct AttributeInPragma { char x; };
+
+struct AttributeInherited {
+  char x;
+  long y;
+};
+#else
+AttributeInMacro testDiagnosticWhenAttributeIsInMacro;
+// expected-error@first.h:* {{'Types::Attributes::AttributeInMacro' has different definitions in different modules; first difference is definition in module 'FirstModule' found attribute 'packed'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-text-not-enforceable@first.h:* {{expanded from macro 'PACKED'}}
+// expected-note@second.h:* {{but in 'SecondModule' found no attribute}}
+AttributeInPragma testDiagnosticShowsPragmaLocation;
+// expected-error@first.h:* {{'Types::Attributes::AttributeInPragma' has different definitions in different modules; first difference is definition in module 'FirstModule' found attribute 'abi_tag'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found no attribute}}
+AttributeInherited testShowLocationOnFwdDecl;
+// expected-error@first.h:* {{'Types::Attributes::AttributeInherited' has different definitions in different modules; first difference is definition in module 'FirstModule' found attribute 'packed'}}
+// expected-note@first.h:* {{attribute specified here}}
+// expected-note@second.h:* {{but in 'SecondModule' found no attribute}}
+#endif
+}  // namespace Attributes
+
 namespace InjectedClassName {
 #if defined(FIRST)
 struct Invalid {
   template <int>
   struct L2 {
     template <int>
     struct L3 {
       L3 *x;