[CodeGen][ObjC] Invalidate cached ObjC class layout information after parsing ObjC class implementations if new ivars are added to the interface (#126591)

The layout and the size of an ObjC interface can change after its
corresponding implementation is parsed when synthesized ivars or ivars
declared in categories are added to the interface's list of ivars. This
can cause clang to mis-compile if the optimization that emits fixed
offsets for ivars (see 923ddf6) uses an
ObjC class layout that is outdated and no longer reflects the current
state of the class.

For example, when compiling `constant-non-fragile-ivar-offset.m`, clang
emits 20 instead of 24 as the offset for `IntermediateClass2Property` as
the class layout for `SuperClass2`, which is created when the
implementation of IntermediateClass3 is parsed, is outdated when the
implementation of `IntermediateClass2` is parsed.

This commit invalidates the stale layout information of the class and
its subclasses if new ivars are added to the interface.

With this change, we can also stop using ObjC implementation decls as
the key to retrieve ObjC class layouts information as the layout
retrieved using the ObjC interface as the key will always be up to date.

rdar://139531391

Author: ahatanak

clang/include/clang/AST/ASTContext.h

@@ -287,8 +287,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
   /// This is lazily created.  This is intentionally not serialized.
   mutable llvm::DenseMap<const RecordDecl*, const ASTRecordLayout*>
     ASTRecordLayouts;
-  mutable llvm::DenseMap<const ObjCContainerDecl*, const ASTRecordLayout*>
-    ObjCLayouts;
+  mutable llvm::DenseMap<const ObjCInterfaceDecl *, const ASTRecordLayout *>
+      ObjCLayouts;
 
   /// A cache from types to size and alignment information.
   using TypeInfoMap = llvm::DenseMap<const Type *, struct TypeInfo>;
@@ -500,6 +500,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
   static constexpr unsigned GeneralTypesLog2InitSize = 9;
   static constexpr unsigned FunctionProtoTypesLog2InitSize = 12;
 
+  /// A mapping from an ObjC class to its subclasses.
+  llvm::DenseMap<const ObjCInterfaceDecl *,
+                 SmallVector<const ObjCInterfaceDecl *, 4>>
+      ObjCSubClasses;
+
   ASTContext &this_() { return *this; }
 
 public:
@@ -2671,13 +2676,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
   void DumpRecordLayout(const RecordDecl *RD, raw_ostream &OS,
                         bool Simple = false) const;
 
-  /// Get or compute information about the layout of the specified
-  /// Objective-C implementation.
-  ///
-  /// This may differ from the interface if synthesized ivars are present.
-  const ASTRecordLayout &
-  getASTObjCImplementationLayout(const ObjCImplementationDecl *D) const;
-
   /// Get our current best idea for the key function of the
   /// given record decl, or nullptr if there isn't one.
   ///
@@ -2716,7 +2714,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
 
   /// Get the offset of an ObjCIvarDecl in bits.
   uint64_t lookupFieldBitOffset(const ObjCInterfaceDecl *OID,
-                                const ObjCImplementationDecl *ID,
                                 const ObjCIvarDecl *Ivar) const;
 
   /// Find the 'this' offset for the member path in a pointer-to-member
@@ -3174,7 +3171,12 @@ class ASTContext : public RefCountedBase<ASTContext> {
       bool &CanUseFirst, bool &CanUseSecond,
       SmallVectorImpl<FunctionProtoType::ExtParameterInfo> &NewParamInfos);
 
-  void ResetObjCLayout(const ObjCContainerDecl *CD);
+  void ResetObjCLayout(const ObjCInterfaceDecl *D);
+
+  void addObjCSubClass(const ObjCInterfaceDecl *D,
+                       const ObjCInterfaceDecl *SubClass) {
+    ObjCSubClasses[D].push_back(SubClass);
+  }
 
   //===--------------------------------------------------------------------===//
   //                    Integer Predicates
@@ -3564,9 +3566,7 @@ OPT_LIST(V)
   friend class DeclarationNameTable;
   friend class DeclContext;
 
-  const ASTRecordLayout &
-  getObjCLayout(const ObjCInterfaceDecl *D,
-                const ObjCImplementationDecl *Impl) const;
+  const ASTRecordLayout &getObjCLayout(const ObjCInterfaceDecl *D) const;
 
   /// A set of deallocations that should be performed when the
   /// ASTContext is destroyed.

clang/lib/AST/ASTContext.cpp

@@ -948,9 +948,11 @@ void ASTContext::cleanup() {
 
   // ASTRecordLayout objects in ASTRecordLayouts must always be destroyed
   // because they can contain DenseMaps.
-  for (llvm::DenseMap<const ObjCContainerDecl*,
-       const ASTRecordLayout*>::iterator
-       I = ObjCLayouts.begin(), E = ObjCLayouts.end(); I != E; )
+  for (llvm::DenseMap<const ObjCInterfaceDecl *,
+                      const ASTRecordLayout *>::iterator
+           I = ObjCLayouts.begin(),
+           E = ObjCLayouts.end();
+       I != E;)
     // Increment in loop to prevent using deallocated memory.
     if (auto *R = const_cast<ASTRecordLayout *>((I++)->second))
       R->Destroy(*this);
@@ -3092,13 +3094,7 @@ TypeSourceInfo *ASTContext::getTrivialTypeSourceInfo(QualType T,
 
 const ASTRecordLayout &
 ASTContext::getASTObjCInterfaceLayout(const ObjCInterfaceDecl *D) const {
-  return getObjCLayout(D, nullptr);
-}
-
-const ASTRecordLayout &
-ASTContext::getASTObjCImplementationLayout(
-                                        const ObjCImplementationDecl *D) const {
-  return getObjCLayout(D->getClassInterface(), D);
+  return getObjCLayout(D);
 }
 
 static auto getCanonicalTemplateArguments(const ASTContext &C,
@@ -8916,8 +8912,7 @@ static void EncodeBitField(const ASTContext *Ctx, std::string& S,
     uint64_t Offset;
 
     if (const auto *IVD = dyn_cast<ObjCIvarDecl>(FD)) {
-      Offset = Ctx->lookupFieldBitOffset(IVD->getContainingInterface(), nullptr,
-                                         IVD);
+      Offset = Ctx->lookupFieldBitOffset(IVD->getContainingInterface(), IVD);
     } else {
       const RecordDecl *RD = FD->getParent();
       const ASTRecordLayout &RL = Ctx->getASTRecordLayout(RD);
@@ -11848,8 +11843,12 @@ bool ASTContext::mergeExtParameterInfo(
   return true;
 }
 
-void ASTContext::ResetObjCLayout(const ObjCContainerDecl *CD) {
-  ObjCLayouts[CD] = nullptr;
+void ASTContext::ResetObjCLayout(const ObjCInterfaceDecl *D) {
+  if (ObjCLayouts.count(D)) {
+    ObjCLayouts[D] = nullptr;
+    for (auto *SubClass : ObjCSubClasses[D])
+      ResetObjCLayout(SubClass);
+  }
 }
 
 /// mergeObjCGCQualifiers - This routine merges ObjC's GC attribute of 'LHS' and

clang/lib/AST/RecordLayoutBuilder.cpp

@@ -3481,22 +3481,10 @@ 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
-  // in here; it should never be necessary because that should be the lexical
-  // decl context for the ivar.
-
-  // If we know have an implementation (and the ivar is in it) then
-  // look up in the implementation layout.
-  const ASTRecordLayout *RL;
-  if (ID && declaresSameEntity(ID->getClassInterface(), Container))
-    RL = &getASTObjCImplementationLayout(ID);
-  else
-    RL = &getASTObjCInterfaceLayout(Container);
+  const ASTRecordLayout *RL = &getASTObjCInterfaceLayout(Container);
 
   // Compute field index.
   //
@@ -3522,8 +3510,7 @@ uint64_t ASTContext::lookupFieldBitOffset(const ObjCInterfaceDecl *OID,
 /// \param Impl - If given, also include the layout of the interface's
 /// implementation. This may differ by including synthesized ivars.
 const ASTRecordLayout &
-ASTContext::getObjCLayout(const ObjCInterfaceDecl *D,
-                          const ObjCImplementationDecl *Impl) const {
+ASTContext::getObjCLayout(const ObjCInterfaceDecl *D) const {
   // Retrieve the definition
   if (D->hasExternalLexicalStorage() && !D->getDefinition())
     getExternalSource()->CompleteType(const_cast<ObjCInterfaceDecl*>(D));
@@ -3532,22 +3519,9 @@ ASTContext::getObjCLayout(const ObjCInterfaceDecl *D,
          "Invalid interface decl!");
 
   // Look up this layout, if already laid out, return what we have.
-  const ObjCContainerDecl *Key =
-    Impl ? (const ObjCContainerDecl*) Impl : (const ObjCContainerDecl*) D;
-  if (const ASTRecordLayout *Entry = ObjCLayouts[Key])
+  if (const ASTRecordLayout *Entry = ObjCLayouts[D])
     return *Entry;
 
-  // Add in synthesized ivar count if laying out an implementation.
-  if (Impl) {
-    unsigned SynthCount = CountNonClassIvars(D);
-    // If there aren't any synthesized ivars then reuse the interface
-    // entry. Note we can't cache this because we simply free all
-    // entries later; however we shouldn't look up implementations
-    // frequently.
-    if (SynthCount == 0)
-      return getObjCLayout(D, nullptr);
-  }
-
   ItaniumRecordLayoutBuilder Builder(*this, /*EmptySubobjects=*/nullptr);
   Builder.Layout(D);
 
@@ -3557,7 +3531,7 @@ ASTContext::getObjCLayout(const ObjCInterfaceDecl *D,
       /*RequiredAlignment : used by MS-ABI)*/
       Builder.Alignment, Builder.getDataSize(), Builder.FieldOffsets);
 
-  ObjCLayouts[Key] = NewEntry;
+  ObjCLayouts[D] = NewEntry;
 
   return *NewEntry;
 }

clang/lib/CodeGen/CGObjCGNU.cpp

@@ -1826,9 +1826,11 @@ class CGObjCGNUstep2 : public CGObjCGNUstep {
       Context.getASTObjCInterfaceLayout(SuperClassDecl).getSize().getQuantity();
     // Instance size is negative for classes that have not yet had their ivar
     // layout calculated.
-    classFields.addInt(LongTy,
-      0 - (Context.getASTObjCImplementationLayout(OID).getSize().getQuantity() -
-      superInstanceSize));
+    classFields.addInt(
+        LongTy, 0 - (Context.getASTObjCInterfaceLayout(OID->getClassInterface())
+                         .getSize()
+                         .getQuantity() -
+                     superInstanceSize));
 
     if (classDecl->all_declared_ivar_begin() == nullptr)
       classFields.addNullPointer(PtrTy);
@@ -3639,8 +3641,9 @@ void CGObjCGNU::GenerateClass(const ObjCImplementationDecl *OID) {
   }
 
   // Get the size of instances.
-  int instanceSize =
-    Context.getASTObjCImplementationLayout(OID).getSize().getQuantity();
+  int instanceSize = Context.getASTObjCInterfaceLayout(OID->getClassInterface())
+                         .getSize()
+                         .getQuantity();
 
   // Collect information about instance variables.
   SmallVector<llvm::Constant*, 16> IvarNames;

clang/lib/CodeGen/CGObjCMac.cpp

@@ -3439,8 +3439,9 @@ void CGObjCMac::GenerateClass(const ObjCImplementationDecl *ID) {
   else if ((hasMRCWeak = hasMRCWeakIvars(CGM, ID)))
     Flags |= FragileABI_Class_HasMRCWeakIvars;
 
-  CharUnits Size =
-      CGM.getContext().getASTObjCImplementationLayout(ID).getSize();
+  CharUnits Size = CGM.getContext()
+                       .getASTObjCInterfaceLayout(ID->getClassInterface())
+                       .getSize();
 
   // FIXME: Set CXX-structors flag.
   if (ID->getClassInterface()->getVisibility() == HiddenVisibility)
@@ -6330,7 +6331,7 @@ void CGObjCNonFragileABIMac::GetClassSizeInfo(const ObjCImplementationDecl *OID,
                                               uint32_t &InstanceStart,
                                               uint32_t &InstanceSize) {
   const ASTRecordLayout &RL =
-      CGM.getContext().getASTObjCImplementationLayout(OID);
+      CGM.getContext().getASTObjCInterfaceLayout(OID->getClassInterface());
 
   // InstanceSize is really instance end.
   InstanceSize = RL.getDataSize().getQuantity();

clang/lib/CodeGen/CGObjCRuntime.cpp

@@ -31,24 +31,22 @@ using namespace CodeGen;
 uint64_t CGObjCRuntime::ComputeIvarBaseOffset(CodeGen::CodeGenModule &CGM,
                                               const ObjCInterfaceDecl *OID,
                                               const ObjCIvarDecl *Ivar) {
-  return CGM.getContext().lookupFieldBitOffset(OID, nullptr, Ivar) /
+  return CGM.getContext().lookupFieldBitOffset(OID, Ivar) /
          CGM.getContext().getCharWidth();
 }
 
 uint64_t CGObjCRuntime::ComputeIvarBaseOffset(CodeGen::CodeGenModule &CGM,
                                               const ObjCImplementationDecl *OID,
                                               const ObjCIvarDecl *Ivar) {
-  return CGM.getContext().lookupFieldBitOffset(OID->getClassInterface(), OID,
-                                               Ivar) /
+  return CGM.getContext().lookupFieldBitOffset(OID->getClassInterface(), Ivar) /
          CGM.getContext().getCharWidth();
 }
 
 unsigned CGObjCRuntime::ComputeBitfieldBitOffset(
     CodeGen::CodeGenModule &CGM,
     const ObjCInterfaceDecl *ID,
     const ObjCIvarDecl *Ivar) {
-  return CGM.getContext().lookupFieldBitOffset(ID, ID->getImplementation(),
-                                               Ivar);
+  return CGM.getContext().lookupFieldBitOffset(ID, Ivar);
 }
 
 LValue CGObjCRuntime::EmitValueForIvarAtOffset(CodeGen::CodeGenFunction &CGF,
@@ -86,7 +84,7 @@ LValue CGObjCRuntime::EmitValueForIvarAtOffset(CodeGen::CodeGenFunction &CGF,
   // non-synthesized ivars but we may be called for synthesized ivars.  However,
   // a synthesized ivar can never be a bit-field, so this is safe.
   uint64_t FieldBitOffset =
-      CGF.CGM.getContext().lookupFieldBitOffset(OID, nullptr, Ivar);
+      CGF.CGM.getContext().lookupFieldBitOffset(OID, Ivar);
   uint64_t BitOffset = FieldBitOffset % CGF.CGM.getContext().getCharWidth();
   uint64_t AlignmentBits = CGF.CGM.getTarget().getCharAlign();
   uint64_t BitFieldSize = Ivar->getBitWidthValue();

clang/lib/Sema/SemaDeclObjC.cpp

@@ -659,6 +659,7 @@ void SemaObjC::ActOnSuperClassOfClassInterface(
 
     IDecl->setSuperClass(SuperClassTInfo);
     IDecl->setEndOfDefinitionLoc(SuperClassTInfo->getTypeLoc().getEndLoc());
+    getASTContext().addObjCSubClass(IDecl->getSuperClass(), IDecl);
   }
 }
 
@@ -2129,6 +2130,12 @@ SemaObjC::ActOnFinishObjCImplementation(Decl *ObjCImpDecl,
 
   DeclsInGroup.push_back(ObjCImpDecl);
 
+  // Reset the cached layout if there are any ivars added to
+  // the implementation.
+  if (auto *ImplD = dyn_cast<ObjCImplementationDecl>(ObjCImpDecl))
+    if (!ImplD->ivar_empty())
+      getASTContext().ResetObjCLayout(ImplD->getClassInterface());
+
   return SemaRef.BuildDeclaratorGroup(DeclsInGroup);
 }
 

clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m

@@ -9,6 +9,9 @@
 // CHECK: @"OBJC_IVAR_$_SubClass.subClassIvar" = constant i64 56
 // CHECK: @"OBJC_IVAR_$_SubClass._subClassProperty" = hidden constant i64 64
 // CHECK: @"OBJC_IVAR_$_NotStaticLayout.not_static_layout_ivar" = hidden global i64 12
+// CHECK: @"OBJC_IVAR_$_SuperClass2._superClassProperty2" = hidden constant i64 20
+// CHECK: @"OBJC_IVAR_$_IntermediateClass2._IntermediateClass2Property" = hidden constant i64 24
+// CHECK: @"OBJC_IVAR_$_SubClass2._subClass2Property" = hidden constant i64 28
 
 @interface NSObject {
   int these, will, never, change, ever;
@@ -138,3 +141,47 @@ -(void)meth {
   // CHECK: load i64, ptr @"OBJC_IVAR_$_NotStaticLayout.not_static_layout_ivar
 }
 @end
+
+// CHECK: define internal i32 @"\01-[IntermediateClass2 IntermediateClass2Property]"(ptr noundef %[[SELF:.*]],
+// CHECK: %[[SELF_ADDR:.*]] = alloca ptr, align 8
+// CHECK: store ptr %[[SELF]], ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load ptr, ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %[[V0]], i64 24
+// CHECK: %[[LOAD:.*]] = load atomic i32, ptr %[[ADD_PTR]] unordered, align 4
+// CHECK: ret i32 %[[LOAD]]
+
+// CHECK: define internal i32 @"\01-[SubClass2 subClass2Property]"(ptr noundef %[[SELF:.*]],
+// CHECK: %[[SELF_ADDR:.*]] = alloca ptr, align 8
+// CHECK: store ptr %[[SELF]], ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[V0:.*]] = load ptr, ptr %[[SELF_ADDR]], align 8
+// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %[[V0]], i64 28
+// CHECK: %[[LOAD:.*]] = load atomic i32, ptr %[[ADD_PTR]] unordered, align 4
+// CHECK: ret i32 %[[LOAD]]
+
+@interface SuperClass2 : NSObject
+@property int superClassProperty2;
+@end
+
+@interface IntermediateClass2 : SuperClass2
+@property int IntermediateClass2Property;
+@end
+
+@interface IntermediateClass3 : SuperClass2
+@property int IntermediateClass3Property;
+@end
+
+@interface SubClass2 : IntermediateClass2
+@property int subClass2Property;
+@end
+
+@implementation IntermediateClass3
+@end
+
+@implementation SuperClass2
+@end
+
+@implementation IntermediateClass2
+@end
+
+@implementation SubClass2
+@end

clang/test/CodeGenObjC/ivar-layout-64.m

@@ -63,8 +63,8 @@ @interface D : A
 @end
 
 // CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"D\00"
-// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"\11p\00"
-// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"!`\00"
+// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"\11\A0\00"
+// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"!\90\00"
 
 @implementation D
 @synthesize p3 = _p3;