New ODR checker for modules

[rtrieu]

Summary

Early prototype of an improved ODR checker for Clang and modules. The current ODR checking of classes is limited to a small number of attributes such as number of methods and base classes. This still allows a number of ODR violations to pass through undetected which only manifests as problems during linking. This improved ODR checker calculates a hash based on the class contents, then checks that the two decls have the same hash before allowing them to be merged. The test case shows a number of ODR violations that the current Clang checker would not have caught.

The hashing relies on three visitors for Stmt's, Decl's, and Type's. For Stmt's, the visitor behind Stmt::Profile was refactored so that a hash could be generated without depending on pointers. For Decl's, a new DeclVisitor was created. The Type visitor has not been written yet. Instead, Types are converted into a string as a stand-in.

The other area for improvement is the diagnostic message. Most have the default message stating the class has different definitions in different modules. New specific messages will need to be created to supplement the default message.

[rtrieu]

Add function void ODRHash(llvm::FoldingSetNodeID &ID) to several classes for computing the hash. Decl, Stmt, TemplateArgument, Type and QualType now have this function, and can call among each others' functions.

[dblaikie]

lib/AST/DeclBase.cpp

1810–1812 ↗

(On Diff #68278)

Inconsistent {} on single line block (in VisitEnumConstantDecl above {} are not used on a single line block) - usually drop the {} on single line blocks.

(several other instances in this patch)

test/Modules/Inputs/odr_hash/first.h

6–9

It might make this test more readable if all the types were in one file, maybe like this:

#ifdef FIRST
  struct S2 { friend S2Friend1 };
#endif
  struct S2 { friend S2Friend2 };
#endif

Etc... - and it could just be a textual header that the two headers include (one header #defines/#undefs the appropriate thing and one doesn't). But maybe that's too complicated - I'm not sure.

[rtrieu]

Add a more detailed error message to let users know where the two records differ. This replaces the generic error which only stated that two definitions are different without any details.

[rtrieu]

lib/AST/DeclBase.cpp

1810–1812 ↗

(On Diff #68278)

These should be more consistent now.

test/Modules/Inputs/odr_hash/first.h

6–9

Modified so entire test is now in one file. The separate headers and module maps are generated into the temp folder for each run.

[rsmith]

There are a bunch of cases here that do this:

if (auto t = getThing())
  ID.addThing(t);
if (auto t = getOtherThing())
  ID.addThing(t);

That will result in hash collisions between objects with thing and objects with otherthing (for instance, struct A { int n : 1; }; and struct A { int n = 1; }; appear to hash the same).

And there are some cases where you add a list of objects without first adding the size, which is liable to allow collisions.

I'm not too worried about these cases, since this is just a hash anyway, and is only a best-effort attempt to check for ODR violations, but some of them look like they'd be easy enough to fix, so I figure why not :)

Anyway, this looks really good. Do you know if the computation of the ODR hash has any measurable performance impact when building a large module? I'm not really expecting one, but it seems worth checking just in case.

lib/AST/DeclBase.cpp

1827 ↗

(On Diff #73388)

You should not include the default argument if it was inherited from a previous declaration. That can happen in a friend declaration:

// module 1
void f(int = 0);
struct X { friend void f(int); }; // has (inherited) default arg

// module 2
struct X { friend void f(int); }; // has no default arg

[rtrieu]

In D21675#560297, @rsmith wrote:

There are a bunch of cases here that do this:

if (auto t = getThing())
  ID.addThing(t);
if (auto t = getOtherThing())
  ID.addThing(t);

That will result in hash collisions between objects with thing and objects with otherthing (for instance, struct A { int n : 1; }; and struct A { int n = 1; }; appear to hash the same).

And there are some cases where you add a list of objects without first adding the size, which is liable to allow collisions.

I'm not too worried about these cases, since this is just a hash anyway, and is only a best-effort attempt to check for ODR violations, but some of them look like they'd be easy enough to fix, so I figure why not :)

The hashing now does:

auto t = getThing();
ID.addBoolean(t);
if (t)
  ID.addThing(t);

The added Boolean value prevents the hash collision.

Anyway, this looks really good. Do you know if the computation of the ODR hash has any measurable performance impact when building a large module? I'm not really expecting one, but it seems worth checking just in case.

I ran some comparisons and did not see any performance impact.

lib/AST/DeclBase.cpp

1827 ↗

(On Diff #73388)

The visitor for friend decl only uses the type (void (int)) and name (f) of the friend. It doesn't process the type, so default arguments doesn't matter for this case.

[rtrieu]

This is a redesign of the ODR checker which was discovered to have several shortcomings when run over test cases. The old version mainly used depth-first processing of AST nodes to gather the information to be hashed. In some instances, a recursive loop developed preventing the termination of the hashing function.

This version of the ODR checker will queue Type and Decl pointers into a vector. When the hashing function needs to refer to the pointers, it will use the index of the pointers' location in the vector instead. After any in progress hashing is finished, unprocessed pointers in the vector will be processed. Other AST nodes, such as Stmt's and TemplateArgument's are processed when received.

The design change also necessitated creation of an ODRHash class to manage the queued nodes. This is in ODRHash.{cpp,h} and most of the hashing logic is also moved into those files. Only the hashing for Stmt is left in StmtProfile.h since it shares code with Stmt::Profile.

[rsmith]

Thanks! I assume there's still no measurable performance impact?

include/clang/AST/ODRHash.h

54 ↗

(On Diff #82702)

Is this always the same as Pointers.size()?

lib/AST/ODRHash.cpp

35 ↗

(On Diff #82702)

AddBoolean for clarity maybe?

335–337 ↗

(On Diff #82702)

You will presumably need to filter out the implicit declarations before you emit the size of the list, otherwise a DeclContext with an implicit declaration will hash differently from one without.

493–495 ↗

(On Diff #82702)

Given that AddStmt already writes out an "is null" flag, could you use AddStmt(hasDefaultArgument ? D->getDefaultArgument() : nullptr) here?

lib/Serialization/ASTReader.cpp

9015–9027 ↗

(On Diff #82702)

Can you factor this out into a local lambda or helper class? These dozen lines are repeated quite a lot of times here with only small variations.

[rtrieu]

Comments have been addressed. I will be testing it for performance impact next.

include/clang/AST/ODRHash.h

54 ↗

(On Diff #82702)

Yes it is. It has been removed and Pointers.size() is used now.

[rsmith]

Thanks, looks good assuming your performance testing doesn't uncover anything.

lib/AST/ODRHash.cpp

319–321 ↗

(On Diff #84340)

I think you can remove these lines: no-one should be calling this function with a null declaration or an implicit declaration.

[rtrieu]

From testing, there is no difference when compiling with pre-compiled headers. However, when building the headers, there is a 3-4% impact on compile time.

[rtrieu]

After changing the ODRHash class a bit, the new performance numbers are 3% in debug and 1-1.5% in release builds.

[teemperor]

I feel like we have a really similar use case in the Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried substituting the call to the statement hashing with a call to the CloneDetection API and it seems that most tests pass and the remaining fails are just minor fixable differences (like ::foo() and foo() having different hash codes).

Would be nice if we could make Stmt::Profile, ODRHash and the CloneDetection use the same backend. We improved a few things that we no longer have the periodic reallocs from the vector inside NodeIDSet and that we use MD5. Also there are are some future plans on how we can better prevent regressions when we add/extend AST nodes. Thoughts?

lib/AST/ODRHash.cpp

11 ↗

(On Diff #84340)

"Instruction class" -> "ODRHash class"

[rtrieu]

In D21675#654659, @teemperor wrote:

I feel like we have a really similar use case in the Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried substituting the call to the statement hashing with a call to the CloneDetection API and it seems that most tests pass and the remaining fails are just minor fixable differences (like ::foo() and foo() having different hash codes).

Would be nice if we could make Stmt::Profile, ODRHash and the CloneDetection use the same backend. We improved a few things that we no longer have the periodic reallocs from the vector inside NodeIDSet and that we use MD5. Also there are are some future plans on how we can better prevent regressions when we add/extend AST nodes. Thoughts?

It would help to understand your use case better. Does CloneDetection care about ::foo versus foo? How about different types with the same name? ODR checking assumes identical token sequences, so it does care about extra "::". ODR checking also will process information about type while CloneDetection looks like it only uses the type name.

I see that CloneDetector uses both llvm::MD5 and llvm::FoldingSetNodeID, and only adds data via StringRef. MD5 will add the bytes as data, while FoldingSetNodeID will add the size of the string, then the bytes as data. This means MD5 may have collisions when two strings are added back to back while FoldingSetNodeID will store extra data for every integer processed. FoldingSetNodeID doesn't have this problem if AddInteger is used. Were the reallocs a big problem for CloneDetection?

I don't thinking merging Stmt::Profile, ODRHash, and CloneDetection would be a good idea unless the hashes needed are very similar. Stmt::Profile and ODRHash already share a base for Stmt processing, which might be extendable to CloneDetection as well, but a full merge may be difficult.

Sadly, we don't have a good story for when an AST node gets changed with new properties. The Stmt profiler does declare all the visit methods in the class definition, so that will catch any new Stmt nodes without a new visit method.

[rsmith]

In D21675#654659, @teemperor wrote:

Would be nice if we could make Stmt::Profile, ODRHash and the CloneDetection use the same backend.

This code is *already* reusing the Stmt::Profile code for hashing of statements. Why was a different mechanism invented for CloneDetection? If it doesn't have different requirements, maybe it should be rewritten in terms of the Stmt::Profile implementation too.

[rtrieu]

Changes made to the ODR hash algorithm:

Separated Decl and Type pointers into their own DenseMap's.
Removed the queue of pointers to process at the end. Instead, process pointers at first use.
Save Boolean values and add them together at the end to reduce bits wasted. Shrinks data from bools 32x.

With these changes, the overhead is now 1-1.5%

[rtrieu]

This patch will be landing in small chunks to hopefully avoid the large reverts.

include/clang/AST/DeclCXX.h

struct DefinitionData {
     unsigned HasDeclaredCopyAssignmentWithConstParam : 1;
 
     /// \brief Whether this class describes a C++ lambda.
     unsigned IsLambda : 1;
 
     /// \brief Whether we are currently parsing base specifiers.
     unsigned IsParsingBaseSpecifiers : 1;
 
+    /// \brief A hash of parts of the class to help in ODR checking.
+    unsigned ODRHash;
+
     /// \brief The number of base class specifiers in Bases.
     unsigned NumBases;
 
     /// \brief The number of virtual base class specifiers in VBases.
     unsigned NumVBases;
 
     /// \brief Base classes of this class.
     ///
public:
   }
 
   void setIsParsingBaseSpecifiers() { data().IsParsingBaseSpecifiers = true; }
 
   bool isParsingBaseSpecifiers() const {
     return data().IsParsingBaseSpecifiers;
   }
 
+  void computeODRHash();
+  unsigned getODRHash() { return data().ODRHash; }
+
   /// \brief Sets the base classes of this struct or class.
   void setBases(CXXBaseSpecifier const * const *Bases, unsigned NumBases);
 
   /// \brief Retrieves the number of base classes of this class.
   unsigned getNumBases() const { return data().NumBases; }
 
   typedef llvm::iterator_range<base_class_iterator> base_class_range;
   typedef llvm::iterator_range<base_class_const_iterator>

include/clang/AST/Stmt.h

public:
   ///
   /// \param Canonical whether the profile should be based on the canonical
   /// representation of this statement (e.g., where non-type template
   /// parameters are identified by index/level rather than their
   /// declaration pointers) or the exact representation of the statement as
   /// written in the source.
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
                bool Canonical) const;
+  void ODRProfile(llvm::FoldingSetNodeID &ID) const;
 };
 
 /// DeclStmt - Adaptor class for mixing declarations with statements and
 /// expressions. For example, CompoundStmt mixes statements, expressions
 /// and declarations (variables, types). Another example is ForStmt, where
 /// the first statement can be an expression or a declaration.
 ///
 class DeclStmt : public Stmt {

lib/AST/DeclCXX.cpp

 //
 //===----------------------------------------------------------------------===//
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 using namespace clang;
 
: UserDeclaredConstructor(false), UserDeclaredSpecialMembers(0),
       DefaultedDefaultConstructorIsConstexpr(true),
       HasConstexprDefaultConstructor(false),
       HasNonLiteralTypeFieldsOrBases(false), ComputedVisibleConversions(false),
       UserProvidedDefaultConstructor(false), DeclaredSpecialMembers(0),
       ImplicitCopyConstructorHasConstParam(true),
       ImplicitCopyAssignmentHasConstParam(true),
       HasDeclaredCopyConstructorWithConstParam(false),
       HasDeclaredCopyAssignmentWithConstParam(false), IsLambda(false),
-      IsParsingBaseSpecifiers(false), NumBases(0), NumVBases(0), Bases(),
-      VBases(), Definition(D), FirstFriend() {}
+      IsParsingBaseSpecifiers(false), ODRHash(0), NumBases(0), NumVBases(0),
+      Bases(), VBases(), Definition(D), FirstFriend() {}
 
 CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getBasesSlowCase() const {
   return Bases.get(Definition->getASTContext().getExternalSource());
 }
 
 CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getVBasesSlowCase() const {
   return VBases.get(Definition->getASTContext().getExternalSource());
 }
for (int I = 0, E = VBases.size(); I != E; ++I) {
     if (!Type->isDependentType())
       addedClassSubobject(Type->getAsCXXRecordDecl());
     data().getVBases()[I] = *VBases[I];
   }
 
   data().IsParsingBaseSpecifiers = false;
 }
 
+// This DeclVisitor computes a hash for the common Decl types
+// encountered from a CXXRecordDecl.
+class ODRDeclVisitor : public DeclVisitor<ODRDeclVisitor> {
+  typedef DeclVisitor<ODRDeclVisitor> Inherited;
+  llvm::FoldingSetNodeID &ID;
+
+public:
+  ODRDeclVisitor(llvm::FoldingSetNodeID &ID)
+      : ID(ID) {}
+
+  unsigned ComputeHash() { return ID.ComputeHash(); }
+
+  void Visit(Decl *D) {
+    if (!D)
+      return;
+    if (D->isImplicit())
+      return;
+    if (D->isInvalidDecl())
+      return;
+    ID.AddInteger(D->getKind());
+    ID.AddInteger(D->hasAttrs());
+
+    if (auto *DC = dyn_cast<DeclContext>(D))
+      for (auto *SubD : DC->decls())
+        Visit(SubD);
+
+    Inherited::Visit(D);
+  }
+
+  void VisitLabelDecl(LabelDecl *D) {
+    Inherited::VisitLabelDecl(D);
+  }
+
+  void VisitEnumDecl(EnumDecl *D) {
+    ID.AddInteger(D->isFixed());
+    if (D->isFixed())
+      VisitType(D->getIntegerType());
+    ID.AddInteger(D->isScoped());
+    ID.AddInteger(D->isScopedUsingClassTag());
+
+    Inherited::VisitEnumDecl(D);
+  }
+
+  void VisitEnumConstantDecl(EnumConstantDecl *D) {
+    if (auto *E = D->getInitExpr())
+      VisitStmt(E);
+
+    Inherited::VisitEnumConstantDecl(D);
+  }
+
+  void VisitNamedDecl(NamedDecl *D) {
+    if (IdentifierInfo *II = D->getIdentifier()) {
+      ID.AddString(II->getName());
+    }
+    Inherited::VisitNamedDecl(D);
+  }
+
+  void VisitValueDecl(ValueDecl *D) {
+    VisitType(D->getType());
+    Inherited::VisitValueDecl(D);
+  }
+
+  void VisitAccessSpecDecl(AccessSpecDecl *D) {
+    ID.AddInteger(D->getAccess());
+    Inherited::VisitAccessSpecDecl(D);
+  }
+
+  void VisitFriendDecl(FriendDecl *D) {
+    if (TypeSourceInfo *TSI = D->getFriendType())
+      VisitType(TSI->getType());
+    unsigned NumLists = D->getFriendTypeNumTemplateParameterLists();
+    ID.AddInteger(NumLists);
+    for (unsigned i = 0; i < NumLists; ++i)
+      VisitTemplateParameterList(D->getFriendTypeTemplateParameterList(i));
+    Inherited::VisitFriendDecl(D);
+  }
+
+  void VisitStaticAssertDecl(StaticAssertDecl *D) {
+    VisitStmt(D->getAssertExpr());
+    ID.AddString(D->getMessage()->getString());
+    Inherited::VisitStaticAssertDecl(D);
+  }
+
+  void VisitTypedefNameDecl(TypedefNameDecl *D) {
+    VisitType(D->getUnderlyingType());
+
+    Inherited::VisitTypedefNameDecl(D);
+  }
+
+  void VisitFunctionDecl(FunctionDecl *D) {
+    if (D->hasBody())
+      VisitStmt(D->getBody());
+    else
+      ID.AddInteger(false);
+
+    ID.AddInteger(D->getStorageClass());
+    ID.AddInteger(D->isInlineSpecified());
+    ID.AddInteger(D->isVirtualAsWritten());
+    ID.AddInteger(D->isPure());
+    ID.AddInteger(D->isDeletedAsWritten());
+
+    Inherited::VisitFunctionDecl(D);
+  }
+
+  void VisitCXXMethodDecl(CXXMethodDecl *D) {
+    ID.AddInteger(D->isStatic());
+    ID.AddInteger(D->isInstance());
+    ID.AddInteger(D->isConst());
+    ID.AddInteger(D->isVolatile());
+    Inherited::VisitCXXMethodDecl(D);
+  }
+
+  void VisitCXXConstructorDecl(CXXConstructorDecl *D) {
+    ID.AddInteger(D->isExplicitSpecified());
+    ID.AddInteger(D->getNumCtorInitializers());
+    for (auto Initializer : D->inits()) {
+      if (Initializer->isWritten()) {
+        Initializer->getInit()->ODRProfile(ID);
+      }
+    }
+    Inherited::VisitCXXConstructorDecl(D);
+  }
+
+
+  void VisitCXXConversionDecl(CXXConversionDecl *D) {
+    VisitType(D->getConversionType());
+    ID.AddInteger(D->isExplicitSpecified());
+    Inherited::VisitCXXConversionDecl(D);
+  }
+
+  void VisitFieldDecl(FieldDecl *D) {
+    ID.AddInteger(D->isMutable());
+    ID.AddInteger(D->isBitField());
+    if (D->isBitField())
+      D->getBitWidth()->ODRProfile(ID);
+    Inherited::VisitFieldDecl(D);
+  }
+
+  void VisitTemplateDecl(TemplateDecl *D) {
+    if (NamedDecl *ND = D->getTemplatedDecl())
+      Visit(ND);
+    auto *Parameters = D->getTemplateParameters();
+    ID.AddInteger(Parameters->size());
+    for (auto *ND : *Parameters)
+      Visit(ND);
+    Inherited::VisitTemplateDecl(D);
+  }
+
+  void VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
+    Inherited::VisitFunctionTemplateDecl(D);
+  }
+
+  void VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
+    if (D->hasDefaultArgument())
+      VisitType(D->getDefaultArgument());
+    Inherited::VisitTemplateTypeParmDecl(D);
+  }
+
+  void VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
+    if (D->hasDefaultArgument())
+      D->getDefaultArgument()->ODRProfile(ID);
+    Inherited::VisitNonTypeTemplateParmDecl(D);
+  }
+
+  void VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
+    if (D->hasDefaultArgument())
+      VisitTemplateArgument(D->getDefaultArgument().getArgument());
+    Inherited::VisitTemplateTemplateParmDecl(D);
+  }
+
+  void VisitStmt(Stmt *S) {
+    if (!S)
+      return;
+    S->ODRProfile(ID);
+  }
+
+  void VisitTemplateParameterList(TemplateParameterList *TPL) {
+    if (!TPL)
+      return;
+    for (auto * ND : TPL->asArray()) {
+      Visit(ND);
+    }
+  }
+
+  void VisitTemplateArgument(const TemplateArgument& TA) {
+    switch (TA.getKind()) {
+      case TemplateArgument::Null:
+        llvm_unreachable("Require valid TemplateArgument");
+      case TemplateArgument::Type:
+        VisitType(TA.getAsType());
+        return;
+      case TemplateArgument::Declaration:
+        Visit(TA.getAsDecl());
+        return;
+      case TemplateArgument::NullPtr:
+        ID.AddInteger(0);
+        return;
+      case TemplateArgument::Integral:
+        TA.getAsIntegral().Profile(ID);
+        VisitType(TA.getIntegralType());
+        break;
+      case TemplateArgument::Template:
+      case TemplateArgument::TemplateExpansion:
+        Visit(TA.getAsTemplateOrTemplatePattern().getAsTemplateDecl());
+        break;
+      case TemplateArgument::Expression:
+        TA.getAsExpr()->ODRProfile(ID);
+        break;
+      case TemplateArgument::Pack:
+        ID.AddInteger(TA.pack_size());
+        for (auto SubTA : TA.pack_elements())
+          VisitTemplateArgument(SubTA);
+        break;
+    }
+  }
+
+  void VisitType(QualType T) {
+    // TODO: String type is used instead of a proper hash for the type.
+    // Eventually, a hash built from a type visitor should be used.
+    LangOptions options;
+    PrintingPolicy policy(options);
+    policy.AnonymousTagLocations = false;
+    ID.AddString(T.getAsString(policy));
+  }
+};
+
+void CXXRecordDecl::computeODRHash() {
+  if (!DefinitionData)
+    return;
+  llvm::FoldingSetNodeID ID;
+  ODRDeclVisitor Visitor(ID);
+  for (auto *D : decls()) {
+    Visitor.Visit(D);
+  }
+
+  for (auto base : bases()) {
+    ID.AddInteger(base.isVirtual());
+    Visitor.VisitType(base.getType());
+  }
+
+  if (ClassTemplateDecl *TD = getDescribedClassTemplate()) {
+    Visitor.VisitTemplateParameterList(TD->getTemplateParameters());
+  }
+
+  DefinitionData->ODRHash = ID.ComputeHash();
+}
+
 void CXXRecordDecl::addedClassSubobject(CXXRecordDecl *Subobj) {
   // C++11 [class.copy]p11:
   //   A defaulted copy/move constructor for a class X is defined as
   //   deleted if X has:
   //    -- a direct or virtual base class B that cannot be copied/moved [...]
   //    -- a non-static data member of class type M (or array thereof)
   //       that cannot be copied or moved [...]
   if (!Subobj->hasSimpleMoveConstructor())

lib/AST/StmtProfile.cpp

 // This file implements the Stmt::Profile method, which builds a unique bit
 // representation that identifies a statement/expression.
 //
 //===----------------------------------------------------------------------===//
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/DeclVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/StmtVisitor.h"
 #include "llvm/ADT/FoldingSet.h"
 using namespace clang;
 
 namespace {
   class StmtProfiler : public ConstStmtVisitor<StmtProfiler> {
+   protected:
     llvm::FoldingSetNodeID &ID;
-    const ASTContext &Context;
     bool Canonical;
 
   public:
-    StmtProfiler(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
-                 bool Canonical)
-      : ID(ID), Context(Context), Canonical(Canonical) { }
+    StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical)
+        : ID(ID), Canonical(Canonical) {}
+
+    virtual ~StmtProfiler() {}
 
     void VisitStmt(const Stmt *S);
 
 #define STMT(Node, Base) void Visit##Node(const Node *S);
 #include "clang/AST/StmtNodes.inc"
 
     /// \brief Visit a declaration that is referenced within an expression
     /// or statement.
-    void VisitDecl(const Decl *D);
+    virtual void VisitDecl(const Decl *D) = 0;
 
     /// \brief Visit a type that is referenced within an expression or
     /// statement.
-    void VisitType(QualType T);
+    virtual void VisitType(QualType T) = 0;
 
     /// \brief Visit a name that occurs within an expression or statement.
-    void VisitName(DeclarationName Name);
+    virtual void VisitName(DeclarationName Name) = 0;
+
+    /// \brief Visit identifiers that are not in Decl's or Type's.
+    virtual void VisitIdentifierInfo(IdentifierInfo *II) = 0;
 
     /// \brief Visit a nested-name-specifier that occurs within an expression
     /// or statement.
-    void VisitNestedNameSpecifier(NestedNameSpecifier *NNS);
+    virtual void VisitNestedNameSpecifier(NestedNameSpecifier *NNS) = 0;
 
     /// \brief Visit a template name that occurs within an expression or
     /// statement.
-    void VisitTemplateName(TemplateName Name);
+    virtual void VisitTemplateName(TemplateName Name) = 0;
 
     /// \brief Visit template arguments that occur within an expression or
     /// statement.
     void VisitTemplateArguments(const TemplateArgumentLoc *Args,
                                 unsigned NumArgs);
 
     /// \brief Visit a single template argument.
     void VisitTemplateArgument(const TemplateArgument &Arg);
   };
+
+  class StmtProfilerWithPointers : public StmtProfiler {
+    const ASTContext &Context;
+
+  public:
+    StmtProfilerWithPointers(llvm::FoldingSetNodeID &ID,
+                             const ASTContext &Context, bool Canonical)
+        : StmtProfiler(ID, Canonical), Context(Context) {}
+  private:
+    void VisitDecl(const Decl *D) override {
+      ID.AddInteger(D ? D->getKind() : 0);
+
+      if (Canonical && D) {
+        if (const NonTypeTemplateParmDecl *NTTP =
+                dyn_cast<NonTypeTemplateParmDecl>(D)) {
+          ID.AddInteger(NTTP->getDepth());
+          ID.AddInteger(NTTP->getIndex());
+          ID.AddBoolean(NTTP->isParameterPack());
+          VisitType(NTTP->getType());
+          return;
+        }
+
+        if (const ParmVarDecl *Parm = dyn_cast<ParmVarDecl>(D)) {
+          // The Itanium C++ ABI uses the type, scope depth, and scope
+          // index of a parameter when mangling expressions that involve
+          // function parameters, so we will use the parameter's type for
+          // establishing function parameter identity. That way, our
+          // definition of "equivalent" (per C++ [temp.over.link]) is at
+          // least as strong as the definition of "equivalent" used for
+          // name mangling.
+          VisitType(Parm->getType());
+          ID.AddInteger(Parm->getFunctionScopeDepth());
+          ID.AddInteger(Parm->getFunctionScopeIndex());
+          return;
+        }
+
+        if (const TemplateTypeParmDecl *TTP =
+                dyn_cast<TemplateTypeParmDecl>(D)) {
+          ID.AddInteger(TTP->getDepth());
+          ID.AddInteger(TTP->getIndex());
+          ID.AddBoolean(TTP->isParameterPack());
+          return;
+        }
+
+        if (const TemplateTemplateParmDecl *TTP =
+                dyn_cast<TemplateTemplateParmDecl>(D)) {
+          ID.AddInteger(TTP->getDepth());
+          ID.AddInteger(TTP->getIndex());
+          ID.AddBoolean(TTP->isParameterPack());
+          return;
+        }
+      }
+
+      ID.AddPointer(D ? D->getCanonicalDecl() : nullptr);
+    }
+
+    void VisitType(QualType T) override {
+      if (Canonical)
+        T = Context.getCanonicalType(T);
+
+      ID.AddPointer(T.getAsOpaquePtr());
+    }
+
+    void VisitName(DeclarationName Name) override {
+      ID.AddPointer(Name.getAsOpaquePtr());
+    }
+
+    void VisitIdentifierInfo(IdentifierInfo *II) override {
+      ID.AddPointer(II);
+    }
+
+    void VisitNestedNameSpecifier(NestedNameSpecifier *NNS) override {
+      if (Canonical)
+        NNS = Context.getCanonicalNestedNameSpecifier(NNS);
+      ID.AddPointer(NNS);
+    }
+
+    void VisitTemplateName(TemplateName Name) override {
+      if (Canonical)
+        Name = Context.getCanonicalTemplateName(Name);
+
+      Name.Profile(ID);
+    }
+  };
+
+  class StmtProfilerWithoutPointers : public StmtProfiler {
+  public:
+    StmtProfilerWithoutPointers(llvm::FoldingSetNodeID &ID)
+        : StmtProfiler(ID, false) {}
+
+  private:
+    void VisitType(QualType T) override {
+      // TODO: String of type is used instead of a proper hash for the type.
+      // Eventually, a hash built from a type visitor should be used.
+      LangOptions options;
+      PrintingPolicy policy(options);
+      policy.AnonymousTagLocations = false;
+      ID.AddString(T.getAsString(policy));
+    }
+    void VisitName(DeclarationName Name) override {
+      if (Name.isEmpty()) {
+        ID.AddInteger(0);
+        return;
+      }
+      auto Kind = Name.getNameKind();
+      ID.AddInteger(Kind);
+      // TODO: Hash the relevant parts of DeclarationName instead of using
+      // the string.
+      SmallString<64> Buffer;
+      llvm::raw_svector_ostream StrOS(Buffer);
+      Name.print(StrOS, PrintingPolicy(LangOptions()));
+      ID.AddString(Buffer);
+    }
+    void VisitIdentifierInfo(IdentifierInfo *II) override {
+      ID.AddString(II->getName());
+    }
+    void VisitDecl(const Decl *D) override {
+      ID.AddInteger(D ? D->getKind() : 0);
+      if (!D) {
+        return;
+      }
+      if (auto *ND = dyn_cast<NamedDecl>(D)) {
+        ID.AddString(ND->getNameAsString());
+      }
+    }
+    void VisitTemplateName(TemplateName Name) override { }
+    void VisitNestedNameSpecifier(NestedNameSpecifier *NNS) override { }
+  };
 }
 
 void StmtProfiler::VisitStmt(const Stmt *S) {
   assert(S && "Requires non-null Stmt pointer");
   ID.AddInteger(S->getStmtClass());
   for (const Stmt *SubStmt : S->children()) {
     if (SubStmt)
       Visit(SubStmt);
case OffsetOfNode::Array:
       // Expressions handled below.
       break;
 
     case OffsetOfNode::Field:
       VisitDecl(ON.getField());
       break;
 
     case OffsetOfNode::Identifier:
-      ID.AddPointer(ON.getFieldName());
+      VisitIdentifierInfo(ON.getFieldName());
       break;
 
     case OffsetOfNode::Base:
       // These nodes are implicit, and therefore don't need profiling.
       break;
     }
   }
   
StmtProfiler::VisitCXXPseudoDestructorExpr(const CXXPseudoDestructorExpr *S) {
   VisitNestedNameSpecifier(S->getQualifier());
   ID.AddBoolean(S->getScopeTypeInfo() != nullptr);
   if (S->getScopeTypeInfo())
     VisitType(S->getScopeTypeInfo()->getType());
   ID.AddBoolean(S->getDestroyedTypeInfo() != nullptr);
   if (S->getDestroyedTypeInfo())
     VisitType(S->getDestroyedType());
   else
-    ID.AddPointer(S->getDestroyedTypeIdentifier());
+    VisitIdentifierInfo(S->getDestroyedTypeIdentifier());
 }
 
 void StmtProfiler::VisitOverloadExpr(const OverloadExpr *S) {
   VisitExpr(S);
   VisitNestedNameSpecifier(S->getQualifier());
   VisitName(S->getName());
   ID.AddBoolean(S->hasExplicitTemplateArgs());
   if (S->hasExplicitTemplateArgs())
void StmtProfiler::VisitObjCIndirectCopyRestoreExpr(
   ID.AddBoolean(S->shouldCopy());
 }
 
 void StmtProfiler::VisitObjCBridgedCastExpr(const ObjCBridgedCastExpr *S) {
   VisitExplicitCastExpr(S);
   ID.AddBoolean(S->getBridgeKind());
 }
 
-void StmtProfiler::VisitDecl(const Decl *D) {
-  ID.AddInteger(D? D->getKind() : 0);
-
-  if (Canonical && D) {
-    if (const NonTypeTemplateParmDecl *NTTP =
-          dyn_cast<NonTypeTemplateParmDecl>(D)) {
-      ID.AddInteger(NTTP->getDepth());
-      ID.AddInteger(NTTP->getIndex());
-      ID.AddBoolean(NTTP->isParameterPack());
-      VisitType(NTTP->getType());
-      return;
-    }
-
-    if (const ParmVarDecl *Parm = dyn_cast<ParmVarDecl>(D)) {
-      // The Itanium C++ ABI uses the type, scope depth, and scope
-      // index of a parameter when mangling expressions that involve
-      // function parameters, so we will use the parameter's type for
-      // establishing function parameter identity. That way, our
-      // definition of "equivalent" (per C++ [temp.over.link]) is at
-      // least as strong as the definition of "equivalent" used for
-      // name mangling.
-      VisitType(Parm->getType());
-      ID.AddInteger(Parm->getFunctionScopeDepth());
-      ID.AddInteger(Parm->getFunctionScopeIndex());
-      return;
-    }
-
-    if (const TemplateTypeParmDecl *TTP =
-          dyn_cast<TemplateTypeParmDecl>(D)) {
-      ID.AddInteger(TTP->getDepth());
-      ID.AddInteger(TTP->getIndex());
-      ID.AddBoolean(TTP->isParameterPack());
-      return;
-    }
-
-    if (const TemplateTemplateParmDecl *TTP =
-          dyn_cast<TemplateTemplateParmDecl>(D)) {
-      ID.AddInteger(TTP->getDepth());
-      ID.AddInteger(TTP->getIndex());
-      ID.AddBoolean(TTP->isParameterPack());
-      return;
-    }
-  }
-
-  ID.AddPointer(D? D->getCanonicalDecl() : nullptr);
-}
-
-void StmtProfiler::VisitType(QualType T) {
-  if (Canonical)
-    T = Context.getCanonicalType(T);
-
-  ID.AddPointer(T.getAsOpaquePtr());
-}
-
-void StmtProfiler::VisitName(DeclarationName Name) {
-  ID.AddPointer(Name.getAsOpaquePtr());
-}
-
-void StmtProfiler::VisitNestedNameSpecifier(NestedNameSpecifier *NNS) {
-  if (Canonical)
-    NNS = Context.getCanonicalNestedNameSpecifier(NNS);
-  ID.AddPointer(NNS);
-}
-
-void StmtProfiler::VisitTemplateName(TemplateName Name) {
-  if (Canonical)
-    Name = Context.getCanonicalTemplateName(Name);
-
-  Name.Profile(ID);
-}
-
 void StmtProfiler::VisitTemplateArguments(const TemplateArgumentLoc *Args,
                                           unsigned NumArgs) {
   ID.AddInteger(NumArgs);
   for (unsigned I = 0; I != NumArgs; ++I)
     VisitTemplateArgument(Args[I].getArgument());
 }
 
 void StmtProfiler::VisitTemplateArgument(const TemplateArgument &Arg) {
case TemplateArgument::Pack:
     for (const auto &P : Arg.pack_elements())
       VisitTemplateArgument(P);
     break;
   }
 }
 
 void Stmt::Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
                    bool Canonical) const {
-  StmtProfiler Profiler(ID, Context, Canonical);
+  StmtProfilerWithPointers Profiler(ID, Context, Canonical);
+  Profiler.Visit(this);
+}
+
+void Stmt::ODRProfile(llvm::FoldingSetNodeID &ID) const {
+  StmtProfilerWithoutPointers Profiler(ID);
   Profiler.Visit(this);
 }

lib/Sema/SemaDecl.cpp

void Sema::ActOnTagFinishDefinition(Scope *S, Decl *TagD,
 
   // Make sure we "complete" the definition even it is invalid.
   if (Tag->isBeingDefined()) {
     assert(Tag->isInvalidDecl() && "We should already have completed it");
     if (RecordDecl *RD = dyn_cast<RecordDecl>(Tag))
       RD->completeDefinition();
   }
 
-  if (isa<CXXRecordDecl>(Tag))
+  if (auto *RD = dyn_cast<CXXRecordDecl>(Tag)) {
     FieldCollector->FinishClass();
+    if (Context.getLangOpts().Modules)
+      RD->computeODRHash();
+  }
 
   // Exit this scope of this tag's definition.
   PopDeclContext();
 
   if (getCurLexicalContext()->isObjCContainer() &&
       Tag->getDeclContext()->isFileContext())
     Tag->setTopLevelDeclInObjCContainer();
 

lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::ReadCXXDefinitionData(
   Data.HasNonLiteralTypeFieldsOrBases = Record[Idx++];
   Data.ComputedVisibleConversions = Record[Idx++];
   Data.UserProvidedDefaultConstructor = Record[Idx++];
   Data.DeclaredSpecialMembers = Record[Idx++];
   Data.ImplicitCopyConstructorHasConstParam = Record[Idx++];
   Data.ImplicitCopyAssignmentHasConstParam = Record[Idx++];
   Data.HasDeclaredCopyConstructorWithConstParam = Record[Idx++];
   Data.HasDeclaredCopyAssignmentWithConstParam = Record[Idx++];
+  Data.ODRHash = Record[Idx++];
 
   Data.NumBases = Record[Idx++];
   if (Data.NumBases)
     Data.Bases = ReadGlobalOffset(F, Record, Idx);
   Data.NumVBases = Record[Idx++];
   if (Data.NumVBases)
     Data.VBases = ReadGlobalOffset(F, Record, Idx);
   
#define MATCH_FIELD(Field) \
   // ComputedVisibleConversions is handled below.
   MATCH_FIELD(UserProvidedDefaultConstructor)
   OR_FIELD(DeclaredSpecialMembers)
   MATCH_FIELD(ImplicitCopyConstructorHasConstParam)
   MATCH_FIELD(ImplicitCopyAssignmentHasConstParam)
   OR_FIELD(HasDeclaredCopyConstructorWithConstParam)
   OR_FIELD(HasDeclaredCopyAssignmentWithConstParam)
   MATCH_FIELD(IsLambda)
+  MATCH_FIELD(ODRHash)
 #undef OR_FIELD
 #undef MATCH_FIELD
 
   if (DD.NumBases != MergeDD.NumBases || DD.NumVBases != MergeDD.NumVBases)
     DetectedOdrViolation = true;
   // FIXME: Issue a diagnostic if the base classes don't match when we come
   // to lazily load them.
 

lib/Serialization/ASTWriter.cpp

void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
   Record->push_back(Data.HasNonLiteralTypeFieldsOrBases);
   Record->push_back(Data.ComputedVisibleConversions);
   Record->push_back(Data.UserProvidedDefaultConstructor);
   Record->push_back(Data.DeclaredSpecialMembers);
   Record->push_back(Data.ImplicitCopyConstructorHasConstParam);
   Record->push_back(Data.ImplicitCopyAssignmentHasConstParam);
   Record->push_back(Data.HasDeclaredCopyConstructorWithConstParam);
   Record->push_back(Data.HasDeclaredCopyAssignmentWithConstParam);
+  Record->push_back(Data.ODRHash);
   // IsLambda bit is already saved.
 
   Record->push_back(Data.NumBases);
   if (Data.NumBases > 0)
     AddCXXBaseSpecifiers(Data.bases());
 
   // FIXME: Make VBases lazily computed when needed to avoid storing them.
   Record->push_back(Data.NumVBases);

test/Modules/Inputs/odr_hash/first.h

+struct S1 {
+  public:
+};
+
+struct S2Friend2 {};
+struct S2 {
+  friend S2Friend2;
+};
+
+template<class>
+struct S3Template {};
+
+struct S3 {
+  friend S3Template<int>;
+};
+
+struct S4 {
+  static_assert(1 == 1, "First");
+};
+
+struct S5 {
+  static_assert(1 == 1, "Message");
+};
+
+struct S6 {
+  int First();
+};
+
+struct S7 {
+  double foo();
+};
+
+struct S8 {
+  void foo();
+};
+
+struct S9 {
+  void foo() { int y = 5; }
+};
+
+struct S10 {
+  struct {
+    int x;
+  } a;
+};
+
+struct S11 {
+  void foo() { int y = sizeof(int); }
+};
+
+struct S12 {
+  int x = sizeof(x);
+  int y = sizeof(x);
+};
+
+struct S13 {
+  template <typename A> void foo();
+};
+
+struct S14 {
+  template <typename A, typename B> void foo();
+};
+
+template <typename T>
+struct S15 : T {
+  void foo() {
+    int x = __builtin_offsetof(T, first);
+  }
+};
+
+struct S16 {
+  template <template<int = 0> class Y>
+  void foo() {
+    Y<> y;
+  }
+};
+
+struct S17 {
+  template <template <typename> class T>
+  static int foo(int a = 1);
+  template <template <typename> class T, template <typename> class U>
+  using Q_type = T<int>;
+};
+
+struct S18 {
+  enum E { X1 };
+};
+
+struct S19 {
+  enum E { X1 };
+};
+
+struct S20 {
+  enum E { X1 = 1 };
+};
+
+struct S21 {
+  void foo() {
+    label:
+    ;
+  }
+};
+
+struct S22 {
+  void foo() {
+    label_first:
+    ;
+  }
+};
+
+struct S23 {
+  typedef int a;
+  typedef char b;
+};
+
+struct S24 {
+  inline int foo();
+};
+
+struct S25 {
+  int x;
+  S25() : x(5) {}
+};
+
+struct S26 {
+  int x;
+  S26() : x(5) {}
+};
+
+struct S27 {
+  explicit S27(int) {}
+  S27() {}
+};
+
+struct Base1 {
+  Base1();
+  Base1(int);
+  Base1(double);
+};
+
+struct Base2 {
+  Base2();
+  Base2(int);
+  Base2(double);
+};
+
+struct S28 : public Base1 {};
+struct S29 : virtual Base1 {};
+
+struct S30 : public Base1 {
+  S30() : Base1(1) {}
+};
+struct S31 : virtual Base1 {
+  S31() : Base1(1) {}
+};
+struct S32 : public Base1, Base2 {
+  S32() : Base1(1), Base2(1.0) {}
+};
+
+struct S33 {
+  S33() : S33(5) {}
+  S33(int) {int a;}
+};
+
+struct S34 {
+  operator bool();
+};
+
+struct S35 {
+  explicit operator bool();
+};
+
+struct S36 {
+  int x : 3;
+};
+
+struct S37 {
+  mutable int x;
+  int y;
+};
+
+template <class X>
+struct S38 { };
+
+template <class X = int>
+struct S39 { X x; };
+
+template <int X = 5>
+struct S40 { int x = X; };
+
+template <int> class T41a{};
+template <template<int> class T = T41a>
+struct S41 {};
+
+struct S42 {
+  void foo() const {}
+  void bar() {}
+};
+
+struct S43 {
+  static constexpr int x = 1;
+  int y = 1;
+};
+

test/Modules/Inputs/odr_hash/module.map

+module first {
+  header "first.h"
+}
+module second {
+  header "second.h"
+}

test/Modules/Inputs/odr_hash/second.h

+struct S1 {
+  private:
+};
+
+struct S2Friend1 {};
+struct S2 {
+  friend S2Friend1;
+};
+
+template<class>
+struct S3Template {};
+
+struct S3 {
+  friend S3Template<double>;
+};
+
+struct S4 {
+  static_assert(1 == 1, "Second");
+};
+
+struct S5 {
+  static_assert(2 == 2, "Message");
+};
+
+struct S6 {
+  int Second();
+};
+
+struct S7 {
+  int foo();
+};
+
+struct S8 {
+  void foo() {}
+};
+
+struct S9 {
+  void foo() { int x = 5; }
+};
+
+struct S10 {
+  struct {
+    int x;
+    int y;
+  } a;
+};
+
+struct S11 {
+  void foo() { int y = sizeof(double); }
+};
+
+struct S12 {
+  int x = sizeof(x);
+  int y = sizeof(y);
+};
+
+struct S13 {
+  template <typename B> void foo();
+};
+
+struct S14 {
+  template <typename A> void foo();
+};
+
+template <typename T>
+struct S15 : T {
+  void foo() {
+    int x = __builtin_offsetof(T, second);
+  }
+};
+
+struct S16 {
+  template <template<int = 1> class Y>
+  void foo() {
+    Y<> y;
+  }
+};
+
+struct S17 {
+  template <template <typename> class T>
+  static int foo(int a = 1);
+  template <template <typename> class T, template <typename> class U>
+  using Q_type = U<int>;
+};
+
+struct S18 {
+  enum X { X1 };
+};
+
+struct S19 {
+  enum E { X1, X2 };
+};
+
+struct S20 {
+  enum E { X1 = 5};
+};
+
+struct S21 {
+  void foo() {
+    ;
+  }
+};
+
+struct S22 {
+  void foo() {
+    label_second:
+    ;
+  }
+};
+
+struct S23 {
+  typedef char a;
+  typedef int b;
+};
+
+struct S24 {
+  int foo();
+};
+
+struct S25 {
+  int x;
+  S25() {}
+};
+
+struct S26 {
+  int x;
+  S26() : x(2) {}
+};
+
+struct S27 {
+  S27(int) {}
+  S27() {}
+};
+
+struct Base1 {
+  Base1();
+  Base1(int);
+  Base1(double);
+};
+
+struct Base2 {
+  Base2();
+  Base2(int);
+  Base2(double);
+};
+
+struct S28 : public Base2 {};
+struct S29 : virtual Base2 {};
+
+struct S30 : public Base1 {
+  S30() : Base1(1.0) {}
+};
+struct S31 : virtual Base1 {
+  S31() : Base1(1.0) {}
+};
+struct S32 : public Base2, Base1 {
+  S32() : Base2(1), Base1(1.0) {}
+};
+
+struct S33 {
+  S33() : S33(5) {}
+  S33(int) {}
+};
+
+struct S34 {
+  operator int();
+};
+
+struct S35 {
+  operator bool();
+};
+
+struct S36 {
+  int x : 4;
+};
+
+struct S37 {
+  int x;
+  mutable int y;
+};
+
+template <class Y>
+struct S38 { };
+
+template <class X = double>
+struct S39 { X x; };
+
+template <int X = 7>
+struct S40 { int x = X; };
+
+template <int> class T41b{};
+template <template<int> class T = T41b>
+struct S41 {};
+
+struct S42 {
+  void foo() {}
+  void bar() const {}
+};
+
+struct S43 {
+  int x = 1;
+  static constexpr int y = 1;
+};

test/Modules/merge-using-decls.cpp

 template int UseAll<Y>();
 
 // Which of these two sets of diagnostics is chosen is not important. It's OK
 // if this varies with ORDER, but it must be consistent across runs.
 #if ORDER == 1
 // Here, we're instantiating the definition from 'A' and merging the definition
 // from 'B' into it.
 
+// expected-error@b.h:* {{'D::type' from module 'B' is not present in definition of 'D<T>' in module 'A'}}
+// expected-error@b.h:* {{'D::value' from module 'B' is not present in definition of 'D<T>' in module 'A'}}
+
 // expected-error@b.h:* {{'E::value' from module 'B' is not present in definition of 'E<T>' in module 'A'}}
 // expected-error@b.h:* {{'E::v' from module 'B' is not present in definition of 'E<T>' in module 'A'}}
 
 // expected-error@b.h:* {{'F::type' from module 'B' is not present in definition of 'F<T>' in module 'A'}}
 // expected-error@b.h:* {{'F::t' from module 'B' is not present in definition of 'F<T>' in module 'A'}}
 // expected-error@b.h:* {{'F::value' from module 'B' is not present in definition of 'F<T>' in module 'A'}}
 // expected-error@b.h:* {{'F::v' from module 'B' is not present in definition of 'F<T>' in module 'A'}}
 

test/Modules/odr_hash.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x c++ -I%S/Inputs/odr_hash -verify %s -std=c++11
+
+#include "first.h"
+#include "second.h"
+
+namespace test1 {
+S1 s1;
+// expected-error@first.h:* {{'S1' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S3 s3;
+// expected-error@first.h:* {{'S3' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S6 s6;
+// expected-error@second.h:* {{'S6::Second' from module 'second' is not present in definition of 'S6' in module 'first'}}
+// expected-note@first.h:* {{definition has no member 'Second'}}
+
+S7 s7;
+// expected-error@second.h:* {{'S7::foo' from module 'second' is not present in definition of 'S7' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'foo' does not match}}
+
+S8 s8;
+// expected-error@first.h:* {{'S8' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S9 s9;
+// expected-error@first.h:* {{'S9' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S10 s10;
+// expected-error@second.h:* {{'S10::(anonymous struct)::y' from module 'second' is not present in definition of 'S10::(anonymous struct at /usr/local/google/home/rtrieu/clang/miss/llvm/tools/clang/test/Modules/Inputs/odr_hash/first.h:42:3)' in module 'first'}}
+// expected-note@first.h:* {{definition has no member 'y'}}
+
+// expected-error@first.h:* {{'S10' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S11 s11;
+// expected-error@first.h:* {{'S11' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S12 s12;
+
+S13 s13;
+// expected-error@first.h:* {{'S13' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S14 s14;
+// expected-error@second.h:* {{'S14::foo' from module 'second' is not present in definition of 'S14' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'foo' does not match}}
+
+template <typename T>
+void Test15() {
+  S15<T> s15;
+// expected-error@first.h:* {{'S15' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+}
+
+void TestS16() {
+  S16 s16;
+}
+// expected-error@first.h:* {{'S16' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S17 s17;
+// expected-error@second.h:* {{'S17::Q_type' from module 'second' is not present in definition of 'S17' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'Q_type' does not match}}
+
+S18 s18;
+// expected-error@second.h:* {{'S18::X' from module 'second' is not present in definition of 'S18' in module 'first'}}
+// expected-note@first.h:* {{definition has no member 'X'}}
+
+S19 s19;
+// expected-error@first.h:* {{'S19' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S20 s20;
+// expected-error@first.h:* {{'S20' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S21 s21;
+// expected-error@first.h:* {{'S21' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S22 s22;
+// expected-error@first.h:* {{'S22' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S23 s23;
+// expected-error@second.h:* {{'S23::a' from module 'second' is not present in definition of 'S23' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'a' does not match}}
+// expected-error@second.h:* {{'S23::b' from module 'second' is not present in definition of 'S23' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'b' does not match}}
+
+S24 s24;
+// expected-error@first.h:* {{'S24' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S25 s25;
+// expected-error@first.h:* {{'S25' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+S26 s26;
+// expected-error@first.h:* {{'S26' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+S27 s27;
+// expected-error@first.h:* {{'S27' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S28 s28;
+// expected-error@first.h:* {{'S28' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S29 s29;
+// expected-error@first.h:* {{'S29' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S30 s30;
+// expected-error@first.h:* {{'S30' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S31 s31;
+// expected-error@first.h:* {{'S31' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S32 s32;
+// expected-error@first.h:* {{'S32' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S33 s33;
+// expected-error@first.h:* {{'S33' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S34 s34;
+// expected-error@second.h:* {{'S34::operator int' from module 'second' is not present in definition of 'S34' in module 'first'}}
+// expected-note@first.h:* {{definition has no member 'operator int'}}
+
+S35 s35;
+// expected-error@first.h:* {{'S35' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S36 s36;
+// expected-error@first.h:* {{'S36' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S37 s37;
+// expected-error@first.h:* {{'S37' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S38<int> s38;
+// expected-error@first.h:* {{'S38' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S39<> s39;
+// expected-error@first.h:* {{'S39' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S40<> s40;
+// expected-error@first.h:* {{'S40' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+using ::S41;
+// expected-error@first.h:* {{'S41' has different definitions in different modules; definition in module 'first' is here}}
+// expected-note@second.h:* {{definition in module 'second' is here}}
+
+S42 s42;
+// expected-error@second.h:* {{'S42::bar' from module 'second' is not present in definition of 'S42' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'bar' does not match}}
+// expected-error@second.h:* {{'S42::foo' from module 'second' is not present in definition of 'S42' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'foo' does not match}}
+
+S43 s43;
+// expected-error@second.h:* {{'S43::x' from module 'second' is not present in definition of 'S43' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+// expected-error@second.h:* {{'S43::y' from module 'second' is not present in definition of 'S43' in module 'first'}}
+// expected-note@first.h:* {{declaration of 'y' does not match}}
+
+}