[PATCH] D21675: New ODR checker for modules
rtrieu added a comment. This patch will be landing in small chunks to hopefully avoid the large reverts. Repository: rL LLVM https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293585: Add better ODR checking for modules. (authored by
rtrieu).
Changed prior to commit:
https://reviews.llvm.org/D21675?vs=86142=86376#toc
Repository:
rL LLVM
https://reviews.llvm.org/D21675
Files:
cfe/trunk/include/clang/AST/DeclCXX.h
cfe/trunk/include/clang/AST/ODRHash.h
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td
cfe/trunk/lib/AST/CMakeLists.txt
cfe/trunk/lib/AST/DeclCXX.cpp
cfe/trunk/lib/AST/ODRHash.cpp
cfe/trunk/lib/AST/StmtProfile.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Modules/merge-using-decls.cpp
cfe/trunk/test/Modules/odr_hash.cpp
Index: cfe/trunk/lib/AST/DeclCXX.cpp
===
--- cfe/trunk/lib/AST/DeclCXX.cpp
+++ cfe/trunk/lib/AST/DeclCXX.cpp
@@ -18,6 +18,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/ODRHash.h"
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/IdentifierTable.h"
#include "llvm/ADT/STLExtras.h"
@@ -71,8 +72,8 @@
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());
@@ -371,6 +372,16 @@
data().IsParsingBaseSpecifiers = false;
}
+void CXXRecordDecl::computeODRHash() {
+ if (!DefinitionData)
+return;
+
+ ODRHash Hash;
+ Hash.AddCXXRecordDecl(this);
+
+ DefinitionData->ODRHash = Hash.CalculateHash();
+}
+
void CXXRecordDecl::addedClassSubobject(CXXRecordDecl *Subobj) {
// C++11 [class.copy]p11:
// A defaulted copy/move constructor for a class X is defined as
Index: cfe/trunk/lib/AST/ODRHash.cpp
===
--- cfe/trunk/lib/AST/ODRHash.cpp
+++ cfe/trunk/lib/AST/ODRHash.cpp
@@ -0,0 +1,892 @@
+//===-- ODRHash.cpp - Hashing to diagnose ODR failures --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+///
+/// \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/DeclVisitor.h"
+#include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/AST/TypeVisitor.h"
+
+using namespace clang;
+
+// This method adds more information that AddDecl does.
+void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
+ assert(Record && Record->hasDefinition() &&
+ "Expected non-null record to be a definition.");
+ AddDecl(Record);
+
+ // Additional information that is not needed in AddDecl. Do not move this
+ // to ODRTypeVisitor.
+ ID.AddInteger(Record->getNumBases());
+ for (auto base : Record->bases()) {
+AddBoolean(base.isVirtual());
+AddQualType(base.getType());
+ }
+
+ const ClassTemplateDecl *TD = Record->getDescribedClassTemplate();
+ AddBoolean(TD);
+ if (TD) {
+AddTemplateParameterList(TD->getTemplateParameters());
+ }
+}
+
+// Hashing for Stmt is with Stmt::Profile, since they derive from the same base
+// class.
+void ODRHash::AddStmt(const Stmt *S) {
+ assert(S && "Expecting non-null pointer.");
+ S->ProcessODRHash(ID, *this);
+}
+
+void ODRHash::AddIdentifierInfo(const IdentifierInfo *II) {
+ assert(II && "Expecting non-null pointer.");
+ ID.AddString(II->getName());
+}
+
+void ODRHash::AddNestedNameSpecifier(const NestedNameSpecifier *NNS) {
+ assert(NNS && "Expecting non-null pointer.");
+ const auto *Prefix = NNS->getPrefix();
+ AddBoolean(Prefix);
+ if (Prefix) {
+AddNestedNameSpecifier(Prefix);
+ }
+
+ auto Kind = NNS->getKind();
+ ID.AddInteger(Kind);
+ switch (Kind) {
+ case NestedNameSpecifier::Identifier:
+AddIdentifierInfo(NNS->getAsIdentifier());
+break;
+ case NestedNameSpecifier::Namespace:
+AddDecl(NNS->getAsNamespace());
+break;
+ case NestedNameSpecifier::NamespaceAlias:
+AddDecl(NNS->getAsNamespaceAlias());
+break;
+ case
[PATCH] D21675: New ODR checker for modules
rtrieu updated this revision to Diff 86142.
rtrieu added a comment.
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%
https://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclCXX.h
include/clang/AST/ODRHash.h
include/clang/AST/Stmt.h
include/clang/Basic/DiagnosticSerializationKinds.td
lib/AST/CMakeLists.txt
lib/AST/DeclCXX.cpp
lib/AST/ODRHash.cpp
lib/AST/StmtProfile.cpp
lib/Sema/SemaDecl.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Modules/merge-using-decls.cpp
test/Modules/odr_hash.cpp
Index: test/Modules/odr_hash.cpp
===
--- test/Modules/odr_hash.cpp
+++ test/Modules/odr_hash.cpp
@@ -0,0 +1,1077 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module first {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module second {" >> %t/Inputs/module.map
+// RUN: echo "header \"second.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x c++ -I%t/Inputs -verify %s -std=c++11
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {
+ public:
+};
+#elif defined(SECOND)
+struct S1 {
+ private:
+};
+#else
+S1 s1;
+// expected-error@first.h:* {{'S1' has different definitions in different modules; first difference is definition in module 'first' found public access specifier}}
+// expected-note@second.h:* {{but in 'second' found private access specifier}}
+#endif
+
+#if defined(FIRST)
+struct S2Friend2 {};
+struct S2 {
+ friend S2Friend2;
+};
+#elif defined(SECOND)
+struct S2Friend1 {};
+struct S2 {
+ friend S2Friend1;
+};
+#else
+S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'first' found friend 'S2Friend2'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S2Friend1'}}
+#endif
+
+#if defined(FIRST)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#elif defined(SECOND)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#else
+S3 s3;
+// expected-error@first.h:* {{'S3' has different definitions in different modules; first difference is definition in module 'first' found friend 'S3Template'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S3Template'}}
+#endif
+
+#if defined(FIRST)
+struct S4 {
+ static_assert(1 == 1, "First");
+};
+#elif defined(SECOND)
+struct S4 {
+ static_assert(1 == 1, "Second");
+};
+#else
+S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'first' found static assert with message}}
+// expected-note@second.h:* {{but in 'second' found static assert with different message}}
+#endif
+
+#if defined(FIRST)
+struct S5 {
+ static_assert(1 == 1, "Message");
+};
+#elif defined(SECOND)
+struct S5 {
+ static_assert(2 == 2, "Message");
+};
+#else
+S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'first' found static assert with condition}}
+// expected-note@second.h:* {{but in 'second' found static assert with different condition}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+ int First();
+};
+#elif defined(SECOND)
+struct S6 {
+ int Second();
+};
+#else
+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'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+ double foo();
+};
+#elif defined(SECOND)
+struct S7 {
+ int foo();
+};
+#else
+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}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+ void foo();
+};
+#elif defined(SECOND)
+struct S8 {
+ void foo() {}
+};
+#else
[PATCH] D21675: New ODR checker for modules
rsmith added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu added a comment.
In https://reviews.llvm.org/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 `realloc`s 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.
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
teemperor added a comment.
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 `realloc`s 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?
Comment at: lib/AST/ODRHash.cpp:11
+/// \file
+/// This file implements the Instruction class, which calculates a hash based
+/// on AST nodes, which is stable across different runs.
"Instruction class" -> "ODRHash class"
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu added a comment. After changing the ODRHash class a bit, the new performance numbers are 3% in debug and 1-1.5% in release builds. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu marked an inline comment as done. rtrieu added a comment. 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. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, looks good assuming your performance testing doesn't uncover anything. Comment at: lib/AST/ODRHash.cpp:319-321 +if (!D) return; +if (D->isImplicit()) + return; I think you can remove these lines: no-one should be calling this function with a null declaration or an implicit declaration. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu marked 5 inline comments as done. rtrieu added a comment. Comments have been addressed. I will be testing it for performance impact next. Comment at: include/clang/AST/ODRHash.h:54 + // in Pointers. + size_type NextFreeIndex; + rsmith wrote: > Is this always the same as `Pointers.size()`? Yes it is. It has been removed and Pointers.size() is used now. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rsmith added a comment.
Thanks! I assume there's still no measurable performance impact?
Comment at: include/clang/AST/ODRHash.h:54
+ // in Pointers.
+ size_type NextFreeIndex;
+
Is this always the same as `Pointers.size()`?
Comment at: lib/AST/ODRHash.cpp:35
+ for (auto base : Record->bases()) {
+ID.AddInteger(base.isVirtual());
+AddQualType(base.getType());
AddBoolean for clarity maybe?
Comment at: lib/AST/ODRHash.cpp:335-337
+ llvm::SmallVector Decls(DC->decls_begin(),
+DC->decls_end());
+ ID.AddInteger(Decls.size());
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.
Comment at: lib/AST/ODRHash.cpp:493-495
+ID.AddBoolean(hasDefaultArgument);
+if (hasDefaultArgument)
+ AddStmt(D->getDefaultArgument());
Given that `AddStmt` already writes out an "is null" flag, could you use
`AddStmt(hasDefaultArgument ? D->getDefaultArgument() : nullptr)` here?
Comment at: lib/Serialization/ASTReader.cpp:9015-9027
+if (FirstEnum->getName() != SecondEnum->getName()) {
+ Diag(FirstEnum->getLocStart(),
+ diag::err_module_odr_violation_mismatch_decl_diff)
+ << Merge.first << FirstModule.empty()
+ << FirstEnum->getSourceRange() << FirstModule << EnumName
+ << FirstEnum;
+ Diag(SecondEnum->getLocStart(),
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.
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu updated this revision to Diff 82702.
rtrieu added a comment.
Herald added a subscriber: mgorny.
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.
https://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclCXX.h
include/clang/AST/ODRHash.h
include/clang/AST/Stmt.h
include/clang/Basic/DiagnosticSerializationKinds.td
lib/AST/CMakeLists.txt
lib/AST/DeclCXX.cpp
lib/AST/ODRHash.cpp
lib/AST/StmtProfile.cpp
lib/Sema/SemaDecl.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Modules/merge-using-decls.cpp
test/Modules/odr_hash.cpp
Index: test/Modules/odr_hash.cpp
===
--- test/Modules/odr_hash.cpp
+++ test/Modules/odr_hash.cpp
@@ -0,0 +1,1077 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module first {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module second {" >> %t/Inputs/module.map
+// RUN: echo "header \"second.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x c++ -I%t/Inputs -verify %s -std=c++11
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {
+ public:
+};
+#elif defined(SECOND)
+struct S1 {
+ private:
+};
+#else
+S1 s1;
+// expected-error@first.h:* {{'S1' has different definitions in different modules; first difference is definition in module 'first' found public access specifier}}
+// expected-note@second.h:* {{but in 'second' found private access specifier}}
+#endif
+
+#if defined(FIRST)
+struct S2Friend2 {};
+struct S2 {
+ friend S2Friend2;
+};
+#elif defined(SECOND)
+struct S2Friend1 {};
+struct S2 {
+ friend S2Friend1;
+};
+#else
+S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'first' found friend 'S2Friend2'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S2Friend1'}}
+#endif
+
+#if defined(FIRST)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#elif defined(SECOND)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#else
+S3 s3;
+// expected-error@first.h:* {{'S3' has different definitions in different modules; first difference is definition in module 'first' found friend 'S3Template'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S3Template'}}
+#endif
+
+#if defined(FIRST)
+struct S4 {
+ static_assert(1 == 1, "First");
+};
+#elif defined(SECOND)
+struct S4 {
+ static_assert(1 == 1, "Second");
+};
+#else
+S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'first' found static assert with message}}
+// expected-note@second.h:* {{but in 'second' found static assert with different message}}
+#endif
+
+#if defined(FIRST)
+struct S5 {
+ static_assert(1 == 1, "Message");
+};
+#elif defined(SECOND)
+struct S5 {
+ static_assert(2 == 2, "Message");
+};
+#else
+S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'first' found static assert with condition}}
+// expected-note@second.h:* {{but in 'second' found static assert with different condition}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+ int First();
+};
+#elif
[PATCH] D21675: New ODR checker for modules
rtrieu added a comment.
In https://reviews.llvm.org/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.
Comment at: lib/AST/DeclBase.cpp:1827
+ void VisitParmVarDecl(const ParmVarDecl *D) {
+VisitStmt(D->getDefaultArg());
+Inherited::VisitParmVarDecl(D);
rsmith wrote:
> 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
> ```
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.
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu updated this revision to Diff 75970.
rtrieu marked 2 inline comments as done.
https://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclBase.h
include/clang/AST/DeclCXX.h
include/clang/AST/Stmt.h
include/clang/AST/TemplateBase.h
include/clang/AST/Type.h
include/clang/Basic/DiagnosticSerializationKinds.td
lib/AST/DeclBase.cpp
lib/AST/DeclCXX.cpp
lib/AST/StmtProfile.cpp
lib/AST/TemplateBase.cpp
lib/AST/Type.cpp
lib/Sema/SemaDecl.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Modules/merge-using-decls.cpp
test/Modules/odr_hash.cpp
Index: test/Modules/odr_hash.cpp
===
--- test/Modules/odr_hash.cpp
+++ test/Modules/odr_hash.cpp
@@ -0,0 +1,752 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module first {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module second {" >> %t/Inputs/module.map
+// RUN: echo "header \"second.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x c++ -I%t/Inputs -verify %s -std=c++11 -fcolor-diagnostics
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {
+ public:
+};
+#elif defined(SECOND)
+struct S1 {
+ private:
+};
+#else
+S1 s1;
+// expected-error@first.h:* {{'S1' has different definitions in different modules; first difference is definition in module 'first' found public access specifier}}
+// expected-note@second.h:* {{but in 'second' found private access specifier}}
+#endif
+
+#if defined(FIRST)
+struct S2Friend2 {};
+struct S2 {
+ friend S2Friend2;
+};
+#elif defined(SECOND)
+struct S2Friend1 {};
+struct S2 {
+ friend S2Friend1;
+};
+#else
+S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'first' found friend 'S2Friend2'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S2Friend1'}}
+#endif
+
+#if defined(FIRST)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#elif defined(SECOND)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#else
+S3 s3;
+// expected-error@first.h:* {{'S3' has different definitions in different modules; first difference is definition in module 'first' found friend 'S3Template'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S3Template'}}
+#endif
+
+#if defined(FIRST)
+struct S4 {
+ static_assert(1 == 1, "First");
+};
+#elif defined(SECOND)
+struct S4 {
+ static_assert(1 == 1, "Second");
+};
+#else
+S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'first' found static assert with message}}
+// expected-note@second.h:* {{but in 'second' found static assert with different message}}
+#endif
+
+#if defined(FIRST)
+struct S5 {
+ static_assert(1 == 1, "Message");
+};
+#elif defined(SECOND)
+struct S5 {
+ static_assert(2 == 2, "Message");
+};
+#else
+S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'first' found static assert with condition}}
+// expected-note@second.h:* {{but in 'second' found static assert with different condition}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+ int First();
+};
+#elif defined(SECOND)
+struct S6 {
+ int Second();
+};
+#else
+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'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+ double foo();
+};
+#elif defined(SECOND)
+struct S7 {
+ int foo();
+};
+#else
+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}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+ void foo();
+};
+#elif defined(SECOND)
+struct S8 {
+ void foo() {}
+};
+#else
+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}}
+#endif
+
+#if defined(FIRST)
[PATCH] D21675: New ODR checker for modules
rsmith added a comment.
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.
> DeclBase.cpp:1827
> + void VisitParmVarDecl(const ParmVarDecl *D) {
> +VisitStmt(D->getDefaultArg());
> +Inherited::VisitParmVarDecl(D);
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
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu marked 2 inline comments as done.
rtrieu added inline comments.
> dblaikie wrote in DeclBase.cpp:1810-1812
> 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)
These should be more consistent now.
> dblaikie wrote in first.h:5-8
> 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.
Modified so entire test is now in one file. The separate headers and module
maps are generated into the temp folder for each run.
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rtrieu updated this revision to Diff 73388.
rtrieu added a comment.
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.
https://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclBase.h
include/clang/AST/DeclCXX.h
include/clang/AST/Stmt.h
include/clang/AST/TemplateBase.h
include/clang/AST/Type.h
include/clang/Basic/DiagnosticSerializationKinds.td
lib/AST/DeclBase.cpp
lib/AST/DeclCXX.cpp
lib/AST/StmtProfile.cpp
lib/AST/TemplateBase.cpp
lib/AST/Type.cpp
lib/Sema/SemaDecl.cpp
lib/Serialization/ASTReader.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Modules/merge-using-decls.cpp
test/Modules/odr_hash.cpp
Index: test/Modules/odr_hash.cpp
===
--- test/Modules/odr_hash.cpp
+++ test/Modules/odr_hash.cpp
@@ -0,0 +1,731 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module first {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module second {" >> %t/Inputs/module.map
+// RUN: echo "header \"second.h\"" >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x c++ -I%t/Inputs -verify %s -std=c++11 -fcolor-diagnostics
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {
+ public:
+};
+#elif defined(SECOND)
+struct S1 {
+ private:
+};
+#else
+S1 s1;
+// expected-error@first.h:* {{'S1' has different definitions in different modules; first difference is definition in module 'first' found public access specifier}}
+// expected-note@second.h:* {{but in 'second' found private access specifier}}
+#endif
+
+#if defined(FIRST)
+struct S2Friend2 {};
+struct S2 {
+ friend S2Friend2;
+};
+#elif defined(SECOND)
+struct S2Friend1 {};
+struct S2 {
+ friend S2Friend1;
+};
+#else
+S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'first' found friend 'S2Friend2'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S2Friend1'}}
+#endif
+
+#if defined(FIRST)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#elif defined(SECOND)
+template
+struct S3Template {};
+struct S3 {
+ friend S3Template;
+};
+#else
+S3 s3;
+// expected-error@first.h:* {{'S3' has different definitions in different modules; first difference is definition in module 'first' found friend 'S3Template'}}
+// expected-note@second.h:* {{but in 'second' found other friend 'S3Template'}}
+#endif
+
+#if defined(FIRST)
+struct S4 {
+ static_assert(1 == 1, "First");
+};
+#elif defined(SECOND)
+struct S4 {
+ static_assert(1 == 1, "Second");
+};
+#else
+S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'first' found static assert with message}}
+// expected-note@second.h:* {{but in 'second' found static assert with different message}}
+#endif
+
+#if defined(FIRST)
+struct S5 {
+ static_assert(1 == 1, "Message");
+};
+#elif defined(SECOND)
+struct S5 {
+ static_assert(2 == 2, "Message");
+};
+#else
+S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'first' found static assert with condition}}
+// expected-note@second.h:* {{but in 'second' found static assert with different condition}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+ int First();
+};
+#elif defined(SECOND)
+struct S6 {
+ int Second();
+};
+#else
+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'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+ double foo();
+};
+#elif defined(SECOND)
+struct S7 {
+ int foo();
+};
+#else
+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}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+ void foo();
+};
+#elif defined(SECOND)
+struct S8 {
+ void foo() {}
+};
+#else
+S8 s8;
+// expected-error@first.h:* {{'S8' has
Re: [PATCH] D21675: New ODR checker for modules
dblaikie added inline comments.
Comment at: lib/AST/DeclBase.cpp:1810-1812
@@ +1809,5 @@
+ void VisitNamedDecl(const NamedDecl *D) {
+if (IdentifierInfo *II = D->getIdentifier()) {
+ ID.AddString(II->getName());
+}
+Inherited::VisitNamedDecl(D);
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)
Comment at: test/Modules/Inputs/odr_hash/first.h:5-8
@@ +4,6 @@
+
+struct S2Friend2 {};
+struct S2 {
+ friend S2Friend2;
+};
+
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.
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21675: New ODR checker for modules
rtrieu updated this revision to Diff 68278.
rtrieu added a comment.
Add function void ODRHash(llvm::FoldingSetNodeID ) to several classes for
computing the hash. Decl, Stmt, TemplateArgument, Type and QualType now have
this function, and can call among each others' functions.
https://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclBase.h
include/clang/AST/DeclCXX.h
include/clang/AST/Stmt.h
include/clang/AST/TemplateBase.h
include/clang/AST/Type.h
lib/AST/DeclBase.cpp
lib/AST/DeclCXX.cpp
lib/AST/StmtProfile.cpp
lib/AST/TemplateBase.cpp
lib/AST/Type.cpp
lib/Sema/SemaDecl.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Modules/Inputs/odr_hash/first.h
test/Modules/Inputs/odr_hash/module.map
test/Modules/Inputs/odr_hash/second.h
test/Modules/merge-using-decls.cpp
test/Modules/odr_hash.cpp
Index: test/Modules/odr_hash.cpp
===
--- test/Modules/odr_hash.cpp
+++ test/Modules/odr_hash.cpp
@@ -0,0 +1,191 @@
+// 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
+void Test15() {
+ S15 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'
[PATCH] D21675: New ODR checker for modules
rtrieu created this revision.
rtrieu added a subscriber: cfe-commits.
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.
http://reviews.llvm.org/D21675
Files:
include/clang/AST/DeclCXX.h
include/clang/AST/Stmt.h
lib/AST/DeclCXX.cpp
lib/AST/StmtProfile.cpp
lib/Sema/SemaDecl.cpp
lib/Serialization/ASTReaderDecl.cpp
lib/Serialization/ASTWriter.cpp
test/Modules/Inputs/odr_hash/first.h
test/Modules/Inputs/odr_hash/module.map
test/Modules/Inputs/odr_hash/second.h
test/Modules/merge-using-decls.cpp
test/Modules/odr_hash.cpp
Index: test/Modules/odr_hash.cpp
===
--- test/Modules/odr_hash.cpp
+++ test/Modules/odr_hash.cpp
@@ -0,0 +1,191 @@
+// 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'}}
+//
