[Modules] Implement ODR-like semantics for tag types in C/ObjC

[bruno]

Summary

Allow ODR for ObjC/C in the sense that we won't keep more that one definition around (merge them). However, ensure the decl pass the structural compatibility check in C11 6.2.7/1, for that, reuse the structural equivalence checks used by the ASTImporter.

Few other considerations:

• Create error diagnostics for tag types mismatches and thread them into the structural equivalence checks.
• Note that by doing this we only support redefinition between types that are considered "compatible types" by C11.

This is mixed approach of the suggestions discussed in http://lists.llvm.org/pipermail/cfe-dev/2017-March/053257.html

Depends on D31777

[aprantl]

Thanks!
Added a couple of superficial coding style comments.

I think there should also be a test for C99 and one for C11 (and potentially ObjC2).

include/clang/AST/ASTStructuralEquivalence.h

52 ↗

(On Diff #94400)

No need to use \brief in new code any more. We are compiling with the Doxygen autobrief option.

lib/Parse/ParseDecl.cpp

4316 ↗

(On Diff #94400)

I think the LLVM coding style wants this to be

{
    // Bail out.
    DS.SetTypeSpecError();
    return;
}

or

// Bail out.
return DS.SetTypeSpecError();

lib/Parse/ParseDeclCXX.cpp

1913 ↗

(On Diff #94400)

.

lib/Sema/SemaDecl.cpp

13386 ↗

(On Diff #94400)

Does ObjC2 imply ObjC1?

lib/Sema/SemaType.cpp

7085 ↗

(On Diff #94400)

Comment should go on the declaration.

[bruno]

include/clang/AST/ASTStructuralEquivalence.h

52 ↗

(On Diff #94400)

Sure, updated D31777 to also remove the \briefs in the transition

lib/Parse/ParseDecl.cpp

4316 ↗

(On Diff #94400)

OK

lib/Sema/SemaDecl.cpp

13386 ↗

(On Diff #94400)

Yep!

[bruno]

Update patch on top of Adrian comments

[bruno]

@rsmith ping!

[bruno]

Ping!

[rsmith]

include/clang/Sema/Sema.h

1464 ↗

(On Diff #94471)

Typo 'abd'

lib/Parse/ParseDecl.cpp

4236–4237 ↗

(On Diff #94471)

Do we really need both of these? The new stuff seems to be a natural extension of SkipBodyInfo, to say "parse it, check it's the same as the previous definition, then throw the new one away".

lib/Sema/SemaDecl.cpp

12929 ↗

(On Diff #94471)

No void here, please.

12962 ↗

(On Diff #94471)

"act on tag decl" -> ActOnTag?

13383–13384 ↗

(On Diff #94471)

Plurality mismatch in "decl pass"

lib/Sema/SemaExpr.cpp

2198–2209 ↗

(On Diff #94471)

This is gross. In order to make this work, you'd need to propagate the IgnorePrevDef flag through the entire expression parsing machinery.

Instead of this, how about deferring making the old definition visible until after you complete parsing the new one and do the structural comparison?

[bruno]

lib/Parse/ParseDecl.cpp

4236–4237 ↗

(On Diff #94471)

No specific reason. I'll augment SkipBodyInfo with two new fields then (a) a flag for the check and (b) the new tag.

lib/Sema/SemaExpr.cpp

2198–2209 ↗

(On Diff #94471)

Thanks for pointing it out, I totally missed this approach. Your suggestion works and I'll change the patch accordingly. However, IgnorePrevDef still needs to be threaded in ParseEnumBody and ActOnEnumConstant in order to prevent the latter to emit err_redefinition_of_enumerator-like diagnostics.

[bruno]

Update the patch after Richard's suggestions

[bruno]

@rsmith ping!

[rsmith]

include/clang/Sema/Sema.h

1464 ↗

(On Diff #94471)

Typo 'compatibale' =)

lib/Parse/ParseDecl.cpp

4307 ↗

(On Diff #96080)

Commented-out code, please remove.

4313–4321 ↗

(On Diff #96080)

The parser shouldn't be doing this itself, please move this to a function on Sema (ActOnDuplicateDefinition or something) and call that from here.

lib/Sema/SemaExpr.cpp

2198–2209 ↗

(On Diff #94471)

I think the problem is that we don't take module visibility into account when doing redefinition checking for enumerators. Instead of passing through this flag, we should probably just ignore hidden declarations when checking for a redefinition of an enumerator.

[bruno]

I think the problem is that we don't take module visibility into account when doing redefinition checking for enumerators. Instead of passing through this flag, we should probably just ignore hidden declarations when checking for a redefinition of an enumerator.

Right, now that the patch changed to only merge the definition after the structural check it does makes sense to do that.

[bruno]

Update the patch to address @rsmith comments and rebase

[rsmith]

include/clang/Parse/Parser.h

1956–1957 ↗

(On Diff #99585)

Please remove this (now unused) parameter.

lib/Parse/ParseDecl.cpp

4391 ↗

(On Diff #99585)

Please revert this no-op change (left over from when you were passing in IgnorePrevDef).

lib/Sema/SemaDecl.cpp

15423 ↗

(On Diff #99585)

The isFromASTFile check here is not correct: whether a declaration is from an AST file is independent of whether it might be not visible due to being in a module (local submodule visibility rules allow a local declaration to not be visible). The Modules check is also not correct: we do visibility checking for local #includes of module headers under -fmodules-local-submodule-visibility -fno-modules.

We also shouldn't diagnose ambiguity if we find two different hidden previous declarations, but we will do so (within LookupSingleName) with this approach.

Digging into this a bit more, I think the root of the problem is that the ND->isExternallyVisible() call in LookupResult::isHiddenDeclarationVisible is wrong. Redeclaration lookup should never find hidden enumerators in C, because they do not have linkage (C11 6.2.2/6). (The same is true in C++, but I don't know whether we can apply the same thing there too, due to the different merging rules.) A function foo in some source file should not conflict with an enumerator foo in a non-imported module, for instance.

[rsmith]

What's the status of this patch? https://reviews.llvm.org/D34510 is touching on a related issue, and we need to resolve how we're going to deal with ODR-like issues in C to determine how to address the issue that it is tackling.

[bruno]

Digging into this a bit more, I think the root of the problem is that the ND->isExternallyVisible() call in LookupResult::isHiddenDeclarationVisible is wrong. Redeclaration lookup should never find hidden enumerators in C, because they do not have linkage (C11 6.2.2/6). (The same is true in C++, but I don't know whether we can apply the same thing there too, due to the different

merging rules.) A function foo in some source file should not conflict with an enumerator foo in a non-imported module, for instance.

The linkage of an enumerator should probably be VisibleNoLinkage, and isHiddenDeclarationVisible should probably be checking hasExternalFormalLinkage...

Implemented this suggestions from last round of review. Change didn't seem to have side effects (at least from clang tests). @rsmith, what do you think? (I should probably split this small change into a different commit when the patch is accepted)

[rsmith]

lib/Sema/SemaDecl.cpp

13693–13694 ↗

(On Diff #104881)

I don't think this should require C11 -- in C89 the corresponding rule is in 6.1.2.6/1, so the rule exists in all languages that Clang supports.

cfe/trunk/include/clang/AST/ASTStructuralEquivalence.h

struct StructuralEquivalenceContext {
   bool Complain;
 
   /// \c true if the last diagnostic came from ToCtx.
   bool LastDiagFromC2;
 
   StructuralEquivalenceContext(
       ASTContext &FromCtx, ASTContext &ToCtx,
       llvm::DenseSet<std::pair<Decl *, Decl *>> &NonEquivalentDecls,
-      bool StrictTypeSpelling = false, bool Complain = true)
+      bool StrictTypeSpelling = false, bool Complain = true,
+      bool ErrorOnTagTypeMismatch = false)
       : FromCtx(FromCtx), ToCtx(ToCtx), NonEquivalentDecls(NonEquivalentDecls),
-        StrictTypeSpelling(StrictTypeSpelling), Complain(Complain),
+        StrictTypeSpelling(StrictTypeSpelling),
+        ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Complain(Complain),
         LastDiagFromC2(false) {}
 
   DiagnosticBuilder Diag1(SourceLocation Loc, unsigned DiagID);
   DiagnosticBuilder Diag2(SourceLocation Loc, unsigned DiagID);
 
   /// Determine whether the two declarations are structurally
   /// equivalent.
   bool IsStructurallyEquivalent(Decl *D1, Decl *D2);

cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td

def err_odr_variable_type_inconsistent : Error<
   "translation units (%1 vs. %2)">;
 def err_odr_variable_multiple_def : Error<
   "external variable %0 defined in multiple translation units">;
 def note_odr_value_here : Note<"declared here with type %0">;
 def note_odr_defined_here : Note<"also defined here">;
 def err_odr_function_type_inconsistent : Error<
   "external function %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
-def warn_odr_tag_type_inconsistent : Warning<
-  "type %0 has incompatible definitions in different translation units">,
+def warn_odr_tag_type_inconsistent
+    : Warning<"type %0 has incompatible definitions in different translation "
+              "units">,
       InGroup<DiagGroup<"odr">>;
+def err_odr_tag_type_inconsistent
+    : Error<"type %0 has incompatible definitions in different translation "
+            "units">;
 def note_odr_tag_kind_here: Note<
   "%0 is a %select{struct|interface|union|class|enum}1 here">;
 def note_odr_field : Note<"field %0 has type %1 here">;
+def note_odr_field_name : Note<"field has name %0 here">;
 def note_odr_missing_field : Note<"no corresponding field here">;
 def note_odr_bit_field : Note<"bit-field %0 with type %1 and length %2 here">;
 def note_odr_not_bit_field : Note<"field %0 is not a bit-field">;
 def note_odr_base : Note<"class has base type %0">;
 def note_odr_virtual_base : Note<
   "%select{non-virtual|virtual}0 derivation here">;
 def note_odr_missing_base : Note<"no corresponding base class here">;
 def note_odr_number_of_bases : Note<

cfe/trunk/include/clang/Sema/Sema.h

hasVisibleDeclaration(const NamedDecl *D,
                         llvm::SmallVectorImpl<Module *> *Modules = nullptr) {
     return isVisible(D) || hasVisibleDeclarationSlow(D, Modules);
   }
   bool hasVisibleDeclarationSlow(const NamedDecl *D,
                                  llvm::SmallVectorImpl<Module *> *Modules);
 
   bool hasVisibleMergedDefinition(NamedDecl *Def);
 
+  /// Determine if \p D and \p Suggested have a structurally compatible
+  /// layout as described in C11 6.2.7/1.
+  bool hasStructuralCompatLayout(Decl *D, Decl *Suggested);
+
   /// Determine if \p D has a visible definition. If not, suggest a declaration
   /// that should be made visible to expose the definition.
   bool hasVisibleDefinition(NamedDecl *D, NamedDecl **Suggested,
                             bool OnlyNeedComplete = false);
   bool hasVisibleDefinition(const NamedDecl *D) {
     NamedDecl *Hidden;
     return hasVisibleDefinition(const_cast<NamedDecl*>(D), &Hidden);
   }
QualType BuildUnaryTransformType(QualType BaseType,
                                    UnaryTransformType::UTTKind UKind,
                                    SourceLocation Loc);
 
   //===--------------------------------------------------------------------===//
   // Symbol table / Decl tracking callbacks: SemaDecl.cpp.
   //
 
   struct SkipBodyInfo {
-    SkipBodyInfo() : ShouldSkip(false), Previous(nullptr) {}
+    SkipBodyInfo()
+        : ShouldSkip(false), CheckSameAsPrevious(false), Previous(nullptr),
+          New(nullptr) {}
     bool ShouldSkip;
+    bool CheckSameAsPrevious;
     NamedDecl *Previous;
+    NamedDecl *New;
   };
 
   DeclGroupPtrTy ConvertDeclToDeclGroup(Decl *Ptr, Decl *OwnedType = nullptr);
 
   void DiagnoseUseOfUnimplementedSelectors();
 
   bool isSimpleTypeSpecifier(tok::TokenKind Kind) const;
 
public:
   enum TagUseKind {
     TUK_Reference,   // Reference to a tag:  'struct foo *X;'
     TUK_Declaration, // Fwd decl of a tag:   'struct foo;'
     TUK_Definition,  // Definition of a tag: 'struct foo { int X; } Y;'
     TUK_Friend       // Friend declaration:  'friend struct foo;'
   };
 
   Decl *ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
-                 SourceLocation KWLoc, CXXScopeSpec &SS,
-                 IdentifierInfo *Name, SourceLocation NameLoc,
-                 AttributeList *Attr, AccessSpecifier AS,
-                 SourceLocation ModulePrivateLoc,
-                 MultiTemplateParamsArg TemplateParameterLists,
-                 bool &OwnedDecl, bool &IsDependent,
-                 SourceLocation ScopedEnumKWLoc,
+                 SourceLocation KWLoc, CXXScopeSpec &SS, IdentifierInfo *Name,
+                 SourceLocation NameLoc, AttributeList *Attr,
+                 AccessSpecifier AS, SourceLocation ModulePrivateLoc,
+                 MultiTemplateParamsArg TemplateParameterLists, bool &OwnedDecl,
+                 bool &IsDependent, SourceLocation ScopedEnumKWLoc,
                  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
                  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
                  SkipBodyInfo *SkipBody = nullptr);
 
   Decl *ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
                                 unsigned TagSpec, SourceLocation TagLoc,
                                 CXXScopeSpec &SS,
                                 IdentifierInfo *Name, SourceLocation NameLoc,
void ActOnFields(Scope* S, SourceLocation RecLoc, Decl *TagDecl,
                    SourceLocation LBrac, SourceLocation RBrac,
                    AttributeList *AttrList);
 
   /// ActOnTagStartDefinition - Invoked when we have entered the
   /// scope of a tag's definition (e.g., for an enumeration, class,
   /// struct, or union).
   void ActOnTagStartDefinition(Scope *S, Decl *TagDecl);
 
+  /// Perform ODR-like check for C/ObjC when merging tag types from modules.
+  /// Differently from C++, actually parse the body and reject / error out
+  /// in case of a structural mismatch.
+  bool ActOnDuplicateDefinition(DeclSpec &DS, Decl *Prev,
+                                SkipBodyInfo &SkipBody);
+
   typedef void *SkippedDefinitionContext;
 
   /// \brief Invoked when we enter a tag definition that we're skipping.
   SkippedDefinitionContext ActOnTagStartSkippedDefinition(Scope *S, Decl *TD);
 
   Decl *ActOnObjCContainerStartDefinition(Decl *IDecl);
 
   /// ActOnStartCXXMemberDeclarations - Invoked when we have parsed a
public:
 
   /// Determine whether the body of an anonymous enumeration should be skipped.
   /// \param II The name of the first enumerator.
   SkipBodyInfo shouldSkipAnonEnumBody(Scope *S, IdentifierInfo *II,
                                       SourceLocation IILoc);
 
   Decl *ActOnEnumConstant(Scope *S, Decl *EnumDecl, Decl *LastEnumConstant,
                           SourceLocation IdLoc, IdentifierInfo *Id,
-                          AttributeList *Attrs,
-                          SourceLocation EqualLoc, Expr *Val);
+                          AttributeList *Attrs, SourceLocation EqualLoc,
+                          Expr *Val);
   void ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
                      Decl *EnumDecl,
                      ArrayRef<Decl *> Elements,
                      Scope *S, AttributeList *Attr);
 
   DeclContext *getContainingDC(DeclContext *DC);
 
   /// Set the current declaration context until it gets popped.

cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp

if (Field1->isAnonymousStructOrUnion() &&
     RecordDecl *D1 = Field1->getType()->castAs<RecordType>()->getDecl();
     RecordDecl *D2 = Field2->getType()->castAs<RecordType>()->getDecl();
     return IsStructurallyEquivalent(Context, D1, D2);
   }
 
   // Check for equivalent field names.
   IdentifierInfo *Name1 = Field1->getIdentifier();
   IdentifierInfo *Name2 = Field2->getIdentifier();
-  if (!::IsStructurallyEquivalent(Name1, Name2))
+  if (!::IsStructurallyEquivalent(Name1, Name2)) {
+    if (Context.Complain) {
+      Context.Diag2(Owner2->getLocation(),
+                    Context.ErrorOnTagTypeMismatch
+                        ? diag::err_odr_tag_type_inconsistent
+                        : diag::warn_odr_tag_type_inconsistent)
+          << Context.ToCtx.getTypeDeclType(Owner2);
+      Context.Diag2(Field2->getLocation(), diag::note_odr_field_name)
+          << Field2->getDeclName();
+      Context.Diag1(Field1->getLocation(), diag::note_odr_field_name)
+          << Field1->getDeclName();
+    }
     return false;
+  }
 
   if (!IsStructurallyEquivalent(Context, Field1->getType(),
                                 Field2->getType())) {
     if (Context.Complain) {
-      Context.Diag2(Owner2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+      Context.Diag2(Owner2->getLocation(),
+                    Context.ErrorOnTagTypeMismatch
+                        ? diag::err_odr_tag_type_inconsistent
+                        : diag::warn_odr_tag_type_inconsistent)
           << Context.ToCtx.getTypeDeclType(Owner2);
       Context.Diag2(Field2->getLocation(), diag::note_odr_field)
           << Field2->getDeclName() << Field2->getType();
       Context.Diag1(Field1->getLocation(), diag::note_odr_field)
           << Field1->getDeclName() << Field1->getType();
     }
     return false;
   }
 
   if (Field1->isBitField() != Field2->isBitField()) {
     if (Context.Complain) {
-      Context.Diag2(Owner2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+      Context.Diag2(Owner2->getLocation(),
+                    Context.ErrorOnTagTypeMismatch
+                        ? diag::err_odr_tag_type_inconsistent
+                        : diag::warn_odr_tag_type_inconsistent)
           << Context.ToCtx.getTypeDeclType(Owner2);
       if (Field1->isBitField()) {
         Context.Diag1(Field1->getLocation(), diag::note_odr_bit_field)
             << Field1->getDeclName() << Field1->getType()
             << Field1->getBitWidthValue(Context.FromCtx);
         Context.Diag2(Field2->getLocation(), diag::note_odr_not_bit_field)
             << Field2->getDeclName();
       } else {
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   if (Field1->isBitField()) {
     // Make sure that the bit-fields are the same length.
     unsigned Bits1 = Field1->getBitWidthValue(Context.FromCtx);
     unsigned Bits2 = Field2->getBitWidthValue(Context.ToCtx);
 
     if (Bits1 != Bits2) {
       if (Context.Complain) {
         Context.Diag2(Owner2->getLocation(),
-                      diag::warn_odr_tag_type_inconsistent)
+                      Context.ErrorOnTagTypeMismatch
+                          ? diag::err_odr_tag_type_inconsistent
+                          : diag::warn_odr_tag_type_inconsistent)
             << Context.ToCtx.getTypeDeclType(Owner2);
         Context.Diag2(Field2->getLocation(), diag::note_odr_bit_field)
             << Field2->getDeclName() << Field2->getType() << Bits2;
         Context.Diag1(Field1->getLocation(), diag::note_odr_bit_field)
             << Field1->getDeclName() << Field1->getType() << Bits1;
       }
       return false;
     }
   }
 
   return true;
 }
 
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      RecordDecl *D1, RecordDecl *D2) {
   if (D1->isUnion() != D2->isUnion()) {
     if (Context.Complain) {
-      Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+      Context.Diag2(D2->getLocation(),
+                    Context.ErrorOnTagTypeMismatch
+                        ? diag::err_odr_tag_type_inconsistent
+                        : diag::warn_odr_tag_type_inconsistent)
           << Context.ToCtx.getTypeDeclType(D2);
       Context.Diag1(D1->getLocation(), diag::note_odr_tag_kind_here)
           << D1->getDeclName() << (unsigned)D1->getTagKind();
     }
     return false;
   }
 
   if (D1->isAnonymousStructOrUnion() && D2->isAnonymousStructOrUnion()) {
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   // Check the fields for consistency.
   RecordDecl::field_iterator Field2 = D2->field_begin(),
                              Field2End = D2->field_end();
   for (RecordDecl::field_iterator Field1 = D1->field_begin(),
                                   Field1End = D1->field_end();
        Field1 != Field1End; ++Field1, ++Field2) {
     if (Field2 == Field2End) {
       if (Context.Complain) {
-        Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+        Context.Diag2(D2->getLocation(),
+                      Context.ErrorOnTagTypeMismatch
+                          ? diag::err_odr_tag_type_inconsistent
+                          : diag::warn_odr_tag_type_inconsistent)
             << Context.ToCtx.getTypeDeclType(D2);
         Context.Diag1(Field1->getLocation(), diag::note_odr_field)
             << Field1->getDeclName() << Field1->getType();
         Context.Diag2(D2->getLocation(), diag::note_odr_missing_field);
       }
       return false;
     }
 
     if (!IsStructurallyEquivalent(Context, *Field1, *Field2))
       return false;
   }
 
   if (Field2 != Field2End) {
     if (Context.Complain) {
-      Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+      Context.Diag2(D2->getLocation(),
+                    Context.ErrorOnTagTypeMismatch
+                        ? diag::err_odr_tag_type_inconsistent
+                        : diag::warn_odr_tag_type_inconsistent)
           << Context.ToCtx.getTypeDeclType(D2);
       Context.Diag2(Field2->getLocation(), diag::note_odr_field)
           << Field2->getDeclName() << Field2->getType();
       Context.Diag1(D1->getLocation(), diag::note_odr_missing_field);
     }
     return false;
   }
 
   return true;
 }
 
 /// Determine structural equivalence of two enums.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      EnumDecl *D1, EnumDecl *D2) {
   EnumDecl::enumerator_iterator EC2 = D2->enumerator_begin(),
                                 EC2End = D2->enumerator_end();
   for (EnumDecl::enumerator_iterator EC1 = D1->enumerator_begin(),
                                      EC1End = D1->enumerator_end();
        EC1 != EC1End; ++EC1, ++EC2) {
     if (EC2 == EC2End) {
       if (Context.Complain) {
-        Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+        Context.Diag2(D2->getLocation(),
+                      Context.ErrorOnTagTypeMismatch
+                          ? diag::err_odr_tag_type_inconsistent
+                          : diag::warn_odr_tag_type_inconsistent)
             << Context.ToCtx.getTypeDeclType(D2);
         Context.Diag1(EC1->getLocation(), diag::note_odr_enumerator)
             << EC1->getDeclName() << EC1->getInitVal().toString(10);
         Context.Diag2(D2->getLocation(), diag::note_odr_missing_enumerator);
       }
       return false;
     }
 
     llvm::APSInt Val1 = EC1->getInitVal();
     llvm::APSInt Val2 = EC2->getInitVal();
     if (!llvm::APSInt::isSameValue(Val1, Val2) ||
         !IsStructurallyEquivalent(EC1->getIdentifier(), EC2->getIdentifier())) {
       if (Context.Complain) {
-        Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+        Context.Diag2(D2->getLocation(),
+                      Context.ErrorOnTagTypeMismatch
+                          ? diag::err_odr_tag_type_inconsistent
+                          : diag::warn_odr_tag_type_inconsistent)
             << Context.ToCtx.getTypeDeclType(D2);
         Context.Diag2(EC2->getLocation(), diag::note_odr_enumerator)
             << EC2->getDeclName() << EC2->getInitVal().toString(10);
         Context.Diag1(EC1->getLocation(), diag::note_odr_enumerator)
             << EC1->getDeclName() << EC1->getInitVal().toString(10);
       }
       return false;
     }
   }
 
   if (EC2 != EC2End) {
     if (Context.Complain) {
-      Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+      Context.Diag2(D2->getLocation(),
+                    Context.ErrorOnTagTypeMismatch
+                        ? diag::err_odr_tag_type_inconsistent
+                        : diag::warn_odr_tag_type_inconsistent)
           << Context.ToCtx.getTypeDeclType(D2);
       Context.Diag2(EC2->getLocation(), diag::note_odr_enumerator)
           << EC2->getDeclName() << EC2->getInitVal().toString(10);
       Context.Diag1(D1->getLocation(), diag::note_odr_missing_enumerator);
     }
     return false;
   }
 

cfe/trunk/lib/Parse/ParseDecl.cpp

if (Tok.is(tok::l_brace) && TUK != Sema::TUK_Reference) {
       ConsumeBrace();
       SkipUntil(tok::r_brace, StopAtSemi);
     }
 
     DS.SetTypeSpecError();
     return;
   }
 
-  if (Tok.is(tok::l_brace) && TUK != Sema::TUK_Reference)
-    ParseEnumBody(StartLoc, TagDecl);
+  if (Tok.is(tok::l_brace) && TUK != Sema::TUK_Reference) {
+    Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
+    ParseEnumBody(StartLoc, D);
+    if (SkipBody.CheckSameAsPrevious &&
+        !Actions.ActOnDuplicateDefinition(DS, TagDecl, SkipBody)) {
+      DS.SetTypeSpecError();
+      return;
+    }
+  }
 
   if (DS.SetTypeSpecType(DeclSpec::TST_enum, StartLoc,
                          NameLoc.isValid() ? NameLoc : StartLoc,
                          PrevSpec, DiagID, TagDecl, Owned,
                          Actions.getASTContext().getPrintingPolicy()))
     Diag(StartLoc, DiagID) << PrevSpec;
 }
 
while (Tok.isNot(tok::r_brace)) {
 
     if (TryConsumeToken(tok::equal, EqualLoc)) {
       AssignedVal = ParseConstantExpression();
       if (AssignedVal.isInvalid())
         SkipUntil(tok::comma, tok::r_brace, StopBeforeMatch);
     }
 
     // Install the enumerator constant into EnumDecl.
-    Decl *EnumConstDecl = Actions.ActOnEnumConstant(getCurScope(), EnumDecl,
-                                                    LastEnumConstDecl,
-                                                    IdentLoc, Ident,
-                                                    attrs.getList(), EqualLoc,
-                                                    AssignedVal.get());
+    Decl *EnumConstDecl = Actions.ActOnEnumConstant(
+        getCurScope(), EnumDecl, LastEnumConstDecl, IdentLoc, Ident,
+        attrs.getList(), EqualLoc, AssignedVal.get());
     EnumAvailabilityDiags.back().done();
 
     EnumConstantDecls.push_back(EnumConstDecl);
     LastEnumConstDecl = EnumConstDecl;
 
     if (Tok.is(tok::identifier)) {
       // We're missing a comma between enumerators.
       SourceLocation Loc = getEndOfPreviousToken();

cfe/trunk/lib/Parse/ParseDeclCXX.cpp

assert(Tok.is(tok::l_brace) ||
            (getLangOpts().CPlusPlus && Tok.is(tok::colon)) ||
            isCXX11FinalKeyword());
     if (SkipBody.ShouldSkip)
       SkipCXXMemberSpecification(StartLoc, AttrFixitLoc, TagType,
                                  TagOrTempResult.get());
     else if (getLangOpts().CPlusPlus)
       ParseCXXMemberSpecification(StartLoc, AttrFixitLoc, attrs, TagType,
                                   TagOrTempResult.get());
-    else
-      ParseStructUnionBody(StartLoc, TagType, TagOrTempResult.get());
+    else {
+      Decl *D =
+          SkipBody.CheckSameAsPrevious ? SkipBody.New : TagOrTempResult.get();
+      // Parse the definition body.
+      ParseStructUnionBody(StartLoc, TagType, D);
+      if (SkipBody.CheckSameAsPrevious &&
+          !Actions.ActOnDuplicateDefinition(DS, TagOrTempResult.get(),
+                                            SkipBody)) {
+        DS.SetTypeSpecError();
+        return;
+      }
+    }
   }
 
   if (!TagOrTempResult.isInvalid())
     // Delayed processing of attributes.
     Actions.ProcessDeclAttributeDelayed(TagOrTempResult.get(), attrs.getList());
 
   const char *PrevSpec = nullptr;
   unsigned DiagID;

cfe/trunk/lib/Sema/SemaDecl.cpp

Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
   DeclContext *DC = CurContext;
   bool isStdBadAlloc = false;
   bool isStdAlignValT = false;
 
   RedeclarationKind Redecl = ForRedeclaration;
   if (TUK == TUK_Friend || TUK == TUK_Reference)
     Redecl = NotForRedeclaration;
 
+  /// Create a new tag decl in C/ObjC. Since the ODR-like semantics for ObjC/C
+  /// implemented asks for structural equivalence checking, the returned decl
+  /// here is passed back to the parser, allowing the tag body to be parsed.
+  auto createTagFromNewDecl = [&]() -> TagDecl * {
+    assert(!getLangOpts().CPlusPlus && "not meant for C++ usage");
+    // If there is an identifier, use the location of the identifier as the
+    // location of the decl, otherwise use the location of the struct/union
+    // keyword.
+    SourceLocation Loc = NameLoc.isValid() ? NameLoc : KWLoc;
+    TagDecl *New = nullptr;
+
+    if (Kind == TTK_Enum) {
+      New = EnumDecl::Create(Context, SearchDC, KWLoc, Loc, Name, nullptr,
+                             ScopedEnum, ScopedEnumUsesClassTag,
+                             !EnumUnderlying.isNull());
+      // If this is an undefined enum, bail.
+      if (TUK != TUK_Definition && !Invalid)
+        return nullptr;
+      if (EnumUnderlying) {
+        EnumDecl *ED = cast<EnumDecl>(New);
+        if (TypeSourceInfo *TI = EnumUnderlying.dyn_cast<TypeSourceInfo *>())
+          ED->setIntegerTypeSourceInfo(TI);
+        else
+          ED->setIntegerType(QualType(EnumUnderlying.get<const Type *>(), 0));
+        ED->setPromotionType(ED->getIntegerType());
+      }
+    } else { // struct/union
+      New = RecordDecl::Create(Context, Kind, SearchDC, KWLoc, Loc, Name,
+                               nullptr);
+    }
+
+    if (RecordDecl *RD = dyn_cast<RecordDecl>(New)) {
+      // Add alignment attributes if necessary; these attributes are checked
+      // when the ASTContext lays out the structure.
+      //
+      // It is important for implementing the correct semantics that this
+      // happen here (in ActOnTag). The #pragma pack stack is
+      // maintained as a result of parser callbacks which can occur at
+      // many points during the parsing of a struct declaration (because
+      // the #pragma tokens are effectively skipped over during the
+      // parsing of the struct).
+      if (TUK == TUK_Definition) {
+        AddAlignmentAttributesForRecord(RD);
+        AddMsStructLayoutForRecord(RD);
+      }
+    }
+    return New;
+  };
+
   LookupResult Previous(*this, Name, NameLoc, LookupTagName, Redecl);
   if (Name && SS.isNotEmpty()) {
     // We have a nested-name tag ('struct foo::bar').
 
     // Check for invalid 'foo::'.
     if (SS.isInvalid()) {
       Name = nullptr;
       goto CreateNewDecl;
if (TagDecl *PrevTagDecl = dyn_cast<TagDecl>(PrevDecl)) {
                     RD->getTemplateSpecializationKind() !=
                     TSK_ExplicitSpecialization;
                 else if (EnumDecl *ED = dyn_cast<EnumDecl>(Def))
                   IsExplicitSpecializationAfterInstantiation =
                     ED->getTemplateSpecializationKind() !=
                     TSK_ExplicitSpecialization;
               }
 
+              // Note that clang allows ODR-like semantics for ObjC/C, i.e., do
+              // not keep more that one definition around (merge them). However,
+              // ensure the decl passes the structural compatibility check in
+              // C11 6.2.7/1 (or 6.1.2.6/1 in C89).
               NamedDecl *Hidden = nullptr;
-              if (SkipBody && getLangOpts().CPlusPlus &&
-                  !hasVisibleDefinition(Def, &Hidden)) {
+              if (SkipBody && !hasVisibleDefinition(Def, &Hidden)) {
                 // There is a definition of this tag, but it is not visible. We
                 // explicitly make use of C++'s one definition rule here, and
                 // assume that this definition is identical to the hidden one
                 // we already have. Make the existing definition visible and
                 // use it in place of this one.
+                if (!getLangOpts().CPlusPlus) {
+                  // Postpone making the old definition visible until after we
+                  // complete parsing the new one and do the structural
+                  // comparison.
+                  SkipBody->CheckSameAsPrevious = true;
+                  SkipBody->New = createTagFromNewDecl();
+                  SkipBody->Previous = Hidden;
+                } else {
                   SkipBody->ShouldSkip = true;
                   makeMergedDefinitionVisible(Hidden);
+                }
                 return Def;
               } else if (!IsExplicitSpecializationAfterInstantiation) {
                 // A redeclaration in function prototype scope in C isn't
                 // visible elsewhere, so merely issue a warning.
                 if (!getLangOpts().CPlusPlus && S->containedInPrototypeScope())
                   Diag(NameLoc, diag::warn_redefinition_in_param_list) << Name;
                 else
                   Diag(NameLoc, diag::err_redefinition) << Name;
else
       Invalid = true;
   }
 
   if (RecordDecl *RD = dyn_cast<RecordDecl>(New)) {
     // Add alignment attributes if necessary; these attributes are checked when
     // the ASTContext lays out the structure.
     //
     // It is important for implementing the correct semantics that this
-    // happen here (in act on tag decl). The #pragma pack stack is
+    // happen here (in ActOnTag). The #pragma pack stack is
     // maintained as a result of parser callbacks which can occur at
     // many points during the parsing of a struct declaration (because
     // the #pragma tokens are effectively skipped over during the
     // parsing of the struct).
     if (TUK == TUK_Definition) {
       AddAlignmentAttributesForRecord(RD);
       AddMsStructLayoutForRecord(RD);
     }
void Sema::ActOnTagStartDefinition(Scope *S, Decl *TagD) {
 
   ActOnDocumentableDecl(TagD);
 
   // If there's a #pragma GCC visibility in scope, set the visibility of this
   // record.
   AddPushedVisibilityAttribute(Tag);
 }
 
+bool Sema::ActOnDuplicateDefinition(DeclSpec &DS, Decl *Prev,
+                                    SkipBodyInfo &SkipBody) {
+  if (!hasStructuralCompatLayout(Prev, SkipBody.New))
+    return false;
+
+  // Make the previous decl visible.
+  makeMergedDefinitionVisible(SkipBody.Previous);
+  return true;
+}
+
 Decl *Sema::ActOnObjCContainerStartDefinition(Decl *IDecl) {
   assert(isa<ObjCContainerDecl>(IDecl) &&
          "ActOnObjCContainerStartDefinition - Not ObjCContainerDecl");
   DeclContext *OCD = cast<DeclContext>(IDecl);
   assert(getContainingDC(OCD) == CurContext &&
       "The next DeclContext should be lexically contained in the current one.");
   CurContext = OCD;
   return IDecl;
if (PrevDecl && PrevDecl->isTemplateParameter()) {
     PrevDecl = nullptr;
   }
 
   // C++ [class.mem]p15:
   // If T is the name of a class, then each of the following shall have a name
   // different from T:
   // - every enumerator of every member of class T that is an unscoped
   // enumerated type
-  if (!TheEnumDecl->isScoped())
+  if (getLangOpts().CPlusPlus && !TheEnumDecl->isScoped())
     DiagnoseClassNameShadow(TheEnumDecl->getDeclContext(),
                             DeclarationNameInfo(Id, IdLoc));
 
   EnumConstantDecl *New =
     CheckEnumConstant(TheEnumDecl, LastEnumConst, IdLoc, Id, Val);
   if (!New)
     return nullptr;
 

cfe/trunk/lib/Sema/SemaType.cpp

 //  This file implements type-related semantic analysis.
 //
 //===----------------------------------------------------------------------===//
 
 #include "TypeLocBuilder.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTMutationListener.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeLocVisitor.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/TargetInfo.h"
if (const TagType *Tag = T->getAs<TagType>()) {
     if (!Tag->getDecl()->isCompleteDefinitionRequired()) {
       Tag->getDecl()->setCompleteDefinitionRequired();
       Consumer.HandleTagDeclRequiredDefinition(Tag->getDecl());
     }
   }
   return false;
 }
 
+bool Sema::hasStructuralCompatLayout(Decl *D, Decl *Suggested) {
+  llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+  if (!Suggested)
+    return false;
+
+  // FIXME: Add a specific mode for C11 6.2.7/1 in StructuralEquivalenceContext
+  // and isolate from other C++ specific checks.
+  StructuralEquivalenceContext Ctx(
+      D->getASTContext(), Suggested->getASTContext(), NonEquivalentDecls,
+      false /*StrictTypeSpelling*/, true /*Complain*/,
+      true /*ErrorOnTagTypeMismatch*/);
+  return Ctx.IsStructurallyEquivalent(D, Suggested);
+}
+
 /// \brief Determine whether there is any declaration of \p D that was ever a
 ///        definition (perhaps before module merging) and is currently visible.
 /// \param D The definition of the entity.
 /// \param Suggested Filled in with the declaration that should be made visible
 ///        in order to provide a definition of this entity.
 /// \param OnlyNeedComplete If \c true, we only need the type to be complete,
 ///        not defined. This only matters for enums with a fixed underlying
 ///        type, since in all other cases, a type is complete if and only if it

cfe/trunk/test/Modules/Inputs/F.framework/Headers/F.h

+// F.h

cfe/trunk/test/Modules/Inputs/F.framework/Modules/module.modulemap

+framework module F [extern_c] [system] {
+  umbrella header "F.h"
+  module * {
+      export *
+  }
+  export *
+}

cfe/trunk/test/Modules/Inputs/F.framework/Modules/module.private.modulemap

+module F.Private [system] {
+  explicit module NS {
+      header "NS.h"
+      export *
+  }
+  export *
+}

cfe/trunk/test/Modules/Inputs/F.framework/PrivateHeaders/NS.h

+struct NS {
+  int a;
+  int b;
+};
+
+enum NSE {
+  FST = 22,
+  SND = 43,
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+  MinXOther = MinX,
+};

cfe/trunk/test/Modules/elaborated-type-specifier-from-hidden-module.m

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
 
 @import ElaboratedTypeStructs.Empty; // The structs are now hidden.
 struct S1 *x;
 struct S2 *y;
-// FIXME: compatible definition should not be an error.
-struct S2 { int x; }; // expected-error {{redefinition}}
+struct S2 { int x; };
 struct S3 *z;
 // Incompatible definition.
-struct S3 { float y; }; // expected-error {{redefinition}}
-// expected-note@elaborated-type-structs.h:* 2 {{previous definition is here}}
+struct S3 { float y; }; // expected-error {{has incompatible definitions}} // expected-note {{field has name}}
+// expected-note@Inputs/elaborated-type-structs.h:3 {{field has name}}
 
 @import ElaboratedTypeStructs.Structs;
 
 void useS1(struct S1 *x);
 void useS2(struct S2 *x);
 void useS2(struct S2 *x);

cfe/trunk/test/Modules/redefinition-c-tagtypes.m

+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -verify
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -DCHANGE_TAGS -verify
+#include "F/F.h"
+
+#ifndef CHANGE_TAGS
+// expected-no-diagnostics
+#endif
+
+struct NS {
+  int a;
+#ifndef CHANGE_TAGS
+  int b;
+#else
+  int c; // expected-note {{field has name 'c' here}}
+  // expected-error@redefinition-c-tagtypes.m:12 {{type 'struct NS' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:3 {{field has name 'b' here}}
+#endif
+};
+
+enum NSE {
+  FST = 22,
+#ifndef CHANGE_TAGS
+  SND = 43,
+#else
+  SND = 44, // expected-note {{enumerator 'SND' with value 44 here}}
+  // expected-error@redefinition-c-tagtypes.m:23 {{type 'enum NSE' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:8 {{enumerator 'SND' with value 43 here}}
+#endif
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+#ifndef CHANGE_TAGS
+  MinXOther = MinX,
+#else
+  MinXOther = TRD, // expected-note {{enumerator 'MinXOther' with value 55 here}}
+  // expected-error@redefinition-c-tagtypes.m:39 {{type 'enum NSMyEnum' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:18 {{enumerator 'MinXOther' with value 11 here}}
+#endif
+};

cfe/trunk/test/Modules/redefinition-same-header.m

 // RUN: rm -rf %t.tmp
 // RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SameHeader -fmodules \
 // RUN:   -fimplicit-module-maps -fmodules-cache-path=%t.tmp %s -verify
 
 // expected-error@Inputs/SameHeader/C.h:3 {{redefinition of 'c'}}
 // expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}C.h' included multiple times, additional include site in header from module 'X.B'}}
 // expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
 // expected-note-re@redefinition-same-header.m:20 {{'{{.*}}C.h' included multiple times, additional include site here}}
+// expected-warning@Inputs/SameHeader/C.h:9 {{typedef requires a name}}
 
-// expected-error@Inputs/SameHeader/C.h:5 {{redefinition of 'aaa'}}
-// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}C.h' included multiple times, additional include site in header from module 'X.B'}}
-// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
-// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}C.h' included multiple times, additional include site here}}
-
-// expected-error@Inputs/SameHeader/C.h:9 {{redefinition of 'fd_set'}}
-// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}C.h' included multiple times, additional include site in header from module 'X.B'}}
-// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
-// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}C.h' included multiple times, additional include site here}}
 #include "A.h" // maps to a modular
 #include "C.h" // textual include