Cherry-pick ObjCInterfaceDecl miscompilation and related clean-up

clang/include/clang/AST/DeclObjC.h

@@ -1965,6 +1965,13 @@ class ObjCIvarDecl : public FieldDecl {
   const ObjCIvarDecl *getNextIvar() const { return NextIvar; }
   void setNextIvar(ObjCIvarDecl *ivar) { NextIvar = ivar; }
 
+  ObjCIvarDecl *getCanonicalDecl() override {
+    return cast<ObjCIvarDecl>(FieldDecl::getCanonicalDecl());
+  }
+  const ObjCIvarDecl *getCanonicalDecl() const {
+    return const_cast<ObjCIvarDecl *>(this)->getCanonicalDecl();
+  }
+
   void setAccessControl(AccessControl ac) { DeclAccess = ac; }
 
   AccessControl getAccessControl() const { return AccessControl(DeclAccess); }

clang/include/clang/AST/Type.h

@@ -6162,18 +6162,17 @@ inline ObjCProtocolDecl **ObjCTypeParamType::getProtocolStorageImpl() {
 class ObjCInterfaceType : public ObjCObjectType {
   friend class ASTContext; // ASTContext creates these.
   friend class ASTReader;
-  friend class ObjCInterfaceDecl;
   template <class T> friend class serialization::AbstractTypeReader;
 
-  mutable ObjCInterfaceDecl *Decl;
+  ObjCInterfaceDecl *Decl;
 
   ObjCInterfaceType(const ObjCInterfaceDecl *D)
       : ObjCObjectType(Nonce_ObjCInterface),
         Decl(const_cast<ObjCInterfaceDecl*>(D)) {}
 
 public:
   /// Get the declaration of this interface.
-  ObjCInterfaceDecl *getDecl() const { return Decl; }
+  ObjCInterfaceDecl *getDecl() const;
 
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }

clang/lib/AST/DeclObjC.cpp

@@ -603,10 +603,6 @@ void ObjCInterfaceDecl::allocateDefinitionData() {
   assert(!hasDefinition() && "ObjC class already has a definition");
   Data.setPointer(new (getASTContext()) DefinitionData());
   Data.getPointer()->Definition = this;
-
-  // Make the type point at the definition, now that we have one.
-  if (TypeForDecl)
-    cast<ObjCInterfaceType>(TypeForDecl)->Decl = this;
 }
 
 void ObjCInterfaceDecl::startDefinition() {

clang/lib/AST/RecordLayoutBuilder.cpp

@@ -3383,6 +3383,7 @@ uint64_t ASTContext::getFieldOffset(const ValueDecl *VD) const {
 uint64_t ASTContext::lookupFieldBitOffset(const ObjCInterfaceDecl *OID,
                                           const ObjCImplementationDecl *ID,
                                           const ObjCIvarDecl *Ivar) const {
+  Ivar = Ivar->getCanonicalDecl();
   const ObjCInterfaceDecl *Container = Ivar->getContainingInterface();
 
   // FIXME: We should eliminate the need to have ObjCImplementationDecl passed

clang/lib/AST/Type.cpp

@@ -821,6 +821,13 @@ QualType ObjCObjectType::stripObjCKindOfTypeAndQuals(
                                /*isKindOf=*/false);
 }
 
+ObjCInterfaceDecl *ObjCInterfaceType::getDecl() const {
+  ObjCInterfaceDecl *Canon = Decl->getCanonicalDecl();
+  if (ObjCInterfaceDecl *Def = Canon->getDefinition())
+    return Def;
+  return Canon;
+}
+
 const ObjCObjectPointerType *ObjCObjectPointerType::stripObjCKindOfTypeAndQuals(
                                const ASTContext &ctx) const {
   if (!isKindOfType() && qual_empty())

clang/lib/Sema/SemaLookup.cpp

@@ -5325,11 +5325,8 @@ static NamedDecl *getDefinitionToImport(NamedDecl *D) {
     return FD->getDefinition();
   if (TagDecl *TD = dyn_cast<TagDecl>(D))
     return TD->getDefinition();
-  // The first definition for this ObjCInterfaceDecl might be in the TU
-  // and not associated with any module. Use the one we know to be complete
-  // and have just seen in a module.
   if (ObjCInterfaceDecl *ID = dyn_cast<ObjCInterfaceDecl>(D))
-    return ID;
+    return ID->getDefinition();
   if (ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(D))
     return PD->getDefinition();
   if (TemplateDecl *TD = dyn_cast<TemplateDecl>(D))

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -1149,6 +1149,13 @@ void ASTDeclReader::ReadObjCDefinitionData(
 
 void ASTDeclReader::MergeDefinitionData(ObjCInterfaceDecl *D,
          struct ObjCInterfaceDecl::DefinitionData &&NewDD) {
+  struct ObjCInterfaceDecl::DefinitionData &DD = D->data();
+  if (DD.Definition != NewDD.Definition) {
+    Reader.MergedDeclContexts.insert(
+        std::make_pair(NewDD.Definition, DD.Definition));
+    Reader.mergeDefinitionVisibility(DD.Definition, NewDD.Definition);
+  }
+
   // FIXME: odr checking?
 }
 
@@ -2647,7 +2654,7 @@ static bool allowODRLikeMergeInC(NamedDecl *ND) {
   if (!ND)
     return false;
   // TODO: implement merge for other necessary decls.
-  if (isa<EnumConstantDecl>(ND))
+  if (isa<EnumConstantDecl, FieldDecl, IndirectFieldDecl>(ND))
     return true;
   return false;
 }
@@ -3323,6 +3330,9 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
     return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
                                                       : nullptr;
 
+  if (auto *OID = dyn_cast<ObjCInterfaceDecl>(DC))
+    return OID->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))

clang/test/Modules/decldef.mm

@@ -1,10 +1,12 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_1 -DUSE_2 -DUSE_3 -DUSE_4
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_2 -DUSE_3 -DUSE_4
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_3 -DUSE_4
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_4
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_1 -DUSE_2 -DUSE_3 -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_2 -DUSE_3 -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_3 -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_5
 
 // expected-note@Inputs/def.h:5 0-1{{here}}
+// expected-note@Inputs/def.h:11 0-1{{here}}
 // expected-note@Inputs/def.h:16 0-1{{here}}
 // expected-note@Inputs/def-include.h:11 0-1{{here}}
 
@@ -51,11 +53,17 @@ void testB() {
 }
 
 void testDef() {
+#ifdef USE_4
   [def defMethod];
+  #ifndef USED
+  // expected-error@-2{{definition of 'Def' must be imported from module 'decldef.Def' before it is required}}
+  #define USED
+  #endif
+#endif
 }
 
 void testDef2() {
-#ifdef USE_4
+#ifdef USE_5
   def2->func();
   def3->func();
   #ifndef USED

clang/test/Modules/interface-diagnose-missing-import.m

@@ -1,11 +1,11 @@
 // RUN: rm -rf %t
 // RUN: %clang_cc1 %s -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F%S/Inputs/interface-diagnose-missing-import -verify
+// expected-no-diagnostics
 @interface Buggy
 @end
 
 @import Foo.Bar;
 
-@interface Buggy (MyExt) // expected-error {{definition of 'Buggy' must be imported from module 'Foo' before it is required}}
+// No diagnostic for inaccessible 'Buggy' definition because we have another definition right in this file.
+@interface Buggy (MyExt)
 @end
-
-// expected-note@Foo/RandoPriv.h:3{{definition here is not reachable}}

clang/test/Modules/merge-objc-interface-visibility.m

@@ -0,0 +1,61 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%t/Frameworks %t/test.m -DHIDDEN_FIRST=1 \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%t/Frameworks %t/test.m -DHIDDEN_FIRST=0 \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test a case when Objective-C interface is imported both as hidden and as visible.
+
+//--- Frameworks/Foundation.framework/Headers/Foundation.h
+@interface NSObject
+@end
+
+//--- Frameworks/Foundation.framework/Modules/module.modulemap
+framework module Foundation {
+  header "Foundation.h"
+  export *
+}
+
+//--- Frameworks/Regular.framework/Headers/Regular.h
+#import <Foundation/Foundation.h>
+@interface Regular : NSObject
+@end
+
+//--- Frameworks/Regular.framework/Modules/module.modulemap
+framework module Regular {
+  header "Regular.h"
+  export *
+}
+
+//--- Frameworks/RegularHider.framework/Headers/Visible.h
+// Empty, file required to create a module.
+
+//--- Frameworks/RegularHider.framework/Headers/Hidden.h
+#import <Foundation/Foundation.h>
+@interface Regular : NSObject
+@end
+
+//--- Frameworks/RegularHider.framework/Modules/module.modulemap
+framework module RegularHider {
+  header "Visible.h"
+  export *
+
+  explicit module Hidden {
+    header "Hidden.h"
+    export *
+  }
+}
+
+//--- test.m
+
+#if HIDDEN_FIRST
+#import <RegularHider/Visible.h>
+#import <Regular/Regular.h>
+#else
+#import <Regular/Regular.h>
+#import <RegularHider/Visible.h>
+#endif
+
+@interface SubClass : Regular
+@end

clang/test/Modules/merge-objc-interface.m

@@ -0,0 +1,105 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%t/Frameworks %t/test.m \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -F%t/Frameworks %t/test-functions.m \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test a case when Objective-C interface ivars are present in two different modules.
+
+//--- Frameworks/Foundation.framework/Headers/Foundation.h
+@interface NSObject
+@end
+
+//--- Frameworks/Foundation.framework/Modules/module.modulemap
+framework module Foundation {
+  header "Foundation.h"
+  export *
+}
+
+//--- Frameworks/ObjCInterface.framework/Headers/ObjCInterface.h
+#import <Foundation/Foundation.h>
+@interface ObjCInterface : NSObject {
+@public
+  id _item;
+}
+@end
+
+@interface WithBitFields : NSObject {
+@public
+  int x: 3;
+  int y: 4;
+}
+@end
+
+//--- Frameworks/ObjCInterface.framework/Modules/module.modulemap
+framework module ObjCInterface {
+  header "ObjCInterface.h"
+  export *
+}
+
+//--- Frameworks/ObjCInterfaceCopy.framework/Headers/ObjCInterfaceCopy.h
+#import <Foundation/Foundation.h>
+@interface ObjCInterface : NSObject {
+@public
+  id _item;
+}
+@end
+
+@interface WithBitFields : NSObject {
+@public
+  int x: 3;
+  int y: 4;
+}
+@end
+
+// Inlined function present only in Copy.framework to make sure it uses decls from Copy module.
+__attribute__((always_inline)) void inlinedIVarAccessor(ObjCInterface *obj, WithBitFields *bitFields) {
+  obj->_item = 0;
+  bitFields->x = 0;
+}
+
+//--- Frameworks/ObjCInterfaceCopy.framework/Modules/module.modulemap
+framework module ObjCInterfaceCopy {
+  header "ObjCInterfaceCopy.h"
+  export *
+}
+
+//--- test.m
+#import <ObjCInterface/ObjCInterface.h>
+#import <ObjCInterfaceCopy/ObjCInterfaceCopy.h>
+
+@implementation ObjCInterface
+- (void)test:(id)item {
+  _item = item;
+}
+@end
+
+@implementation WithBitFields
+- (void)reset {
+  x = 0;
+  y = 0;
+}
+@end
+
+//--- test-functions.m
+#import <ObjCInterface/ObjCInterface.h>
+
+void testAccessIVar(ObjCInterface *obj, id item) {
+  obj->_item = item;
+}
+void testAccessBitField(WithBitFields *obj) {
+  obj->x = 0;
+}
+
+#import <ObjCInterfaceCopy/ObjCInterfaceCopy.h>
+
+void testAccessIVarLater(ObjCInterface *obj, id item) {
+  obj->_item = item;
+}
+void testAccessBitFieldLater(WithBitFields *obj) {
+  obj->y = 0;
+}
+void testInlinedFunction(ObjCInterface *obj, WithBitFields *bitFields) {
+  inlinedIVarAccessor(obj, bitFields);
+}

clang/test/Modules/odr_hash.mm

@@ -241,12 +241,12 @@ @interface Interface4 <T : I1 *> {
 @end
 @interface Interface5 <T : I1 *> {
 @public
-  T x; // FIXME: align with upstream (rdar://43906928).
+  T y; // FIXME: align with upstream (rdar://43906928).
 }
 @end
 @interface Interface6 <T1 : I1 *, T2 : I2 *> {
 @public
-  T1 x;
+  T1 z;
 }
 @end
 #elif defined(SECOND)
@@ -257,14 +257,17 @@ @interface Interface4 <T : I1 *> {
 @end
 @interface Interface5 <T : I1 *> {
 @public
-  T x; // FIXME: align with upstream (rdar://43906928).
+  T y; // FIXME: align with upstream (rdar://43906928).
 }
 @end
 @interface Interface6 <T1 : I1 *, T2 : I2 *> {
 @public
-  T2 x;
+  T2 z;
 }
 @end
+#else
+// [email protected]:* {{'Interface6::z' from module 'FirstModule' is not present in definition of 'Interface6' in module 'SecondModule'}}
+// [email protected]:* {{declaration of 'z' does not match}}
 #endif
 
 namespace Types {
@@ -276,18 +279,18 @@ @interface Interface6 <T1 : I1 *, T2 : I2 *> {
 };
 struct Invalid2 {
   Interface5 *I;
-  decltype(I->x) x;
+  decltype(I->y) y;
 };
 struct Invalid3 {
   Interface6 *I;
-  decltype(I->x) x;
+  decltype(I->z) z;
 };
 #else
 Invalid1 i1;
 Invalid2 i2;
 Invalid3 i3;
-// [email protected]:* {{'Types::ObjCTypeParam::Invalid3::x' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid3' in module 'SecondModule'}}
-// [email protected]:* {{declaration of 'x' does not match}}
+// [email protected]:* {{'Types::ObjCTypeParam::Invalid3::z' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid3' in module 'SecondModule'}}
+// [email protected]:* {{declaration of 'z' does not match}}
 #endif
 
 }  // namespace ObjCTypeParam