Skip to content

Commit f5c5bc5

Browse files
ahatanakahatanak
authored
[CodeGen][ObjC] Invalidate cached ObjC class layout information after parsing ObjC class implementations if new ivars are added to the interface (llvm#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
1 parent 83e180c commit f5c5bc5

File tree

9 files changed

+103
-74
lines changed

9 files changed

+103
-74
lines changed

‎clang/include/clang/AST/ASTContext.h

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -287,8 +287,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
287287
/// This is lazily created. This is intentionally not serialized.
288288
mutable llvm::DenseMap<const RecordDecl*, const ASTRecordLayout*>
289289
ASTRecordLayouts;
290-
mutable llvm::DenseMap<const ObjCContainerDecl*, const ASTRecordLayout*>
291-
ObjCLayouts;
290+
mutable llvm::DenseMap<const ObjCInterfaceDecl *, const ASTRecordLayout *>
291+
ObjCLayouts;
292292

293293
/// A cache from types to size and alignment information.
294294
using TypeInfoMap = llvm::DenseMap<const Type *, struct TypeInfo>;
@@ -500,6 +500,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
500500
static constexpr unsigned GeneralTypesLog2InitSize = 9;
501501
static constexpr unsigned FunctionProtoTypesLog2InitSize = 12;
502502

503+
/// A mapping from an ObjC class to its subclasses.
504+
llvm::DenseMap<const ObjCInterfaceDecl *,
505+
SmallVector<const ObjCInterfaceDecl *, 4>>
506+
ObjCSubClasses;
507+
503508
ASTContext &this_() { return *this; }
504509

505510
public:
@@ -2671,13 +2676,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
26712676
void DumpRecordLayout(const RecordDecl *RD, raw_ostream &OS,
26722677
bool Simple = false) const;
26732678

2674-
/// Get or compute information about the layout of the specified
2675-
/// Objective-C implementation.
2676-
///
2677-
/// This may differ from the interface if synthesized ivars are present.
2678-
const ASTRecordLayout &
2679-
getASTObjCImplementationLayout(const ObjCImplementationDecl *D) const;
2680-
26812679
/// Get our current best idea for the key function of the
26822680
/// given record decl, or nullptr if there isn't one.
26832681
///
@@ -2716,7 +2714,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
27162714

27172715
/// Get the offset of an ObjCIvarDecl in bits.
27182716
uint64_t lookupFieldBitOffset(const ObjCInterfaceDecl *OID,
2719-
const ObjCImplementationDecl *ID,
27202717
const ObjCIvarDecl *Ivar) const;
27212718

27222719
/// Find the 'this' offset for the member path in a pointer-to-member
@@ -3174,7 +3171,12 @@ class ASTContext : public RefCountedBase<ASTContext> {
31743171
bool &CanUseFirst, bool &CanUseSecond,
31753172
SmallVectorImpl<FunctionProtoType::ExtParameterInfo> &NewParamInfos);
31763173

3177-
void ResetObjCLayout(const ObjCContainerDecl *CD);
3174+
void ResetObjCLayout(const ObjCInterfaceDecl *D);
3175+
3176+
void addObjCSubClass(const ObjCInterfaceDecl *D,
3177+
const ObjCInterfaceDecl *SubClass) {
3178+
ObjCSubClasses[D].push_back(SubClass);
3179+
}
31783180

31793181
//===--------------------------------------------------------------------===//
31803182
// Integer Predicates
@@ -3564,9 +3566,7 @@ OPT_LIST(V)
35643566
friend class DeclarationNameTable;
35653567
friend class DeclContext;
35663568

3567-
const ASTRecordLayout &
3568-
getObjCLayout(const ObjCInterfaceDecl *D,
3569-
const ObjCImplementationDecl *Impl) const;
3569+
const ASTRecordLayout &getObjCLayout(const ObjCInterfaceDecl *D) const;
35703570

35713571
/// A set of deallocations that should be performed when the
35723572
/// ASTContext is destroyed.

‎clang/lib/AST/ASTContext.cpp

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -948,9 +948,11 @@ void ASTContext::cleanup() {
948948

949949
// ASTRecordLayout objects in ASTRecordLayouts must always be destroyed
950950
// because they can contain DenseMaps.
951-
for (llvm::DenseMap<const ObjCContainerDecl*,
952-
const ASTRecordLayout*>::iterator
953-
I = ObjCLayouts.begin(), E = ObjCLayouts.end(); I != E; )
951+
for (llvm::DenseMap<const ObjCInterfaceDecl *,
952+
const ASTRecordLayout *>::iterator
953+
I = ObjCLayouts.begin(),
954+
E = ObjCLayouts.end();
955+
I != E;)
954956
// Increment in loop to prevent using deallocated memory.
955957
if (auto *R = const_cast<ASTRecordLayout *>((I++)->second))
956958
R->Destroy(*this);
@@ -3092,13 +3094,7 @@ TypeSourceInfo *ASTContext::getTrivialTypeSourceInfo(QualType T,
30923094

30933095
const ASTRecordLayout &
30943096
ASTContext::getASTObjCInterfaceLayout(const ObjCInterfaceDecl *D) const {
3095-
return getObjCLayout(D, nullptr);
3096-
}
3097-
3098-
const ASTRecordLayout &
3099-
ASTContext::getASTObjCImplementationLayout(
3100-
const ObjCImplementationDecl *D) const {
3101-
return getObjCLayout(D->getClassInterface(), D);
3097+
return getObjCLayout(D);
31023098
}
31033099

31043100
static auto getCanonicalTemplateArguments(const ASTContext &C,
@@ -8916,8 +8912,7 @@ static void EncodeBitField(const ASTContext *Ctx, std::string& S,
89168912
uint64_t Offset;
89178913

89188914
if (const auto *IVD = dyn_cast<ObjCIvarDecl>(FD)) {
8919-
Offset = Ctx->lookupFieldBitOffset(IVD->getContainingInterface(), nullptr,
8920-
IVD);
8915+
Offset = Ctx->lookupFieldBitOffset(IVD->getContainingInterface(), IVD);
89218916
} else {
89228917
const RecordDecl *RD = FD->getParent();
89238918
const ASTRecordLayout &RL = Ctx->getASTRecordLayout(RD);
@@ -11848,8 +11843,12 @@ bool ASTContext::mergeExtParameterInfo(
1184811843
return true;
1184911844
}
1185011845

11851-
void ASTContext::ResetObjCLayout(const ObjCContainerDecl *CD) {
11852-
ObjCLayouts[CD] = nullptr;
11846+
void ASTContext::ResetObjCLayout(const ObjCInterfaceDecl *D) {
11847+
if (ObjCLayouts.count(D)) {
11848+
ObjCLayouts[D] = nullptr;
11849+
for (auto *SubClass : ObjCSubClasses[D])
11850+
ResetObjCLayout(SubClass);
11851+
}
1185311852
}
1185411853

1185511854
/// mergeObjCGCQualifiers - This routine merges ObjC's GC attribute of 'LHS' and

‎clang/lib/AST/RecordLayoutBuilder.cpp

Lines changed: 4 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3481,22 +3481,10 @@ uint64_t ASTContext::getFieldOffset(const ValueDecl *VD) const {
34813481
}
34823482

34833483
uint64_t ASTContext::lookupFieldBitOffset(const ObjCInterfaceDecl *OID,
3484-
const ObjCImplementationDecl *ID,
34853484
const ObjCIvarDecl *Ivar) const {
34863485
Ivar = Ivar->getCanonicalDecl();
34873486
const ObjCInterfaceDecl *Container = Ivar->getContainingInterface();
3488-
3489-
// FIXME: We should eliminate the need to have ObjCImplementationDecl passed
3490-
// in here; it should never be necessary because that should be the lexical
3491-
// decl context for the ivar.
3492-
3493-
// If we know have an implementation (and the ivar is in it) then
3494-
// look up in the implementation layout.
3495-
const ASTRecordLayout *RL;
3496-
if (ID && declaresSameEntity(ID->getClassInterface(), Container))
3497-
RL = &getASTObjCImplementationLayout(ID);
3498-
else
3499-
RL = &getASTObjCInterfaceLayout(Container);
3487+
const ASTRecordLayout *RL = &getASTObjCInterfaceLayout(Container);
35003488

35013489
// Compute field index.
35023490
//
@@ -3522,8 +3510,7 @@ uint64_t ASTContext::lookupFieldBitOffset(const ObjCInterfaceDecl *OID,
35223510
/// \param Impl - If given, also include the layout of the interface's
35233511
/// implementation. This may differ by including synthesized ivars.
35243512
const ASTRecordLayout &
3525-
ASTContext::getObjCLayout(const ObjCInterfaceDecl *D,
3526-
const ObjCImplementationDecl *Impl) const {
3513+
ASTContext::getObjCLayout(const ObjCInterfaceDecl *D) const {
35273514
// Retrieve the definition
35283515
if (D->hasExternalLexicalStorage() && !D->getDefinition())
35293516
getExternalSource()->CompleteType(const_cast<ObjCInterfaceDecl*>(D));
@@ -3532,22 +3519,9 @@ ASTContext::getObjCLayout(const ObjCInterfaceDecl *D,
35323519
"Invalid interface decl!");
35333520

35343521
// Look up this layout, if already laid out, return what we have.
3535-
const ObjCContainerDecl *Key =
3536-
Impl ? (const ObjCContainerDecl*) Impl : (const ObjCContainerDecl*) D;
3537-
if (const ASTRecordLayout *Entry = ObjCLayouts[Key])
3522+
if (const ASTRecordLayout *Entry = ObjCLayouts[D])
35383523
return *Entry;
35393524

3540-
// Add in synthesized ivar count if laying out an implementation.
3541-
if (Impl) {
3542-
unsigned SynthCount = CountNonClassIvars(D);
3543-
// If there aren't any synthesized ivars then reuse the interface
3544-
// entry. Note we can't cache this because we simply free all
3545-
// entries later; however we shouldn't look up implementations
3546-
// frequently.
3547-
if (SynthCount == 0)
3548-
return getObjCLayout(D, nullptr);
3549-
}
3550-
35513525
ItaniumRecordLayoutBuilder Builder(*this, /*EmptySubobjects=*/nullptr);
35523526
Builder.Layout(D);
35533527

@@ -3557,7 +3531,7 @@ ASTContext::getObjCLayout(const ObjCInterfaceDecl *D,
35573531
/*RequiredAlignment : used by MS-ABI)*/
35583532
Builder.Alignment, Builder.getDataSize(), Builder.FieldOffsets);
35593533

3560-
ObjCLayouts[Key] = NewEntry;
3534+
ObjCLayouts[D] = NewEntry;
35613535

35623536
return *NewEntry;
35633537
}

‎clang/lib/CodeGen/CGObjCGNU.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1826,9 +1826,11 @@ class CGObjCGNUstep2 : public CGObjCGNUstep {
18261826
Context.getASTObjCInterfaceLayout(SuperClassDecl).getSize().getQuantity();
18271827
// Instance size is negative for classes that have not yet had their ivar
18281828
// layout calculated.
1829-
classFields.addInt(LongTy,
1830-
0 - (Context.getASTObjCImplementationLayout(OID).getSize().getQuantity() -
1831-
superInstanceSize));
1829+
classFields.addInt(
1830+
LongTy, 0 - (Context.getASTObjCInterfaceLayout(OID->getClassInterface())
1831+
.getSize()
1832+
.getQuantity() -
1833+
superInstanceSize));
18321834

18331835
if (classDecl->all_declared_ivar_begin() == nullptr)
18341836
classFields.addNullPointer(PtrTy);
@@ -3639,8 +3641,9 @@ void CGObjCGNU::GenerateClass(const ObjCImplementationDecl *OID) {
36393641
}
36403642

36413643
// Get the size of instances.
3642-
int instanceSize =
3643-
Context.getASTObjCImplementationLayout(OID).getSize().getQuantity();
3644+
int instanceSize = Context.getASTObjCInterfaceLayout(OID->getClassInterface())
3645+
.getSize()
3646+
.getQuantity();
36443647

36453648
// Collect information about instance variables.
36463649
SmallVector<llvm::Constant*, 16> IvarNames;

‎clang/lib/CodeGen/CGObjCMac.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3439,8 +3439,9 @@ void CGObjCMac::GenerateClass(const ObjCImplementationDecl *ID) {
34393439
else if ((hasMRCWeak = hasMRCWeakIvars(CGM, ID)))
34403440
Flags |= FragileABI_Class_HasMRCWeakIvars;
34413441

3442-
CharUnits Size =
3443-
CGM.getContext().getASTObjCImplementationLayout(ID).getSize();
3442+
CharUnits Size = CGM.getContext()
3443+
.getASTObjCInterfaceLayout(ID->getClassInterface())
3444+
.getSize();
34443445

34453446
// FIXME: Set CXX-structors flag.
34463447
if (ID->getClassInterface()->getVisibility() == HiddenVisibility)
@@ -6330,7 +6331,7 @@ void CGObjCNonFragileABIMac::GetClassSizeInfo(const ObjCImplementationDecl *OID,
63306331
uint32_t &InstanceStart,
63316332
uint32_t &InstanceSize) {
63326333
const ASTRecordLayout &RL =
6333-
CGM.getContext().getASTObjCImplementationLayout(OID);
6334+
CGM.getContext().getASTObjCInterfaceLayout(OID->getClassInterface());
63346335

63356336
// InstanceSize is really instance end.
63366337
InstanceSize = RL.getDataSize().getQuantity();

‎clang/lib/CodeGen/CGObjCRuntime.cpp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,24 +31,22 @@ using namespace CodeGen;
3131
uint64_t CGObjCRuntime::ComputeIvarBaseOffset(CodeGen::CodeGenModule &CGM,
3232
const ObjCInterfaceDecl *OID,
3333
const ObjCIvarDecl *Ivar) {
34-
return CGM.getContext().lookupFieldBitOffset(OID, nullptr, Ivar) /
34+
return CGM.getContext().lookupFieldBitOffset(OID, Ivar) /
3535
CGM.getContext().getCharWidth();
3636
}
3737

3838
uint64_t CGObjCRuntime::ComputeIvarBaseOffset(CodeGen::CodeGenModule &CGM,
3939
const ObjCImplementationDecl *OID,
4040
const ObjCIvarDecl *Ivar) {
41-
return CGM.getContext().lookupFieldBitOffset(OID->getClassInterface(), OID,
42-
Ivar) /
41+
return CGM.getContext().lookupFieldBitOffset(OID->getClassInterface(), Ivar) /
4342
CGM.getContext().getCharWidth();
4443
}
4544

4645
unsigned CGObjCRuntime::ComputeBitfieldBitOffset(
4746
CodeGen::CodeGenModule &CGM,
4847
const ObjCInterfaceDecl *ID,
4948
const ObjCIvarDecl *Ivar) {
50-
return CGM.getContext().lookupFieldBitOffset(ID, ID->getImplementation(),
51-
Ivar);
49+
return CGM.getContext().lookupFieldBitOffset(ID, Ivar);
5250
}
5351

5452
LValue CGObjCRuntime::EmitValueForIvarAtOffset(CodeGen::CodeGenFunction &CGF,
@@ -86,7 +84,7 @@ LValue CGObjCRuntime::EmitValueForIvarAtOffset(CodeGen::CodeGenFunction &CGF,
8684
// non-synthesized ivars but we may be called for synthesized ivars. However,
8785
// a synthesized ivar can never be a bit-field, so this is safe.
8886
uint64_t FieldBitOffset =
89-
CGF.CGM.getContext().lookupFieldBitOffset(OID, nullptr, Ivar);
87+
CGF.CGM.getContext().lookupFieldBitOffset(OID, Ivar);
9088
uint64_t BitOffset = FieldBitOffset % CGF.CGM.getContext().getCharWidth();
9189
uint64_t AlignmentBits = CGF.CGM.getTarget().getCharAlign();
9290
uint64_t BitFieldSize = Ivar->getBitWidthValue();

‎clang/lib/Sema/SemaDeclObjC.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -659,6 +659,7 @@ void SemaObjC::ActOnSuperClassOfClassInterface(
659659

660660
IDecl->setSuperClass(SuperClassTInfo);
661661
IDecl->setEndOfDefinitionLoc(SuperClassTInfo->getTypeLoc().getEndLoc());
662+
getASTContext().addObjCSubClass(IDecl->getSuperClass(), IDecl);
662663
}
663664
}
664665

@@ -2129,6 +2130,12 @@ SemaObjC::ActOnFinishObjCImplementation(Decl *ObjCImpDecl,
21292130

21302131
DeclsInGroup.push_back(ObjCImpDecl);
21312132

2133+
// Reset the cached layout if there are any ivars added to
2134+
// the implementation.
2135+
if (auto *ImplD = dyn_cast<ObjCImplementationDecl>(ObjCImpDecl))
2136+
if (!ImplD->ivar_empty())
2137+
getASTContext().ResetObjCLayout(ImplD->getClassInterface());
2138+
21322139
return SemaRef.BuildDeclaratorGroup(DeclsInGroup);
21332140
}
21342141

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@
99
// CHECK: @"OBJC_IVAR_$_SubClass.subClassIvar" = constant i64 56
1010
// CHECK: @"OBJC_IVAR_$_SubClass._subClassProperty" = hidden constant i64 64
1111
// CHECK: @"OBJC_IVAR_$_NotStaticLayout.not_static_layout_ivar" = hidden global i64 12
12+
// CHECK: @"OBJC_IVAR_$_SuperClass2._superClassProperty2" = hidden constant i64 20
13+
// CHECK: @"OBJC_IVAR_$_IntermediateClass2._IntermediateClass2Property" = hidden constant i64 24
14+
// CHECK: @"OBJC_IVAR_$_SubClass2._subClass2Property" = hidden constant i64 28
1215

1316
@interface NSObject {
1417
int these, will, never, change, ever;
@@ -138,3 +141,47 @@ -(void)meth {
138141
// CHECK: load i64, ptr @"OBJC_IVAR_$_NotStaticLayout.not_static_layout_ivar
139142
}
140143
@end
144+
145+
// CHECK: define internal i32 @"\01-[IntermediateClass2 IntermediateClass2Property]"(ptr noundef %[[SELF:.*]],
146+
// CHECK: %[[SELF_ADDR:.*]] = alloca ptr, align 8
147+
// CHECK: store ptr %[[SELF]], ptr %[[SELF_ADDR]], align 8
148+
// CHECK: %[[V0:.*]] = load ptr, ptr %[[SELF_ADDR]], align 8
149+
// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %[[V0]], i64 24
150+
// CHECK: %[[LOAD:.*]] = load atomic i32, ptr %[[ADD_PTR]] unordered, align 4
151+
// CHECK: ret i32 %[[LOAD]]
152+
153+
// CHECK: define internal i32 @"\01-[SubClass2 subClass2Property]"(ptr noundef %[[SELF:.*]],
154+
// CHECK: %[[SELF_ADDR:.*]] = alloca ptr, align 8
155+
// CHECK: store ptr %[[SELF]], ptr %[[SELF_ADDR]], align 8
156+
// CHECK: %[[V0:.*]] = load ptr, ptr %[[SELF_ADDR]], align 8
157+
// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %[[V0]], i64 28
158+
// CHECK: %[[LOAD:.*]] = load atomic i32, ptr %[[ADD_PTR]] unordered, align 4
159+
// CHECK: ret i32 %[[LOAD]]
160+
161+
@interface SuperClass2 : NSObject
162+
@property int superClassProperty2;
163+
@end
164+
165+
@interface IntermediateClass2 : SuperClass2
166+
@property int IntermediateClass2Property;
167+
@end
168+
169+
@interface IntermediateClass3 : SuperClass2
170+
@property int IntermediateClass3Property;
171+
@end
172+
173+
@interface SubClass2 : IntermediateClass2
174+
@property int subClass2Property;
175+
@end
176+
177+
@implementation IntermediateClass3
178+
@end
179+
180+
@implementation SuperClass2
181+
@end
182+
183+
@implementation IntermediateClass2
184+
@end
185+
186+
@implementation SubClass2
187+
@end

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,8 @@ @interface D : A
6363
@end
6464

6565
// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"D\00"
66-
// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"\11p\00"
67-
// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"!`\00"
66+
// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"\11\A0\00"
67+
// CHECK: @OBJC_CLASS_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"!\90\00"
6868

6969
@implementation D
7070
@synthesize p3 = _p3;

0 commit comments

Comments
 (0)