[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

9388

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

9388

Great idea. Updated the patch with another approach!

[bruno]

Remove some FIXMEs that are now done.

[rtrieu]

clang/lib/Serialization/ASTReader.cpp

11199

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

[bruno]

clang/lib/Serialization/ASTReader.cpp

11199

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

11199

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

4686

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

4086–4087

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

480–481

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

9671–9673

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

4086–4087

Makes sense!

clang/lib/AST/ODRHash.cpp

480–481

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

9671–9673

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

9671–9673

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

492

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

9671–9673

@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

9671–9673

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

492

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

818

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

4687–4688

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 ↗

(On Diff #482981)

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

541–542

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

[vsapsai]

clang/lib/AST/Decl.cpp

4687–4688

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 ↗

(On Diff #482981)

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

541–542

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

4517

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

4687–4688

It sounds not bad.

clang/lib/AST/ODRDiagsEmitter.cpp

1550–1551 ↗

(On Diff #482981)

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

4517

You are right, changed it.

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 {
     uint64_t HasNonTrivialToPrimitiveDestructCUnion : 1;
     uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1;
 
     /// 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;
+
+    /// 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 : 28;
   };
 
   /// Number of non-inherited bits in RecordDeclBitfields.
-  enum { NumRecordDeclBits = 14 };
+  enum { NumRecordDeclBits = 42 };
 
   /// 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/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

private:
   /// 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;
 
+  /// C/ObjC definitions in which the structural equivalence check fails
+  llvm::SmallDenseMap<RecordDecl *, llvm::SmallVector<RecordDecl *, 2>, 2>
+      PendingRecordOdrMergeFailures;
+
   /// DeclContexts in which we have diagnosed an ODR violation.
   llvm::SmallPtrSet<DeclContext*, 2> DiagnosedOdrMergeFailures;
 
   /// The set of Objective-C categories that have been deserialized
   /// since the last time the declaration chains were linked.
   llvm::SmallPtrSet<ObjCCategoryDecl *, 16> CategoriesDeserialized;
 
   /// The set of Objective-C class definitions that have already been

clang/lib/AST/Decl.cpp

RecordDecl::RecordDecl(Kind DK, TagKind TK, const ASTContext &C,
   setNonTrivialToPrimitiveDefaultInitialize(false);
   setNonTrivialToPrimitiveCopy(false);
   setNonTrivialToPrimitiveDestroy(false);
   setHasNonTrivialToPrimitiveDefaultInitializeCUnion(false);
   setHasNonTrivialToPrimitiveDestructCUnion(false);
   setHasNonTrivialToPrimitiveCopyCUnion(false);
   setParamDestroyedInCallee(false);
   setArgPassingRestrictions(APK_CanPassInRegs);
+  RecordDeclBits.ODRHash = 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 28
+  // bit of RecordDeclBits, adjust the hash to accomodate.
+  setODRHash(Hash.CalculateHash() >> 4);
+  return RecordDeclBits.ODRHash;
+}
+
 //===----------------------------------------------------------------------===//
 // BlockDecl Implementation
 //===----------------------------------------------------------------------===//
 
 BlockDecl::BlockDecl(DeclContext *DC, SourceLocation CaretLoc)
     : Decl(Block, DC, CaretLoc), DeclContext(Block) {
   setIsVariadic(false);
   setCapturesCXXThis(false);

clang/lib/AST/ODRHash.cpp

 }
 
 void ODRHash::AddSubDecl(const Decl *D) {
   assert(D && "Expecting non-null pointer.");
 
   ODRDeclVisitor(ID, *this).Visit(D);
 }
 
+void ODRHash::AddRecordDecl(const RecordDecl *Record) {
+  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 (auto *SubRD = dyn_cast<RecordDecl>(SubDecl)) {
+      if (!SubRD->isAnonymousStructOrUnion())
+        continue;
+      ID.AddInteger(SubRD->getODRHash());
+      continue;
+    }
+    if (isDeclToBeProcessed(SubDecl, Record))
+      Decls.push_back(SubDecl);
+  }
+
+  ID.AddInteger(Decls.size());
+  for (auto SubDecl : Decls)
+    AddSubDecl(SubDecl);
+}
+
 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;
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::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;
     if (auto *F = dyn_cast<FunctionDecl>(DC)) {
       if (F->isFunctionTemplateSpecialization()) {

clang/lib/Serialization/ASTReader.cpp

void ASTReader::finishPendingActions() {
   for (auto *ND : PendingMergedDefinitionsToDeduplicate)
     getContext().deduplicateMergedDefinitonsFor(ND);
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
 void ASTReader::diagnoseOdrViolations() {
   if (PendingOdrMergeFailures.empty() && PendingOdrMergeChecks.empty() &&
       PendingFunctionOdrMergeFailures.empty() &&
-      PendingEnumOdrMergeFailures.empty())
+      PendingEnumOdrMergeFailures.empty() &&
+      PendingRecordOdrMergeFailures.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) {
             << Candidates[I];
       }
 
       DiagnosedOdrMergeFailures.insert(CanonDef);
     }
   }
 
   if (OdrMergeFailures.empty() && FunctionOdrMergeFailures.empty() &&
-      EnumOdrMergeFailures.empty())
+      EnumOdrMergeFailures.empty() && RecordOdrMergeFailures.empty())
     return;
 
   // Ensure we don't accidentally recursively enter deserialization while
   // we're producing our diagnostics.
   Deserializing RecursionGuard(this);
 
   // Common code for hashing helpers.
   ODRHash Hash;
if (FirstII->getName() != SecondII->getName()) {
           << FirstII;
       ODRDiagDeclNote(SecondModule, SecondField->getLocation(),
                       SecondField->getSourceRange(), FieldName)
           << SecondII;
 
       return true;
     }
 
-    assert(getContext().hasSameType(FirstField->getType(),
-                                    SecondField->getType()));
-
     QualType FirstType = FirstField->getType();
     QualType SecondType = SecondField->getType();
     if (ComputeQualTypeODRHash(FirstType) !=
         ComputeQualTypeODRHash(SecondType)) {
       ODRDiagDeclError(FirstRecord, FirstModule, FirstField->getLocation(),
                        FirstField->getSourceRange(), FieldTypeName)
           << FirstII << FirstType;
       ODRDiagDeclNote(SecondModule, SecondField->getLocation(),
                       SecondField->getSourceRange(), FieldTypeName)
           << SecondII << SecondType;
 
       return true;
     }
 
     const bool IsFirstBitField = FirstField->isBitField();
     const bool IsSecondBitField = SecondField->isBitField();
     if (IsFirstBitField != IsSecondBitField) {
       ODRDiagDeclError(FirstRecord, FirstModule, FirstField->getLocation(),
                        FirstField->getSourceRange(), FieldSingleBitField)
           << FirstII << IsFirstBitField;
       ODRDiagDeclNote(SecondModule, SecondField->getLocation(),
                       SecondField->getSourceRange(), FieldSingleBitField)
           << SecondII << IsSecondBitField;
       return true;
     }
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;
     }
   }
 
+  auto PopulateRecordHashes = [&ComputeSubDeclODRHash](DeclHashes &Hashes,
+                                                       RecordDecl *Record,
+                                                       const DeclContext *DC) {
+    std::deque<std::pair<Decl *, const DeclContext *>> WorkList;
+    for (auto *D : Record->decls())
+      WorkList.push_back(std::make_pair(D, DC));
+
+    while (!WorkList.empty()) {
+      auto &P = WorkList.front();
+      WorkList.pop_front();
+      auto *SubRec = dyn_cast<RecordDecl>(P.first);
+      if (!SubRec) {
+        if (!ODRHash::isDeclToBeProcessed(P.first, P.second))
+          continue;
+        Hashes.emplace_back(P.first, ComputeSubDeclODRHash(P.first));
+        continue;
+      }
+      if (!SubRec->isAnonymousStructOrUnion())
+        continue;
+      for (auto *SubD : SubRec->decls())
+        WorkList.push_front(std::make_pair(SubD, SubRec));
+    }
+  };
+
+  // Issue any pending ODR-failure (for structural equivalence checks)
+  // diagnostics for RecordDecl in C/ObjC, note that in C++ this is
+  // done as paert 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;
+
+    bool Diagnosed = false;
+    RecordDecl *FirstRecord = Merge.first;
+
+    std::string FirstModule = getOwningModuleNameForDiagnostic(FirstRecord);
+    for (auto *SecondRecord : Merge.second) {
+      // Multiple different declarations got merged together; tell the user
+      // where they came from.
+      if (FirstRecord == SecondRecord)
+        continue;
+
+      std::string SecondModule = getOwningModuleNameForDiagnostic(SecondRecord);
+      DeclHashes FirstHashes;
+      DeclHashes SecondHashes;
+
+      const DeclContext *DC = FirstRecord;
+      PopulateRecordHashes(FirstHashes, FirstRecord, DC);
+      PopulateRecordHashes(SecondHashes, SecondRecord, DC);
+
+      auto DR = FindTypeDiffs(FirstHashes, SecondHashes);
+      ODRMismatchDecl FirstDiffType = DR.FirstDiffType;
+      ODRMismatchDecl SecondDiffType = DR.SecondDiffType;
+      Decl *FirstDecl = DR.FirstDecl;
+      Decl *SecondDecl = DR.SecondDecl;
+
+      // Reaching this point means an unexpected Decl was encountered
+      // or no difference was detected.  This causes a generic error
+      // message to be emitted.
+      if (FirstDiffType == Other || SecondDiffType == Other) {
+        DiagnoseODRUnexpected(DR, FirstRecord, FirstModule, SecondRecord,
+                              SecondModule);
+        Diagnosed = true;
+        break;
+      }
+
+      if (FirstDiffType != SecondDiffType) {
+        DiagnoseODRMismatch(DR, FirstRecord, FirstModule, SecondRecord,
+                            SecondModule);
+        Diagnosed = true;
+        break;
+      }
+
+      assert(FirstDiffType == SecondDiffType);
+      switch (FirstDiffType) {
+      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:
+        llvm_unreachable("Invalid diff type");
+
+      case Field: {
+        Diagnosed = ODRDiagField(FirstRecord, FirstModule, SecondModule,
+                                 cast<FieldDecl>(FirstDecl),
+                                 cast<FieldDecl>(SecondDecl));
+        break;
+      }
+      case TypeDef: {
+        Diagnosed = ODRDiagTypeDefOrAlias(
+            FirstRecord, FirstModule, SecondModule,
+            cast<TypedefNameDecl>(FirstDecl), cast<TypedefNameDecl>(SecondDecl),
+            false /* IsTypeAlias */);
+        break;
+      }
+      case Var: {
+        Diagnosed =
+            ODRDiagVar(FirstRecord, FirstModule, SecondModule,
+                       cast<VarDecl>(FirstDecl), cast<VarDecl>(SecondDecl));
+        break;
+      }
+      }
+
+      if (Diagnosed)
+        continue;
+
+      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 << FirstDiffType << SecondDecl->getSourceRange();
+      Diagnosed = true;
+    }
+  }
+
   // Issue ODR failures diagnostics for functions.
   for (auto &Merge : FunctionOdrMergeFailures) {
     enum ODRFunctionDifference {
       ReturnType,
       ParameterName,
       ParameterType,
       ParameterSingleDefaultArgument,
       ParameterDifferentDefaultArgument,

clang/lib/Serialization/ASTReaderDecl.cpp

ASTDeclReader::RedeclarableResult ASTDeclReader::VisitTagDecl(TagDecl *TD) {
   case 2: // TypedefNameForAnonDecl
     NamedDeclForTagDecl = readDeclID();
     TypedefNameForLinkage = Record.readIdentifier();
     break;
   default:
     llvm_unreachable("unexpected tag info kind");
   }
 
-  if (!isa<CXXRecordDecl>(TD))
-    mergeRedeclarable(TD, Redecl);
+  if (isa<CXXRecordDecl>(TD))
+    return Redecl;
+
+  // Handle merging in C and Objective-C
   return Redecl;
 }
 
 void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
   VisitTagDecl(ED);
   if (TypeSourceInfo *TI = readTypeSourceInfo())
     ED->setIntegerTypeSourceInfo(TI);
   else
ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
   RD->setNonTrivialToPrimitiveDefaultInitialize(Record.readInt());
   RD->setNonTrivialToPrimitiveCopy(Record.readInt());
   RD->setNonTrivialToPrimitiveDestroy(Record.readInt());
   RD->setHasNonTrivialToPrimitiveDefaultInitializeCUnion(Record.readInt());
   RD->setHasNonTrivialToPrimitiveDestructCUnion(Record.readInt());
   RD->setHasNonTrivialToPrimitiveCopyCUnion(Record.readInt());
   RD->setParamDestroyedInCallee(Record.readInt());
   RD->setArgPassingRestrictions((RecordDecl::ArgPassingKind)Record.readInt());
+  if (!isa<CXXRecordDecl>(RD)) {
+    RD->setODRHash(Record.readInt());
+
+    // C++ applies ODR checking in VisitCXXRecordDecl instead. Note that
+    // structural equivalence is the usual way to check for ODR-like semantics
+    // in ObjC/C, but using ODRHash is prefered if possible because of better
+    // performance.
+    RecordDecl *Canon = static_cast<RecordDecl *>(RD->getCanonicalDecl());
+    if (RD == Canon || Canon->getODRHash() == RD->getODRHash())
+      return Redecl;
+    // No point in checking equivalence between types that don't match in
+    // presence of definition. Note that we might still wanna check when
+    // both types don't have a definition (e.g. when adding checks for
+    // mismtaches in their __attribute__ lists)
+    if (RD->isThisDeclarationADefinition() !=
+        Canon->isThisDeclarationADefinition())
+      return Redecl;
+    Reader.PendingRecordOdrMergeFailures[Canon].push_back(RD);
+    // Track that we merged the definitions.
+    Reader.MergedDeclContexts.insert(std::make_pair(RD, Canon));
+  }
+
   return Redecl;
 }
 
 void ASTDeclReader::VisitValueDecl(ValueDecl *VD) {
   VisitNamedDecl(VD);
   // For function declarations, defer reading the type in case the function has
   // a deduced return type that references an entity declared within the
   // function.

 /// check in Sema guarantees the types can be merged (see C11 6.2.7/1 or C89
 /// 6.1.2.6/1). Although most merging is done in Sema, we need to guarantee
 /// that some types are mergeable during deserialization, otherwise name
 /// lookup fails. This is the case for EnumConstantDecl.
 static bool allowODRLikeMergeInC(NamedDecl *ND) {
   if (!ND)
     return false;
   // TODO: implement merge for other necessary decls.
-  if (isa<EnumConstantDecl>(ND))
+  if (isa<EnumConstantDecl>(ND) || isa<FieldDecl>(ND) ||
+      isa<IndirectFieldDecl>(ND))
     return true;
   return false;
 }
 
 /// Attempts to merge LifetimeExtendedTemporaryDecl with
 /// identical class definitions from two different modules.
 void ASTDeclReader::mergeMergeable(LifetimeExtendedTemporaryDecl *D) {
   // If modules are not available, there is no reason to perform this merge.
if (auto *RD = dyn_cast<CXXRecordDecl>(DC)) {
 
     return DD->Definition;
   }
 
   if (auto *ED = dyn_cast<EnumDecl>(DC))
     return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
                                                       : nullptr;
 
+  if (auto *RD = dyn_cast<RecordDecl>(DC))
+    if (!RD->getASTContext().getLangOpts().CPlusPlus)
+      return RD->getCanonicalDecl()->getDefinition();
+
   // We can see the TU here only if we have no Sema object. In that case,
   // there's no TU scope to look in, so using the DC alone is sufficient.
   if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))
     return TU;
 
   return nullptr;
 }
 

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, 28));
 
   // 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/odr_hash-record.c

+// 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
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {"     >> %t/Inputs/module.map
+// RUN: echo "    header \"first.h\""   >> %t/Inputs/module.map
+// RUN: echo "}"                        >> %t/Inputs/module.map
+// RUN: echo "module SecondModule {"    >> %t/Inputs/module.map
+// RUN: echo "    header \"second.h\""  >> %t/Inputs/module.map
+// RUN: echo "}"                        >> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -x c \
+// RUN:   -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {};
+struct S1 s1a;
+#elif defined(SECOND)
+struct S1 {};
+#else
+struct S1 s1;
+#endif
+
+#if defined(FIRST)
+struct S2 {
+  int x;
+  int y;
+};
+#elif defined(SECOND)
+struct S2 {
+  int y;
+  int x;
+};
+#else
+struct S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'y'}}
+#endif
+
+#if defined(FIRST)
+struct S3 {
+  double x;
+};
+#elif defined(SECOND)
+struct S3 {
+  int x;
+};
+#else
+struct S3 s3;
+// expected-error@second.h:* {{'S3::x' from module 'SecondModule' is not present in definition of 'struct S3' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+typedef int A;
+struct S4 {
+  A x;
+};
+
+struct S5 {
+  A x;
+};
+#elif defined(SECOND)
+typedef int B;
+struct S4 {
+  B x;
+};
+
+struct S5 {
+  int x;
+};
+#else
+struct S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'B' (aka 'int')}}
+
+struct S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'int'}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+  unsigned x;
+};
+#elif defined(SECOND)
+struct S6 {
+  unsigned x : 1;
+};
+#else
+struct S6 s6;
+// expected-error@first.h:* {{'S6' has different definitions in different modules; first difference is definition in module 'FirstModule' found non-bitfield 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S7 {
+  unsigned x : 1;
+};
+#else
+struct S7 s7;
+// expected-error@first.h:* {{'S7' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S8 {
+  unsigned x : 1 + 1;
+};
+#else
+struct S8 s8;
+// expected-error@first.h:* {{'S8' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S12 {
+  unsigned x[5];
+};
+#elif defined(SECOND)
+struct S12 {
+  unsigned x[7];
+};
+#else
+struct S12 s12;
+// expected-error@second.h:* {{'S12::x' from module 'SecondModule' is not present in definition of 'struct S12' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+struct S13 {
+  unsigned x[7];
+};
+#elif defined(SECOND)
+struct S13 {
+  double x[7];
+};
+#else
+struct S13 s13;
+// expected-error@second.h:* {{'S13::x' from module 'SecondModule' is not present in definition of 'struct S13' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+struct B1 {};
+struct SS1 {
+  struct B1 x;
+};
+#elif defined(SECOND)
+struct A1 {};
+struct SS1 {
+  struct A1 x;
+};
+#else
+struct SS1 ss1;
+// expected-error@second.h:* {{'SS1::x' from module 'SecondModule' is not present in definition of 'struct SS1' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+enum E1 { x42 };
+struct SE1 {
+  enum E1 x;
+};
+#elif defined(SECOND)
+enum E2 { x42 };
+struct SE1 {
+  enum E2 x;
+};
+#else
+struct SE1 se1;
+// expected-error@second.h:* {{'SE1::x' from module 'SecondModule' is not present in definition of 'struct SE1' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+// struct with forward declaration
+#if defined(FIRST)
+struct P {};
+struct S {
+  struct P *ptr;
+};
+#elif defined(SECOND)
+struct S {
+  struct P *ptr;
+};
+#else
+struct S s;
+#endif
+
+// struct with forward declaration and no definition
+#if defined(FIRST)
+struct PA;
+struct SA {
+  struct PA *ptr;
+};
+#elif defined(SECOND)
+struct SA {
+  struct PA *ptr;
+};
+#else
+struct SA sa;
+#endif
+
+// struct with multiple typedefs
+#if defined(FIRST)
+typedef int BB1;
+typedef BB1 AA1;
+struct TS1 {
+  AA1 x;
+};
+#elif defined(SECOND)
+typedef int AA1;
+struct TS1 {
+  AA1 x;
+};
+#else
+struct TS1 ts1;
+#endif
+
+#if defined(FIRST)
+struct T2 {
+  int x;
+};
+typedef struct T2 B2;
+typedef struct B2 A2;
+struct TS2 {
+  struct T2 x;
+};
+#elif defined(SECOND)
+struct T2 {
+  int x;
+};
+typedef struct T2 A2;
+struct TS2 {
+  struct T2 x;
+};
+#else
+struct TS2 ts2;
+#endif
+
+#if defined(FIRST)
+struct T3;
+struct TS3 {
+  struct T3 *t;
+};
+#elif defined(SECOND)
+typedef struct T3 {
+} T3;
+struct TS3 {
+  struct T3 *t;
+};
+#else
+struct TS3 ts3;
+#endif
+
+#if defined(FIRST)
+struct AU {
+  union {
+    int a;
+  };
+};
+#elif defined(SECOND)
+struct AU {
+  union {
+    char a;
+  };
+};
+#else
+struct AU au;
+// expected-error@first.h:* {{'AU' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'a' with type 'int'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'a' with type 'char'}}
+#endif
+
+#if defined(FIRST)
+struct AUS {
+  union {
+    int a;
+  };
+  struct {
+    char b;
+  };
+};
+#elif defined(SECOND)
+struct AUS {
+  union {
+    int a;
+  };
+  struct {
+    int b;
+  };
+};
+#else
+struct AUS aus;
+// expected-error@first.h:* {{'AUS' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'b' with type 'char'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'b' with type 'int'}}
+#endif
+
+#if defined(FIRST)
+struct AUS1 {
+  union {
+    int x;
+    union {
+      int y;
+      struct {
+        int z;
+      };
+    };
+  };
+};
+#elif defined(SECOND)
+struct AUS1 {
+  union {
+    int x;
+    union {
+      int y;
+      struct {
+        char z;
+      };
+    };
+  };
+};
+#else
+struct AUS1 aus1;
+// expected-error@first.h:* {{'AUS1' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'z' with type 'int'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'z' with type 'char'}}
+#endif
+
+#if defined(FIRST)
+union U {
+  int a;
+  char b;
+};
+#elif defined(SECOND)
+union U {
+  int a;
+  float b;
+};
+#else
+union U u;
+// expected-error@second.h:* {{'U::b' from module 'SecondModule' is not present in definition of 'union U' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'b' does not match}}
+#endif
+
+#if defined(FIRST)
+struct TSS1 {
+  int tss1;
+};
+#elif defined(SECOND)
+typedef struct TSS1 TSS1;
+#else
+struct TSS1 *tss1;
+#endif
+
+// Keep macros contained to one file.
+#ifdef FIRST
+#undef FIRST
+#endif
+
+#ifdef SECOND
+#undef SECOND
+#endif