[ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[vsapsai]

Summary

When two modules contain struct/union with the same name, check the
definitions are equivalent and diagnose if they are not. This is similar
to CXXRecordDecl where we already discover and diagnose mismatches.

rdar://problem/56764293

[martong]

clang/lib/Serialization/ASTReader.cpp

9524

Would it be possible to check the structural non-equivalency with ODRHash?
I wonder if the ODRHash of the two Decls are different in this case or not.
ASTStructuralEquivalency and ODRHash seems to serve a very similar use case, however, AFAIK in the rest of the ASTReader code ODRHash is used exclusively.

(On a long term, I am thinking about that perhaps ASTImporter could use ODRHash instead of ASTStructuralEquivalency.)

[bruno]

Change the approach: handle type merging for tag types using the ODRHash mechanism

[bruno]

clang/lib/Serialization/ASTReader.cpp

9524

Great idea. Updated the patch with another approach!

[bruno]

Remove some FIXMEs that are now done.

[rtrieu]

clang/lib/Serialization/ASTReader.cpp

9699

Is this just a copy of the other loop? That's a lot of code duplication.

[bruno]

clang/lib/Serialization/ASTReader.cpp

9699

There's a lot of copy but a lot of CXXRecord specific logic got trimmed out and there are small differences in the approach, so a direct refactor of it won't work, but there's certainly more small pieces that can be factored out, will follow up with that.

[bruno]

Updated the patch to refactor mode code.

• Move off everything that can be reused for RecordDecl.
• Early exist when not C++.

Once the patch gets approved the idea is to upstream some of the refactoring
first so its easier to spot the actual non NFC part.

[rtrieu]

clang/lib/Serialization/ASTReader.cpp

9699

I like this refactoring much better. Probably more refactoring could be done to this function since it's so long, but that's an issue for another time.

[vsapsai]

Think I'll need to make another review pass but here are my comments so far:

• Why are you adding ODR hash support for RecordDecl and not TagDecl? We already have support for EnumDecl, so TagDecl seems like a good candidate to cover both. Honestly, I don't know if it is possible or a good idea but it looks plausible enough to consider.
• Are anonymous structs working? Worth adding test cases.
• Are unions working? Didn't notice any code specifically for them but RecordDecl covers both structs and unions, so they should be working and we need to test that.
• Few testing additions. These cases might be already covered or might be low value, so take these suggestions with a grain of salt:
  • test self-referential structs like struct Node { struct Node *next; };
  • test comparing structs and forward declarations, e.g., struct S; and struct S { ... };, and another couple struct S { ... }; and struct S; struct S { ... }; The motivation is to make sure we aren't stumped when we cannot find struct definition or when the definition is in unexpected place.

clang/lib/AST/Decl.cpp

4896

To be consistent with the existing code, it is better to call it as setHasODRHash(true)

[rtrieu]

Heads up in case it affects you refactoring work:
While looking through this code, I found a bug I previously made. I fixed it with a small change, but that lies in the middle of your refactoring during FieldDecl handling. The change is here:

https://reviews.llvm.org/rGa60e8927297005898b10a46300d929ba5cf7833c

[bruno]

• Why are you adding ODR hash support for RecordDecl and not TagDecl? We already have support for EnumDecl, so TagDecl seems like a good candidate to cover both. Honestly, I don't know if it is possible or a good idea but it looks plausible enough to consider.

The reason is that in C++ ODR diagnostics for TagDecl are handled as part of CXXRecordDecl, I didn't want add a path in TagDecl for C/ObjC only.

• Are anonymous structs working? Worth adding test cases.

They are at the top level but not nested ones, I fixed that and should upload a new patch with that next.

• Are unions working? Didn't notice any code specifically for them but RecordDecl covers both structs and unions, so they should be working and we need to test that.

Done in upcoming patch.

• Few testing additions. These cases might be already covered or might be low value, so take these suggestions with a grain of salt:
  • test self-referential structs like struct Node { struct Node *next; };
  • test comparing structs and forward declarations, e.g., struct S; and struct S { ... };, and another couple struct S { ... }; and struct S; struct S { ... }; The motivation is to make sure we aren't stumped when we cannot find struct definition or when the definition is in unexpected place.

Ditto.

Heads up in case it affects you refactoring work:
While looking through this code, I found a bug I previously made. I fixed it with a small change, but that lies in the middle of your refactoring during FieldDecl handling. The change is here:

https://reviews.llvm.org/rGa60e8927297005898b10a46300d929ba5cf7833c

Thanks for the heads up @rtrieu, I end up including that as part of rG90f58eaeff5f

[bruno]

• Update patch after Volodymyr's review.
• Refactor bits done as part of rG90f58eaeff5f, update it to remove that part.

[rsmith]

clang/include/clang/AST/Decl.h

4268–4269

We shouldn't store this here; this will make all CXXRecordDecls larger to store a field they will never look at.

We have 27 spare trailing bits in RecordDeclBitfields (28 if you don't add the HasODRHash bit and use 0 to mean "hash not computed"). Can we store this there instead of here?

clang/lib/AST/ODRHash.cpp

559–560

This is, at least in principle, not reliable. After definition merging, we could have picked a different definition of this record type as "the" definition. (It's probably not straightforward to get this to happen in practice, and maybe not even possible, but I'm not certain of that.)

Maybe we could add to TagDecl something like

bool isThisDeclarationADemotedDefinition() const {
  return !isThisDeclarationADefinition() && BraceRange.isValid();
}

and then check isThisDeclarationADefinition() || isThisDeclarationADemotedDefinition() here? (I'm not sure we always update BraceRange properly for demoted definitions either, so maybe that's not quite right.)

clang/lib/Serialization/ASTReader.cpp

9666–9668

I don't understand why this assertion would be correct if the above branch can ever be taken. It's possible for two different types to have the same hash, and it looks like we'll assert here if that happens. Should we be branching on hasSameType above instead of comparing hashes?

[bruno]

Thanks for taking a look Richard, comments inline.

clang/include/clang/AST/Decl.h

4268–4269

Makes sense!

clang/lib/AST/ODRHash.cpp

559–560

There are two things I realized while looking at this:

• What I really intended here was to hash underlying anonymous structs/unions, not just any underlying RecordDecl. Will change the logic in ODRHash::AddRecordDecl do reflect that properly.
• This patch was skipping RecordDecls without definitions during ODR diagnostics, I've changed to instead decide at merge time if we want to register those Decls for later diagnose, and the new rule is clang skips RecordDecls that disagree on whether they have a definition. This should still allow clang to diagnose declarations without defintions that have different attributes (which I intend to add for specific attributes in future patches).

Will update the patch to reflect that. WDYT?

clang/lib/Serialization/ASTReader.cpp

9666–9668

This is trying to cover two things. The first one is to handle nested anonymous unions, which might have the same type, but underlying mismatching fields:

#if defined(FIRST)
struct AU {
  union {
    int a;
  };
};
#elif defined(SECOND)
struct AU {
  union {
    char a;
  };
};
#else
struct AU au;

The second is to allow for @interfaces (downstream patches at the moment) to reuse logic to diagnose fields in ODRDiagField without having to add its own check for the underlying types. Does that makes sense?

[bruno]

Address Richard's review.

[bruno]

Ping!

[rsmith]

clang/lib/AST/DeclCXX.cpp

487 ↗

(On Diff #244435)

I think this change is no longer necessary.

clang/lib/Serialization/ASTReader.cpp

9666–9668

I still don't understand.

Either the types are always the same before the previous if and can only differ in type sugar (in which case this change is unnecessary), or the types can be different in more than just sugar (in which case this assert is wrong because there's no guarantee that different types will have different hashes).

What am I missing?

clang/lib/Serialization/ASTWriterDecl.cpp

496

It would be better to avoid writing this at all for a CXXRecordDecl, to avoid adding 6 unused bits per class to the bitcode. (Please also look at isa<CXXRecordDecl>(D) not at the LangOpts here.)

[bruno]

clang/lib/Serialization/ASTReader.cpp

9666–9668

@vsapsai: just to follow up here a bit. I don't remember the full context anymore, but it should be fine to reintroduce this in a later patch with a better explanation to @rsmith, or change the approach if it makes sense. Thanks for taking this over!

[vsapsai]

Rebase on top of "main" and address review comments.

[vsapsai]

clang/lib/AST/DeclCXX.cpp

487 ↗

(On Diff #244435)

Reverted the change.

clang/lib/Serialization/ASTReader.cpp

9666–9668

I think it makes the most sense to remove the assertion. Based on the local code, it doesn't make much sense, as equality of field names doesn't imply the equality of field types. On the higher level, we were checking field types in a different place, specifically we populated PendingOdrMergeChecks in ASTDeclReader::findExisting and diagnosed it earlier in this mega-method ASTReader::diagnoseOdrViolations (error constant err_module_odr_violation_missing_decl). Also you can notice that the diagnostic for

struct S { int x; };

struct S { float x; };

is different, it is

'S::x' from module 'SecondModule' is not present in definition of 'S' in module 'FirstModule'

compared to

'S' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'int'

that we emit in ODRDiagField.

It is possible to try to make unions behave the same way as structs (and keep the assertion) but I'm not sure it is worth it. Looks like the major difference is that not all tag types are true Redeclerable and I don't know if it is something we should change.

clang/lib/Serialization/ASTWriterDecl.cpp

496

Changed to write ODR hash for everything except CXXRecordDecl (right now everything is RecordDecl). Also updated ASTReaderDecl.cpp. Not 100% sure I understand your comment correctly, so if my change is way off, please let me know.

[vsapsai]

Ping.

[vsapsai]

Ping.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

834

During investigation of a different issue noticed there is no error for the following test case

#if defined(FIRST)
typedef struct FW FW;
struct FW {
  int x;
};
#elif defined(SECOND)
struct FW {
  float a;
};
#else
struct FW fw;
#endif

And that is because Canon is a forward declaration from the typedef and we don't compare non-definition with definition.

[vsapsai]

Found another case that doesn't emit an error

#if defined(FIRST)
struct Indirect {
  int x;
};
struct Direct {
  struct Indirect i;
};
#elif defined(SECOND)
struct Indirect {
  double a;
};
struct Direct {
  struct Indirect i;
};
#else
struct Direct d;
#endif

According to my debugging there is no error because Direct fields aren't deserialized in -fsyntax-only mode and therefore Indirect definitions aren't compared. But during IRGen there is diagnostic and that's because calculating record layout triggers full deserialization. Also there is diagnostic in C++ because we are dealing with default initialization and DeclareImplicitDefaultConstructor iterates through all the fields deserializing them.

I believe the best user experience is consistent diagnostic, so we should emit the error even with -fsyntax-only. If anybody has any objections, please let me know, it would save time.

[vsapsai]

Need to detect mismatches in nested structs.

[vsapsai]

Rebase and use ODRDiagsEmitter. Add more tests.

[vsapsai]

If anybody is curious about anonymous structs/unions, the relevant change is D140055.

[vsapsai]

Ping. Also checked the change on a bunch of internal Objective-C/C code using modules - no new errors.

[ChuanqiXu]

clang/lib/AST/Decl.cpp

4897–4898

I am curious for the operation. It looks dangerous. How can we be sure that the hash value is still meaningful after remove its lower 6 bits?

clang/lib/AST/ODRDiagsEmitter.cpp

1550–1551

The implementation of this function looks redundant with other overloads. Of course this is not the problem of the patch. I think we can add a FIXME at least.

clang/lib/AST/ODRHash.cpp

598–599

For the current implementation, if it makes sense to add an assertion !isa<CXXRecordDecl>(Record);

[vsapsai]

clang/lib/AST/Decl.cpp

4897–4898

Hash value itself has no meaning. The risk with truncation is that RecordDecls with different hashes can end up with equal truncated hashes. It means we'd miss some mismatches. Currently we are missing all mismatches, so some looks like an improvement.

This applies only to C and Objective-C but not to C++ as CXXRecordDecl stores its own ODR hash separately. So some imperfection in Objective-C seems more desirable than adding 4 bytes in memory consumption per each struct and class, including C++ classes.

clang/lib/AST/ODRDiagsEmitter.cpp

1550–1551

Do you have any specific ideas to reduce redundancy? PopulateHashes probably can be made the same for different entities but the rest has many annoying differences. Diagnosing mismatches for different entities consists of the same pieces (that are already put into reusable methods) but the composition of such pieces is different for different entities.

I've tried to push complex logic into reusable methods and repeat the simple stuff. For example, for std::string FirstModule = getOwningModuleNameForDiagnostic(FirstRecord); obtaining a module name is non-trivial and that's why it is in a reusable method. But the same assignment doesn't seem to carry the problems of copy-pasted code.

By the way, Phabricator shows big [visually] contiguous chunks as being copied. But in fact these big chunks consist of smaller pieces that are taken from different places. So I think that reusing the same pieces multiple times and composing them in different ways is actually useful.

clang/lib/AST/ODRHash.cpp

598–599

Yes, that's a good idea as calling AddRecordDecl with CXXRecordDecl looks like an easy mistake to make.

[vsapsai]

Rebase and add assertion in ODRHash::AddRecordDecl.

[ChuanqiXu]

LGTM generally. It'd better to mention this in the Potentially Breaking Changes section of ReleaseNotes.

clang/lib/AST/Decl.cpp

4714

nit: setODRHash(0) may be more consistent with above style.

4897–4898

It sounds not bad.

clang/lib/AST/ODRDiagsEmitter.cpp

1550–1551

No specific ideas really. I feel it is scary to see so many copied chunks and we should be able to extract them into smaller common functions. But this is not the problem of the patch. I don't want to block it for such reasons.

[vsapsai]

setODRHash(0) for consistency, mention the change in Potentially Breaking Changes.

[vsapsai]

In D71734#4061228, @ChuanqiXu wrote:

LGTM generally. It'd better to mention this in the Potentially Breaking Changes section of ReleaseNotes.

Thanks for all your efforts during the review! Mentioned the change in the release notes.

clang/lib/AST/Decl.cpp

4714

You are right, changed it.

clang/docs/ReleaseNotes.rst

 - ``-p`` is rejected for all targets which are not AIX or OpenBSD. ``-p`` led
   to an ``-Wunused-command-line-argument`` warning in previous releases.
 
 - Clang now diagnoses non-inline externally-visible definitions in C++
   standard header units as per ``[module.import/6]``.  Previously, in Clang-15,
   these definitions were allowed.  Note that such definitions are ODR
   violations if the header is included more than once.
 
+- Clang now diagnoses if structs/unions with the same name are different in
+  different used modules. Behavior in C and Objective-C language modes now is
+  the same as in C++.
+
 What's New in Clang |release|?
 ==============================
 Some of the major new features and improvements to Clang are listed
 here. Generic improvements to Clang as a whole or to its underlying
 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 Major New Features

clang/include/clang/AST/Decl.h

 ///   struct X;                  // Forward declaration, no "body".
 ///   union Y { int A, B; };     // Has body with members A and B (FieldDecls).
 /// This decl will be marked invalid if *any* members are invalid.
 class RecordDecl : public TagDecl {
   // This class stores some data in DeclContext::RecordDeclBits
   // to save some space. Use the provided accessors to access it.
 public:
   friend class DeclContext;
+  friend class ASTDeclReader;
   /// Enum that represents the different ways arguments are passed to and
   /// returned from function calls. This takes into account the target-specific
   /// and version-specific rules along with the rules determined by the
   /// language.
   enum ArgPassingKind : unsigned {
     /// The argument of this type can be passed directly in registers.
     APK_CanPassInRegs,
 
public:
   /// These padding are added to help AddressSanitizer detect
   /// intra-object-overflow bugs.
   bool mayInsertExtraPadding(bool EmitRemark = false) const;
 
   /// Finds the first data member which has a name.
   /// nullptr is returned if no named data member exists.
   const FieldDecl *findFirstNamedDataMember() const;
 
+  /// Get precomputed ODRHash or add a new one.
+  unsigned getODRHash();
+
 private:
   /// Deserialize just the fields.
   void LoadFieldsFromExternalStorage() const;
+
+  /// True if a valid hash is stored in ODRHash.
+  bool hasODRHash() const { return RecordDeclBits.ODRHash; }
+  void setODRHash(unsigned Hash) { RecordDeclBits.ODRHash = Hash; }
 };
 
 class FileScopeAsmDecl : public Decl {
   StringLiteral *AsmString;
   SourceLocation RParenLoc;
 
   FileScopeAsmDecl(DeclContext *DC, StringLiteral *asmstring,
                    SourceLocation StartL, SourceLocation EndL)
     : Decl(FileScopeAsm, DC, StartL), AsmString(asmstring), RParenLoc(EndL) {}
 
   virtual void anchor();

clang/include/clang/AST/DeclBase.h

class RecordDeclBitfields {
     /// Indicates whether this struct is destroyed in the callee.
     uint64_t ParamDestroyedInCallee : 1;
 
     /// Represents the way this type is passed to a function.
     uint64_t ArgPassingRestrictions : 2;
 
     /// Indicates whether this struct has had its field layout randomized.
     uint64_t IsRandomized : 1;
+
+    /// True if a valid hash is stored in ODRHash. This should shave off some
+    /// extra storage and prevent CXXRecordDecl to store unused bits.
+    uint64_t ODRHash : 26;
   };
 
   /// Number of non-inherited bits in RecordDeclBitfields.
-  enum { NumRecordDeclBits = 15 };
+  enum { NumRecordDeclBits = 41 };
 
   /// Stores the bits used by OMPDeclareReductionDecl.
   /// If modified NumOMPDeclareReductionDeclBits and the accessor
   /// methods in OMPDeclareReductionDecl should be updated appropriately.
   class OMPDeclareReductionDeclBitfields {
     friend class OMPDeclareReductionDecl;
     /// For the bits in DeclContextBitfields
     uint64_t : NumDeclContextBits;

clang/include/clang/AST/ODRDiagsEmitter.h

public:
   /// Returns true if found a mismatch and diagnosed it.
   /// To compare 2 declarations with merged and identical definition data
   /// you need to provide pre-merge definition data in \p SecondDD.
   bool
   diagnoseMismatch(const CXXRecordDecl *FirstRecord,
                    const CXXRecordDecl *SecondRecord,
                    const struct CXXRecordDecl::DefinitionData *SecondDD) const;
 
+  /// Diagnose ODR mismatch between 2 RecordDecl that are not CXXRecordDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const RecordDecl *FirstRecord,
+                        const RecordDecl *SecondRecord) const;
+
   /// Diagnose ODR mismatch between 2 ObjCProtocolDecl.
   ///
   /// Returns true if found a mismatch and diagnosed it.
   /// To compare 2 declarations with merged and identical definition data
   /// you need to provide pre-merge definition data in \p SecondDD.
   bool diagnoseMismatch(
       const ObjCProtocolDecl *FirstProtocol,
       const ObjCProtocolDecl *SecondProtocol,

clang/include/clang/AST/ODRHash.h

 
 public:
   ODRHash() {}
 
   // Use this for ODR checking classes between modules.  This method compares
   // more information than the AddDecl class.
   void AddCXXRecordDecl(const CXXRecordDecl *Record);
 
+  // Use this for ODR checking records in C/Objective-C between modules. This
+  // method compares more information than the AddDecl class.
+  void AddRecordDecl(const RecordDecl *Record);
+
   // Use this for ODR checking functions between modules.  This method compares
   // more information than the AddDecl class.  SkipBody will process the
   // hash as if the function has no body.
   void AddFunctionDecl(const FunctionDecl *Function, bool SkipBody = false);
 
   // Use this for ODR checking enums between modules.  This method compares
   // more information than the AddDecl class.
   void AddEnumDecl(const EnumDecl *Enum);

clang/include/clang/Serialization/ASTReader.h

using DataPointers =
       std::pair<CXXRecordDecl *, struct CXXRecordDecl::DefinitionData *>;
   using ObjCProtocolDataPointers =
       std::pair<ObjCProtocolDecl *, struct ObjCProtocolDecl::DefinitionData *>;
 
   /// Record definitions in which we found an ODR violation.
   llvm::SmallDenseMap<CXXRecordDecl *, llvm::SmallVector<DataPointers, 2>, 2>
       PendingOdrMergeFailures;
 
+  /// C/ObjC record definitions in which we found an ODR violation.
+  llvm::SmallDenseMap<RecordDecl *, llvm::SmallVector<RecordDecl *, 2>, 2>
+      PendingRecordOdrMergeFailures;
+
   /// Function definitions in which we found an ODR violation.
   llvm::SmallDenseMap<FunctionDecl *, llvm::SmallVector<FunctionDecl *, 2>, 2>
       PendingFunctionOdrMergeFailures;
 
   /// Enum definitions in which we found an ODR violation.
   llvm::SmallDenseMap<EnumDecl *, llvm::SmallVector<EnumDecl *, 2>, 2>
       PendingEnumOdrMergeFailures;
 

clang/lib/AST/Decl.cpp

RecordDecl::RecordDecl(Kind DK, TagKind TK, const ASTContext &C,
   setNonTrivialToPrimitiveCopy(false);
   setNonTrivialToPrimitiveDestroy(false);
   setHasNonTrivialToPrimitiveDefaultInitializeCUnion(false);
   setHasNonTrivialToPrimitiveDestructCUnion(false);
   setHasNonTrivialToPrimitiveCopyCUnion(false);
   setParamDestroyedInCallee(false);
   setArgPassingRestrictions(APK_CanPassInRegs);
   setIsRandomized(false);
+  setODRHash(0);
 }
 
 RecordDecl *RecordDecl::Create(const ASTContext &C, TagKind TK, DeclContext *DC,
                                SourceLocation StartLoc, SourceLocation IdLoc,
                                IdentifierInfo *Id, RecordDecl* PrevDecl) {
   RecordDecl *R = new (C, DC) RecordDecl(Record, TK, C, DC,
                                          StartLoc, IdLoc, Id, PrevDecl);
   R->setMayHaveOutOfDateDef(C.getLangOpts().Modules);
if (const auto *RT = I->getType()->getAs<RecordType>())
               RT->getDecl()->findFirstNamedDataMember())
         return NamedDataMember;
   }
 
   // We didn't find a named data member.
   return nullptr;
 }
 
+unsigned RecordDecl::getODRHash() {
+  if (hasODRHash())
+    return RecordDeclBits.ODRHash;
+
+  // Only calculate hash on first call of getODRHash per record.
+  ODRHash Hash;
+  Hash.AddRecordDecl(this);
+  // For RecordDecl the ODRHash is stored in the remaining 26
+  // bit of RecordDeclBits, adjust the hash to accomodate.
+  setODRHash(Hash.CalculateHash() >> 6);
+  return RecordDeclBits.ODRHash;
+}
+
 //===----------------------------------------------------------------------===//
 // BlockDecl Implementation
 //===----------------------------------------------------------------------===//
 
 BlockDecl::BlockDecl(DeclContext *DC, SourceLocation CaretLoc)
     : Decl(Block, DC, CaretLoc), DeclContext(Block) {
   setIsVariadic(false);
   setCapturesCXXThis(false);

clang/lib/AST/ODRDiagsEmitter.cpp

Diag(FirstDecl->getLocation(),
       << FirstDecl->getSourceRange();
   Diag(SecondDecl->getLocation(),
        diag::note_module_odr_violation_mismatch_decl_unknown)
       << SecondModule.empty() << SecondModule << FirstDiffType
       << SecondDecl->getSourceRange();
   return true;
 }
 
+bool ODRDiagsEmitter::diagnoseMismatch(const RecordDecl *FirstRecord,
+                                       const RecordDecl *SecondRecord) const {
+  if (FirstRecord == SecondRecord)
+    return false;
+
+  std::string FirstModule = getOwningModuleNameForDiagnostic(FirstRecord);
+  std::string SecondModule = getOwningModuleNameForDiagnostic(SecondRecord);
+
+  auto PopulateHashes = [](DeclHashes &Hashes, const RecordDecl *Record,
+                           const DeclContext *DC) {
+    for (const Decl *D : Record->decls()) {
+      if (!ODRHash::isSubDeclToBeProcessed(D, DC))
+        continue;
+      Hashes.emplace_back(D, computeODRHash(D));
+    }
+  };
+
+  DeclHashes FirstHashes;
+  DeclHashes SecondHashes;
+  const DeclContext *DC = FirstRecord;
+  PopulateHashes(FirstHashes, FirstRecord, DC);
+  PopulateHashes(SecondHashes, SecondRecord, DC);
+
+  DiffResult DR = FindTypeDiffs(FirstHashes, SecondHashes);
+  ODRMismatchDecl FirstDiffType = DR.FirstDiffType;
+  ODRMismatchDecl SecondDiffType = DR.SecondDiffType;
+  const Decl *FirstDecl = DR.FirstDecl;
+  const Decl *SecondDecl = DR.SecondDecl;
+
+  if (FirstDiffType == Other || SecondDiffType == Other) {
+    diagnoseSubMismatchUnexpected(DR, FirstRecord, FirstModule, SecondRecord,
+                                  SecondModule);
+    return true;
+  }
+
+  if (FirstDiffType != SecondDiffType) {
+    diagnoseSubMismatchDifferentDeclKinds(DR, FirstRecord, FirstModule,
+                                          SecondRecord, SecondModule);
+    return true;
+  }
+
+  assert(FirstDiffType == SecondDiffType);
+  switch (FirstDiffType) {
+  // Already handled.
+  case EndOfClass:
+  case Other:
+  // C++ only, invalid in this context.
+  case PublicSpecifer:
+  case PrivateSpecifer:
+  case ProtectedSpecifer:
+  case StaticAssert:
+  case CXXMethod:
+  case TypeAlias:
+  case Friend:
+  case FunctionTemplate:
+  // Cannot be contained by RecordDecl, invalid in this context.
+  case ObjCMethod:
+  case ObjCProperty:
+    llvm_unreachable("Invalid diff type");
+
+  case Field: {
+    if (diagnoseSubMismatchField(FirstRecord, FirstModule, SecondModule,
+                                 cast<FieldDecl>(FirstDecl),
+                                 cast<FieldDecl>(SecondDecl)))
+      return true;
+    break;
+  }
+  case TypeDef: {
+    if (diagnoseSubMismatchTypedef(FirstRecord, FirstModule, SecondModule,
+                                   cast<TypedefNameDecl>(FirstDecl),
+                                   cast<TypedefNameDecl>(SecondDecl),
+                                   /*IsTypeAlias=*/false))
+      return true;
+    break;
+  }
+  case Var: {
+    if (diagnoseSubMismatchVar(FirstRecord, FirstModule, SecondModule,
+                               cast<VarDecl>(FirstDecl),
+                               cast<VarDecl>(SecondDecl)))
+      return true;
+    break;
+  }
+  }
+
+  Diag(FirstDecl->getLocation(),
+       diag::err_module_odr_violation_mismatch_decl_unknown)
+      << FirstRecord << FirstModule.empty() << FirstModule << FirstDiffType
+      << FirstDecl->getSourceRange();
+  Diag(SecondDecl->getLocation(),
+       diag::note_module_odr_violation_mismatch_decl_unknown)
+      << SecondModule.empty() << SecondModule << FirstDiffType
+      << SecondDecl->getSourceRange();
+  return true;
+}
+
 bool ODRDiagsEmitter::diagnoseMismatch(
     const FunctionDecl *FirstFunction,
     const FunctionDecl *SecondFunction) const {
   if (FirstFunction == SecondFunction)
     return false;
 
   // Keep in sync with select options in err_module_odr_violation_function.
   enum ODRFunctionDifference {

clang/lib/AST/ODRHash.cpp

 void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
   assert(Record && Record->hasDefinition() &&
          "Expected non-null record to be a definition.");
 
   const DeclContext *DC = Record;
   while (DC) {
     if (isa<ClassTemplateSpecializationDecl>(DC)) {
       return;
     }
     DC = DC->getParent();
   }
 
   AddDecl(Record);
 
   // Filter out sub-Decls which will not be processed in order to get an
   // accurate count of Decl's.
   llvm::SmallVector<const Decl *, 16> Decls;
   for (Decl *SubDecl : Record->decls()) {
void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
   auto Bases = Record->bases();
   for (auto Base : Bases) {
     AddQualType(Base.getType());
     ID.AddInteger(Base.isVirtual());
     ID.AddInteger(Base.getAccessSpecifierAsWritten());
   }
 }
 
+void ODRHash::AddRecordDecl(const RecordDecl *Record) {
+  assert(!isa<CXXRecordDecl>(Record) &&
+         "For CXXRecordDecl should call AddCXXRecordDecl.");
+  AddDecl(Record);
+
+  // Filter out sub-Decls which will not be processed in order to get an
+  // accurate count of Decl's.
+  llvm::SmallVector<const Decl *, 16> Decls;
+  for (Decl *SubDecl : Record->decls()) {
+    if (isSubDeclToBeProcessed(SubDecl, Record))
+      Decls.push_back(SubDecl);
+  }
+
+  ID.AddInteger(Decls.size());
+  for (const Decl *SubDecl : Decls)
+    AddSubDecl(SubDecl);
+}
+
 void ODRHash::AddFunctionDecl(const FunctionDecl *Function,
                               bool SkipBody) {
   assert(Function && "Expecting non-null pointer.");
 
   // Skip functions that are specializations or in specialization context.
   const DeclContext *DC = Function;
   while (DC) {
     if (isa<ClassTemplateSpecializationDecl>(DC)) return;

clang/lib/Serialization/ASTReader.cpp

void ASTReader::finishPendingActions() {
   // Do some cleanup.
   for (auto *ND : PendingMergedDefinitionsToDeduplicate)
     getContext().deduplicateMergedDefinitonsFor(ND);
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
 void ASTReader::diagnoseOdrViolations() {
   if (PendingOdrMergeFailures.empty() && PendingOdrMergeChecks.empty() &&
+      PendingRecordOdrMergeFailures.empty() &&
       PendingFunctionOdrMergeFailures.empty() &&
       PendingEnumOdrMergeFailures.empty() &&
       PendingObjCProtocolOdrMergeFailures.empty())
     return;
 
   // Trigger the import of the full definition of each class that had any
   // odr-merging problems, so we can produce better diagnostics for them.
   // These updates may in turn find and diagnose some ODR failures, so take
   // ownership of the set first.
   auto OdrMergeFailures = std::move(PendingOdrMergeFailures);
   PendingOdrMergeFailures.clear();
   for (auto &Merge : OdrMergeFailures) {
     Merge.first->buildLookup();
     Merge.first->decls_begin();
     Merge.first->bases_begin();
     Merge.first->vbases_begin();
     for (auto &RecordPair : Merge.second) {
       auto *RD = RecordPair.first;
       RD->decls_begin();
       RD->bases_begin();
       RD->vbases_begin();
     }
   }
 
+  // Trigger the import of the full definition of each record in C/ObjC.
+  auto RecordOdrMergeFailures = std::move(PendingRecordOdrMergeFailures);
+  PendingRecordOdrMergeFailures.clear();
+  for (auto &Merge : RecordOdrMergeFailures) {
+    Merge.first->decls_begin();
+    for (auto &D : Merge.second)
+      D->decls_begin();
+  }
+
   // Trigger the import of functions.
   auto FunctionOdrMergeFailures = std::move(PendingFunctionOdrMergeFailures);
   PendingFunctionOdrMergeFailures.clear();
   for (auto &Merge : FunctionOdrMergeFailures) {
     Merge.first->buildLookup();
     Merge.first->decls_begin();
     Merge.first->getBody();
     for (auto &FD : Merge.second) {
if (!Found) {
                diag::note_module_odr_violation_possible_decl)
             << Candidates[I];
       }
 
       DiagnosedOdrMergeFailures.insert(CanonDef);
     }
   }
 
-  if (OdrMergeFailures.empty() && FunctionOdrMergeFailures.empty() &&
-      EnumOdrMergeFailures.empty() && ObjCProtocolOdrMergeFailures.empty())
+  if (OdrMergeFailures.empty() && RecordOdrMergeFailures.empty() &&
+      FunctionOdrMergeFailures.empty() && EnumOdrMergeFailures.empty() &&
+      ObjCProtocolOdrMergeFailures.empty())
     return;
 
   // Ensure we don't accidentally recursively enter deserialization while
   // we're producing our diagnostics.
   Deserializing RecursionGuard(this);
   ODRDiagsEmitter DiagsEmitter(Diags, getContext(),
                                getPreprocessor().getLangOpts());
 
   // Issue any pending ODR-failure diagnostics.
   for (auto &Merge : OdrMergeFailures) {
     // If we've already pointed out a specific problem with this class, don't
     // bother issuing a general "something's different" diagnostic.
     if (!DiagnosedOdrMergeFailures.insert(Merge.first).second)
       continue;
 
     bool Diagnosed = false;
if (!Diagnosed) {
       // FIXME: Indicate which modules had instantiations of this definition.
       // FIXME: How can this even happen?
       Diag(Merge.first->getLocation(),
            diag::err_module_odr_violation_different_instantiations)
           << Merge.first;
     }
   }
 
+  // Issue any pending ODR-failure diagnostics for RecordDecl in C/ObjC. Note
+  // that in C++ this is done as a part of CXXRecordDecl ODR checking.
+  for (auto &Merge : RecordOdrMergeFailures) {
+    // If we've already pointed out a specific problem with this class, don't
+    // bother issuing a general "something's different" diagnostic.
+    if (!DiagnosedOdrMergeFailures.insert(Merge.first).second)
+      continue;
+
+    RecordDecl *FirstRecord = Merge.first;
+    bool Diagnosed = false;
+    for (auto *SecondRecord : Merge.second) {
+      if (DiagsEmitter.diagnoseMismatch(FirstRecord, SecondRecord)) {
+        Diagnosed = true;
+        break;
+      }
+    }
+    (void)Diagnosed;
+    assert(Diagnosed && "Unable to emit ODR diagnostic.");
+  }
+
   // Issue ODR failures diagnostics for functions.
   for (auto &Merge : FunctionOdrMergeFailures) {
     FunctionDecl *FirstFunction = Merge.first;
     bool Diagnosed = false;
     for (auto &SecondFunction : Merge.second) {
       if (DiagsEmitter.diagnoseMismatch(FirstFunction, SecondFunction)) {
         Diagnosed = true;
         break;

clang/lib/Serialization/ASTReaderDecl.cpp

ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
   RD->setHasNonTrivialToPrimitiveCopyCUnion(Record.readInt());
   RD->setParamDestroyedInCallee(Record.readInt());
   RD->setArgPassingRestrictions((RecordDecl::ArgPassingKind)Record.readInt());
   return Redecl;
 }
 
 void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) {
   VisitRecordDeclImpl(RD);
+  RD->setODRHash(Record.readInt());
 
   // Maintain the invariant of a redeclaration chain containing only
   // a single definition.
   if (RD->isCompleteDefinition()) {
     RecordDecl *Canon = static_cast<RecordDecl *>(RD->getCanonicalDecl());
     RecordDecl *&OldDef = Reader.RecordDefinitions[Canon];
     if (!OldDef) {
       // This is the first time we've seen an imported definition. Look for a
       // local definition before deciding that we are the first definition.
       for (auto *D : merged_redecls(Canon)) {
         if (!D->isFromASTFile() && D->isCompleteDefinition()) {
           OldDef = D;
           break;
         }
       }
     }
     if (OldDef) {
       Reader.MergedDeclContexts.insert(std::make_pair(RD, OldDef));
       RD->demoteThisDefinitionToDeclaration();
       Reader.mergeDefinitionVisibility(OldDef, RD);
+      if (OldDef->getODRHash() != RD->getODRHash())
+        Reader.PendingRecordOdrMergeFailures[OldDef].push_back(RD);
     } else {
       OldDef = RD;
     }
   }
 }
 
 void ASTDeclReader::VisitValueDecl(ValueDecl *VD) {
   VisitNamedDecl(VD);

clang/lib/Serialization/ASTWriterDecl.cpp

void ASTDeclWriter::VisitRecordDecl(RecordDecl *D) {
   Record.push_back(D->isNonTrivialToPrimitiveDefaultInitialize());
   Record.push_back(D->isNonTrivialToPrimitiveCopy());
   Record.push_back(D->isNonTrivialToPrimitiveDestroy());
   Record.push_back(D->hasNonTrivialToPrimitiveDefaultInitializeCUnion());
   Record.push_back(D->hasNonTrivialToPrimitiveDestructCUnion());
   Record.push_back(D->hasNonTrivialToPrimitiveCopyCUnion());
   Record.push_back(D->isParamDestroyedInCallee());
   Record.push_back(D->getArgPassingRestrictions());
+  // Only compute this for C/Objective-C, in C++ this is computed as part
+  // of CXXRecordDecl.
+  if (!isa<CXXRecordDecl>(D))
+    Record.push_back(D->getODRHash());
 
   if (D->getDeclContext() == D->getLexicalDeclContext() &&
       !D->hasAttrs() &&
       !D->isImplicit() &&
       !D->isUsed(false) &&
       !D->hasExtInfo() &&
       !D->getTypedefNameForAnonDecl() &&
       D->getFirstDecl() == D->getMostRecentDecl() &&
void ASTWriter::WriteDeclAbbrevs() {
   // hasNonTrivialToPrimitiveDestructCUnion
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1));
   // hasNonTrivialToPrimitiveCopyCUnion
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1));
   // isParamDestroyedInCallee
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1));
   // getArgPassingRestrictions
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2));
+  // ODRHash
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 26));
 
   // DC
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // LexicalOffset
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));   // VisibleOffset
   DeclRecordAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for DECL_PARM_VAR
   Abv = std::make_shared<BitCodeAbbrev>();

clang/test/Modules/compare-record.c

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/include/first.h
+// RUN: cat %t/test.c        >> %t/include/first.h
+// RUN: echo "#undef FIRST"  >> %t/include/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/include/second.h
+// RUN: cat %t/test.c         >> %t/include/second.h
+// RUN: echo "#undef SECOND"  >> %t/include/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/first.h -fblocks -fobjc-arc
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/second.h -fblocks -fobjc-arc
+
+// Run test
+// RUN: %clang_cc1 -I%t/include -verify %t/test.c -fblocks -fobjc-arc \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Run tests for nested structs
+// DEFINE: %{filename} = test-nested-struct.c
+// DEFINE: %{macro_flag} = -DCASE1=1
+// DEFINE: %{command} = %clang_cc1 -I%t/include -verify %t/%{filename} -fblocks -fobjc-arc \
+// DEFINE:             -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache \
+// DEFINE:             %{macro_flag} -emit-llvm -o %t/%{filename}.bc
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE2=1
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE3=1
+// RUN: %{command}
+
+// Test that we don't accept different structs and unions with the same name
+// from multiple modules but detect mismatches and provide actionable
+// diagnostic.
+
+//--- include/first-empty.h
+//--- include/module.modulemap
+module First {
+  module Empty {
+    header "first-empty.h"
+  }
+  module Hidden {
+    header "first.h"
+    header "first-nested-struct.h"
+    export *
+  }
+}
+module Second {
+  header "second.h"
+  header "second-nested-struct.h"
+  export *
+}
+
+//--- test.c
+#if !defined(FIRST) && !defined(SECOND)
+# include "first-empty.h"
+# include "second.h"
+#endif
+
+#if defined(FIRST)
+struct CompareForwardDeclaration1;
+struct CompareForwardDeclaration2 {};
+#elif defined(SECOND)
+struct CompareForwardDeclaration1 {};
+struct CompareForwardDeclaration2;
+#else
+struct CompareForwardDeclaration1 *compareForwardDeclaration1;
+struct CompareForwardDeclaration2 *compareForwardDeclaration2;
+#endif
+
+#if defined(FIRST)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+  int fieldPresence1;
+};
+struct CompareFieldPresence2 {};
+
+struct CompareFieldName {
+  int fieldNameA;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderX;
+  int fieldOrderY;
+};
+#elif defined(SECOND)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+};
+struct CompareFieldPresence2 {
+  int fieldPresence2;
+};
+
+struct CompareFieldName {
+  int fieldNameB;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderY;
+  int fieldOrderX;
+};
+#else
+struct CompareMatchingFields compareMatchingFields;
+struct CompareFieldPresence1 compareFieldPresence1;
+// expected-error@first.h:* {{'CompareFieldPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field}}
+// expected-note@second.h:* {{but in 'Second' found end of class}}
+struct CompareFieldPresence2 compareFieldPresence2;
+// expected-error@second.h:* {{'CompareFieldPresence2::fieldPresence2' from module 'Second' is not present in definition of 'struct CompareFieldPresence2' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldPresence2'}}
+struct CompareFieldName compareFieldName;
+// expected-error@second.h:* {{'CompareFieldName::fieldNameB' from module 'Second' is not present in definition of 'struct CompareFieldName' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldNameB'}}
+struct CompareFieldOrder compareFieldOrder;
+// expected-error@first.h:* {{'CompareFieldOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'fieldOrderX'}}
+// expected-note@second.h:* {{but in 'Second' found field 'fieldOrderY'}}
+#endif
+
+#if defined(FIRST)
+struct CompareFieldType {
+  int fieldType;
+};
+
+typedef int FieldTypedefNameA;
+struct CompareFieldTypedefName {
+  FieldTypedefNameA fieldTypedefName;
+};
+
+typedef int TypedefUnderlyingType;
+struct CompareFieldTypeUnderlyingTypedef {
+  TypedefUnderlyingType fieldTypeUnderlyingTypedef;
+};
+
+typedef int TypedefFinal;
+struct CompareFieldTypedefChain {
+  TypedefFinal fieldTypeTypedefChain;
+};
+#elif defined(SECOND)
+struct CompareFieldType {
+  float fieldType;
+};
+
+typedef int FieldTypedefNameB;
+struct CompareFieldTypedefName {
+  FieldTypedefNameB fieldTypedefName;
+};
+
+struct CompareFieldTypeUnderlyingTypedef {
+  int fieldTypeUnderlyingTypedef;
+};
+
+typedef int TypedefIntermediate;
+typedef TypedefIntermediate TypedefFinal;
+struct CompareFieldTypedefChain {
+  TypedefFinal fieldTypeTypedefChain;
+};
+#else
+struct CompareFieldType compareFieldType;
+// expected-error@second.h:* {{'CompareFieldType::fieldType' from module 'Second' is not present in definition of 'struct CompareFieldType' in module 'First.Hidden'}}
+// expected-note@first.h:* {{declaration of 'fieldType' does not match}}
+struct CompareFieldTypedefName compareFieldTypedefName;
+// expected-error@first.h:* {{'CompareFieldTypedefName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'fieldTypedefName' with type 'FieldTypedefNameA' (aka 'int')}}
+// expected-note@second.h:* {{but in 'Second' found field 'fieldTypedefName' with type 'FieldTypedefNameB' (aka 'int')}}
+struct CompareFieldTypeUnderlyingTypedef compareFieldTypeUnderlyingTypedef;
+// expected-error@first.h:* {{'CompareFieldTypeUnderlyingTypedef' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'fieldTypeUnderlyingTypedef' with type 'TypedefUnderlyingType' (aka 'int')}}
+// expected-note@second.h:* {{but in 'Second' found field 'fieldTypeUnderlyingTypedef' with type 'int'}}
+struct CompareFieldTypedefChain compareFieldTypedefChain;
+#endif
+
+#if defined(FIRST)
+struct CompareMatchingBitfields {
+  unsigned matchingBitfields : 3;
+};
+
+struct CompareBitfieldPresence1 {
+  unsigned bitfieldPresence1 : 1;
+};
+struct CompareBitfieldPresence2 {
+  unsigned bitfieldPresence2;
+};
+
+struct CompareBitfieldWidth {
+  unsigned bitfieldWidth : 2;
+};
+
+struct CompareBitfieldWidthExpression {
+  unsigned bitfieldWidthExpression : 1 + 1;
+};
+#elif defined(SECOND)
+struct CompareMatchingBitfields {
+  unsigned matchingBitfields : 3;
+};
+
+struct CompareBitfieldPresence1 {
+  unsigned bitfieldPresence1;
+};
+struct CompareBitfieldPresence2 {
+  unsigned bitfieldPresence2 : 1;
+};
+
+struct CompareBitfieldWidth {
+  unsigned bitfieldWidth : 1;
+};
+
+struct CompareBitfieldWidthExpression {
+  unsigned bitfieldWidthExpression : 2;
+};
+#else
+struct CompareMatchingBitfields compareMatchingBitfields;
+struct CompareBitfieldPresence1 compareBitfieldPresence1;
+// expected-error@first.h:* {{'CompareBitfieldPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found bitfield 'bitfieldPresence1'}}
+// expected-note@second.h:* {{but in 'Second' found non-bitfield 'bitfieldPresence1'}}
+struct CompareBitfieldPresence2 compareBitfieldPresence2;
+// expected-error@first.h:* {{'CompareBitfieldPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found non-bitfield 'bitfieldPresence2'}}
+// expected-note@second.h:* {{but in 'Second' found bitfield 'bitfieldPresence2'}}
+struct CompareBitfieldWidth compareBitfieldWidth;
+// expected-error@first.h:* {{'CompareBitfieldWidth' has different definitions in different modules; first difference is definition in module 'First.Hidden' found bitfield 'bitfieldWidth' with one width expression}}
+// expected-note@second.h:* {{but in 'Second' found bitfield 'bitfieldWidth' with different width expression}}
+struct CompareBitfieldWidthExpression compareBitfieldWidthExpression;
+// expected-error@first.h:* {{'CompareBitfieldWidthExpression' has different definitions in different modules; first difference is definition in module 'First.Hidden' found bitfield 'bitfieldWidthExpression' with one width expression}}
+// expected-note@second.h:* {{but in 'Second' found bitfield 'bitfieldWidthExpression' with different width expressio}}
+#endif
+
+#if defined(FIRST)
+struct CompareMatchingArrayFields {
+  int matchingArrayField[7];
+};
+
+struct CompareArrayLength {
+  int arrayLengthField[5];
+};
+
+struct CompareArrayType {
+  int arrayTypeField[5];
+};
+#elif defined(SECOND)
+struct CompareMatchingArrayFields {
+  int matchingArrayField[7];
+};
+
+struct CompareArrayLength {
+  int arrayLengthField[7];
+};
+
+struct CompareArrayType {
+  float arrayTypeField[5];
+};
+#else
+struct CompareMatchingArrayFields compareMatchingArrayFields;
+struct CompareArrayLength compareArrayLength;
+// expected-error@second.h:* {{'CompareArrayLength::arrayLengthField' from module 'Second' is not present in definition of 'struct CompareArrayLength' in module 'First.Hidden'}}
+// expected-note@first.h:* {{declaration of 'arrayLengthField' does not match}}
+struct CompareArrayType compareArrayType;
+// expected-error@second.h:* {{'CompareArrayType::arrayTypeField' from module 'Second' is not present in definition of 'struct CompareArrayType' in module 'First.Hidden'}}
+// expected-note@first.h:* {{declaration of 'arrayTypeField' does not match}}
+#endif
+
+#if defined(FIRST)
+struct CompareFieldAsForwardDeclaration {
+  struct FieldForwardDeclaration *fieldForwardDeclaration;
+};
+
+enum FieldEnumA { kFieldEnumValue };
+struct CompareFieldAsEnum {
+  enum FieldEnumA fieldEnum;
+};
+
+struct FieldStructA {};
+struct CompareFieldAsStruct {
+  struct FieldStructA fieldStruct;
+};
+#elif defined(SECOND)
+struct FieldForwardDeclaration {};
+struct CompareFieldAsForwardDeclaration {
+  struct FieldForwardDeclaration *fieldForwardDeclaration;
+};
+
+enum FieldEnumB { kFieldEnumValue };
+struct CompareFieldAsEnum {
+  enum FieldEnumB fieldEnum;
+};
+
+struct FieldStructB {};
+struct CompareFieldAsStruct {
+  struct FieldStructB fieldStruct;
+};
+#else
+struct CompareFieldAsForwardDeclaration compareFieldAsForwardDeclaration;
+struct CompareFieldAsEnum compareFieldAsEnum;
+// expected-error@second.h:* {{'CompareFieldAsEnum::fieldEnum' from module 'Second' is not present in definition of 'struct CompareFieldAsEnum' in module 'First.Hidden'}}
+// expected-note@first.h:* {{declaration of 'fieldEnum' does not match}}
+struct CompareFieldAsStruct compareFieldAsStruct;
+// expected-error@second.h:* {{'CompareFieldAsStruct::fieldStruct' from module 'Second' is not present in definition of 'struct CompareFieldAsStruct' in module 'First.Hidden'}}
+// expected-note@first.h:* {{declaration of 'fieldStruct' does not match}}
+#endif
+
+#if defined(FIRST)
+union CompareMatchingUnionFields {
+  int matchingFieldA;
+  float matchingFieldB;
+};
+
+union CompareUnionFieldOrder {
+  int unionFieldOrderA;
+  float unionFieldOrderB;
+};
+
+union CompareUnionFieldType {
+  int unionFieldType;
+};
+#elif defined(SECOND)
+union CompareMatchingUnionFields {
+  int matchingFieldA;
+  float matchingFieldB;
+};
+
+union CompareUnionFieldOrder {
+  float unionFieldOrderB;
+  int unionFieldOrderA;
+};
+
+union CompareUnionFieldType {
+  unsigned int unionFieldType;
+};
+#else
+union CompareMatchingUnionFields compareMatchingUnionFields;
+union CompareUnionFieldOrder compareUnionFieldOrder;
+// expected-error@first.h:* {{'CompareUnionFieldOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'unionFieldOrderA'}}
+// expected-note@second.h:* {{but in 'Second' found field 'unionFieldOrderB'}}
+union CompareUnionFieldType compareUnionFieldType;
+// expected-error@second.h:* {{'CompareUnionFieldType::unionFieldType' from module 'Second' is not present in definition of 'union CompareUnionFieldType' in module 'First.Hidden'}}
+// expected-note@first.h:* {{declaration of 'unionFieldType' does not match}}
+#endif
+
+// Test that we find and compare definitions even if they are not the first encountered declaration in a module.
+#if defined(FIRST)
+struct CompareDefinitionsRegardlessForwardDeclarations {
+  int definitionField;
+};
+#elif defined(SECOND)
+struct CompareDefinitionsRegardlessForwardDeclarations;
+struct CompareDefinitionsRegardlessForwardDeclarations {
+  float definitionField;
+};
+#else
+struct CompareDefinitionsRegardlessForwardDeclarations compareDefinitions;
+// expected-error@second.h:* {{'CompareDefinitionsRegardlessForwardDeclarations::definitionField' from module 'Second' is not present in definition of 'struct CompareDefinitionsRegardlessForwardDeclarations' in module 'First.Hidden'}}
+// expected-note@first.h:* {{declaration of 'definitionField' does not match}}
+#endif
+
+//--- include/first-nested-struct.h
+struct CompareNestedStruct {
+  struct NestedLevel1 {
+    struct NestedLevel2 {
+      int a;
+    } y;
+  } x;
+};
+
+struct IndirectStruct {
+  int mismatchingField;
+};
+struct DirectStruct {
+  struct IndirectStruct indirectField;
+};
+struct CompareDifferentFieldInIndirectStruct {
+  struct DirectStruct directField;
+};
+struct CompareIndirectStructPointer {
+  struct DirectStruct *directFieldPointer;
+};
+
+//--- include/second-nested-struct.h
+struct CompareNestedStruct {
+  struct NestedLevel1 {
+    struct NestedLevel2 {
+      float b;
+    } y;
+  } x;
+};
+
+struct IndirectStruct {
+  float mismatchingField;
+};
+struct DirectStruct {
+  struct IndirectStruct indirectField;
+};
+struct CompareDifferentFieldInIndirectStruct {
+  struct DirectStruct directField;
+};
+struct CompareIndirectStructPointer {
+  struct DirectStruct *directFieldPointer;
+};
+
+//--- test-nested-struct.c
+#include "first-empty.h"
+#include "second-nested-struct.h"
+
+#if defined(CASE1)
+struct CompareNestedStruct compareNestedStruct;
+// expected-error@second-nested-struct.h:* {{'NestedLevel2::b' from module 'Second' is not present in definition of 'struct NestedLevel2' in module 'First.Hidden'}}
+// expected-note@first-nested-struct.h:* {{definition has no member 'b'}}
+#elif defined(CASE2)
+struct CompareDifferentFieldInIndirectStruct compareIndirectStruct;
+// expected-error@second-nested-struct.h:* {{'IndirectStruct::mismatchingField' from module 'Second' is not present in definition of 'struct IndirectStruct' in module 'First.Hidden'}}
+// expected-note@first-nested-struct.h:* {{declaration of 'mismatchingField' does not match}}
+#elif defined(CASE3)
+struct CompareIndirectStructPointer compareIndirectStructPointer;
+// expected-error@second-nested-struct.h:* {{'IndirectStruct::mismatchingField' from module 'Second' is not present in definition of 'struct IndirectStruct' in module 'First.Hidden'}}
+// expected-note@first-nested-struct.h:* {{declaration of 'mismatchingField' does not match}}
+#endif