Hi David, On May 4, 2014, at 5:18 PM, David Blaikie <[email protected]> wrote:
> Oh, and just for the fun of it, this is a yak I discovered (& is > somewhat related to) while trying to fix PR19598, a memory leak in > Clang. > > On Sun, May 4, 2014 at 5:09 PM, David Blaikie <[email protected]> wrote: >> On Wed, Mar 6, 2013 at 2:03 PM, Adrian Prantl <[email protected]> wrote: >>> Author: adrian >>> Date: Wed Mar 6 16:03:30 2013 >>> New Revision: 176584 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=176584&view=rev >>> Log: >>> Ensure that DIType is regenerated after we visit an implementation >>> that adds ivars to an interface. >>> >>> Fixes rdar://13175234 >>> >>> This is an update to r176116 that performs a smart caching of interfaces. >> >> *performs necromancy* >> >> So, I was looking at this code and how we've refactored type >> completion code in the last year or two (using complete type and >> requires complete type callbacks, etc) and I was going to change >> getOrCreateType to simply bail if getTypeOrNull returned non-null, >> rather than using getCompleteTypeOrNull. Is CGDebugInfo::completeType() and friends that callback mechanism? (Unfortunately Doxygen isn’t of much help here :-) >> >> But that causes these 3 tests to fail, because these tests expect to >> attempt to rebuild the type every time it's referenced if it isn't >> complete. We don't need to do this for C++ types (rather than waiting >> for a use after the type becomes complete to create the complete debug >> info, we use the callback to inform us that the type is complete). >> The main difference to C++ is that there is no one point at which a type is complete, but I suppose we could use a callback to update the type whenever, e.g., an extension is added. This way at least we can simplify the logic in CreateType(). >> I think it will simplify (marginally speed up, but I doubt that's >> relevant) the implementation of getTypeOrNull if we could do something >> similar for IVars - indeed, it would also be more correct. Try adding >> this to the end of debug-info-ivars-indirect.m: >> >> >> I *source(); >> >> @interface I() >> { >> @public int c; >> } >> @end >> >> // CHECK: ; [ DW_TAG_member ] [c] >> >> void use() { >> int _c = source()->c; >> } >> >> Since we never generate debug info for the 'I' type after it is >> extended to have the 'c' member, we never generate debug info for that >> member. That has been bugging me for a while, actually. >> >> Is there a way we could do this in a callback fashion - thus being >> more correct and efficient? >> >> Alternatively: we could kill the callback system, make a list of types >> we've emitted declarations for, and walk that list at the end of the >> translation unit, building whichever types we need (that way we'd be >> sure to find all the ivars, for example). That depends whether it's >> worth visiting all the types to check if they need to be upgraded to >> definitions at the end. >> It’s certainly worth looking at -- I’ll give this some thought and get back to you when I made up my mind about what solution I prefer. thanks for noticing! Adrian >> >>> >>> Added: >>> cfe/trunk/test/CodeGenObjC/debug-info-ivars-extension.m >>> cfe/trunk/test/CodeGenObjC/debug-info-ivars-indirect.m >>> cfe/trunk/test/CodeGenObjC/debug-info-ivars-private.m >>> Modified: >>> cfe/trunk/include/clang/AST/DeclObjC.h >>> cfe/trunk/lib/AST/DeclObjC.cpp >>> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp >>> cfe/trunk/lib/CodeGen/CGDebugInfo.h >>> >>> Modified: cfe/trunk/include/clang/AST/DeclObjC.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclObjC.h?rev=176584&r1=176583&r2=176584&view=diff >>> ============================================================================== >>> --- cfe/trunk/include/clang/AST/DeclObjC.h (original) >>> +++ cfe/trunk/include/clang/AST/DeclObjC.h Wed Mar 6 16:03:30 2013 >>> @@ -651,6 +651,10 @@ class ObjCInterfaceDecl : public ObjCCon >>> /// completed by the external AST source when required. >>> mutable bool ExternallyCompleted : 1; >>> >>> + /// \brief Indicates that the ivar cache does not yet include ivars >>> + /// declared in the implementation. >>> + mutable bool IvarListMissingImplementation : 1; >>> + >>> /// \brief The location of the superclass, if any. >>> SourceLocation SuperClassLoc; >>> >>> @@ -660,7 +664,8 @@ class ObjCInterfaceDecl : public ObjCCon >>> SourceLocation EndLoc; >>> >>> DefinitionData() : Definition(), SuperClass(), CategoryList(), >>> IvarList(), >>> - ExternallyCompleted() { } >>> + ExternallyCompleted(), >>> + IvarListMissingImplementation(true) { } >>> }; >>> >>> ObjCInterfaceDecl(DeclContext *DC, SourceLocation atLoc, IdentifierInfo >>> *Id, >>> >>> Modified: cfe/trunk/lib/AST/DeclObjC.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclObjC.cpp?rev=176584&r1=176583&r2=176584&view=diff >>> ============================================================================== >>> --- cfe/trunk/lib/AST/DeclObjC.cpp (original) >>> +++ cfe/trunk/lib/AST/DeclObjC.cpp Wed Mar 6 16:03:30 2013 >>> @@ -1093,38 +1093,51 @@ namespace { >>> /// all_declared_ivar_begin - return first ivar declared in this class, >>> /// its extensions and its implementation. Lazily build the list on first >>> /// access. >>> +/// >>> +/// Caveat: The list returned by this method reflects the current >>> +/// state of the parser. The cache will be updated for every ivar >>> +/// added by an extension or the implementation when they are >>> +/// encountered. >>> +/// See also ObjCIvarDecl::Create(). >>> ObjCIvarDecl *ObjCInterfaceDecl::all_declared_ivar_begin() { >>> // FIXME: Should make sure no callers ever do this. >>> if (!hasDefinition()) >>> return 0; >>> >>> - if (data().IvarList) >>> - return data().IvarList; >>> - >>> ObjCIvarDecl *curIvar = 0; >>> - if (!ivar_empty()) { >>> - ObjCInterfaceDecl::ivar_iterator I = ivar_begin(), E = ivar_end(); >>> - data().IvarList = *I; ++I; >>> - for (curIvar = data().IvarList; I != E; curIvar = *I, ++I) >>> - curIvar->setNextIvar(*I); >>> - } >>> + if (!data().IvarList) { >>> + if (!ivar_empty()) { >>> + ObjCInterfaceDecl::ivar_iterator I = ivar_begin(), E = ivar_end(); >>> + data().IvarList = *I; ++I; >>> + for (curIvar = data().IvarList; I != E; curIvar = *I, ++I) >>> + curIvar->setNextIvar(*I); >>> + } >>> >>> - for (ObjCInterfaceDecl::known_extensions_iterator >>> - Ext = known_extensions_begin(), >>> - ExtEnd = known_extensions_end(); >>> - Ext != ExtEnd; ++Ext) { >>> - if (!Ext->ivar_empty()) { >>> - ObjCCategoryDecl::ivar_iterator I = Ext->ivar_begin(),E = >>> Ext->ivar_end(); >>> - if (!data().IvarList) { >>> - data().IvarList = *I; ++I; >>> - curIvar = data().IvarList; >>> + for (ObjCInterfaceDecl::known_extensions_iterator >>> + Ext = known_extensions_begin(), >>> + ExtEnd = known_extensions_end(); >>> + Ext != ExtEnd; ++Ext) { >>> + if (!Ext->ivar_empty()) { >>> + ObjCCategoryDecl::ivar_iterator >>> + I = Ext->ivar_begin(), >>> + E = Ext->ivar_end(); >>> + if (!data().IvarList) { >>> + data().IvarList = *I; ++I; >>> + curIvar = data().IvarList; >>> + } >>> + for ( ;I != E; curIvar = *I, ++I) >>> + curIvar->setNextIvar(*I); >>> } >>> - for ( ;I != E; curIvar = *I, ++I) >>> - curIvar->setNextIvar(*I); >>> } >>> + data().IvarListMissingImplementation = true; >>> } >>> + >>> + // cached and complete! >>> + if (!data().IvarListMissingImplementation) >>> + return data().IvarList; >>> >>> if (ObjCImplementationDecl *ImplDecl = getImplementation()) { >>> + data().IvarListMissingImplementation = false; >>> if (!ImplDecl->ivar_empty()) { >>> SmallVector<SynthesizeIvarChunk, 16> layout; >>> for (ObjCImplementationDecl::ivar_iterator I = ImplDecl->ivar_begin(), >>> >>> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=176584&r1=176583&r2=176584&view=diff >>> ============================================================================== >>> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original) >>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Wed Mar 6 16:03:30 2013 >>> @@ -1343,7 +1343,7 @@ llvm::DIType CGDebugInfo::CreateType(con >>> LexicalBlockStack.push_back(FwdDeclNode); >>> RegionMap[Ty->getDecl()] = llvm::WeakVH(FwdDecl); >>> >>> - // Add this to the completed types cache since we're completing it. >>> + // Add this to the completed-type cache while we're completing it >>> recursively. >>> CompletedTypeCache[QualType(Ty, 0).getAsOpaquePtr()] = FwdDecl; >>> >>> // Convert all the elements. >>> @@ -1436,7 +1436,8 @@ llvm::DIType CGDebugInfo::CreateType(con >>> >>> // Otherwise, insert it into the CompletedTypeCache so that recursive uses >>> // will find it and we're emitting the complete type. >>> - CompletedTypeCache[QualType(Ty, 0).getAsOpaquePtr()] = RealDecl; >>> + QualType QualTy = QualType(Ty, 0); >>> + CompletedTypeCache[QualTy.getAsOpaquePtr()] = RealDecl; >>> // Push the struct on region stack. >>> llvm::TrackingVH<llvm::MDNode> FwdDeclNode(RealDecl); >>> >>> @@ -1561,6 +1562,12 @@ llvm::DIType CGDebugInfo::CreateType(con >>> >>> llvm::DIArray Elements = DBuilder.getOrCreateArray(EltTys); >>> FwdDeclNode->replaceOperandWith(10, Elements); >>> + >>> + // If the implementation is not yet set, we do not want to mark it >>> + // as complete. An implementation may declare additional >>> + // private ivars that we would miss otherwise. >>> + if (ID->getImplementation() == 0) >>> + CompletedTypeCache.erase(QualTy.getAsOpaquePtr()); >>> >>> LexicalBlockStack.pop_back(); >>> return llvm::DIType(FwdDeclNode); >>> @@ -1790,13 +1797,27 @@ llvm::DIType CGDebugInfo::getCompletedTy >>> Ty = UnwrapTypeForDebugInfo(Ty, CGM.getContext()); >>> >>> // Check for existing entry. >>> + llvm::Value *V = 0; >>> llvm::DenseMap<void *, llvm::WeakVH>::iterator it = >>> CompletedTypeCache.find(Ty.getAsOpaquePtr()); >>> - if (it != CompletedTypeCache.end()) { >>> - // Verify that the debug info still exists. >>> - if (llvm::Value *V = it->second) >>> - return llvm::DIType(cast<llvm::MDNode>(V)); >>> - } >>> + if (it != CompletedTypeCache.end()) >>> + V = it->second; >>> + else { >>> + // Is there a cached interface that hasn't changed? >>> + llvm::DenseMap<void *, std::pair<llvm::WeakVH, unsigned > > >>> + ::iterator it1 = ObjCInterfaceCache.find(Ty.getAsOpaquePtr()); >>> + >>> + if (it1 != ObjCInterfaceCache.end()) >>> + if (ObjCInterfaceDecl* Decl = getObjCInterfaceDecl(Ty)) >>> + if (Checksum(Decl) == it1->second.second) { >>> + // Return cached type. >>> + V = it1->second.first; >>> + } >>> + } >>> + >>> + // Verify that any cached debug info still exists. >>> + if (V != 0) >>> + return llvm::DIType(cast<llvm::MDNode>(V)); >>> >>> return llvm::DIType(); >>> } >>> @@ -1824,6 +1845,16 @@ llvm::DIType CGDebugInfo::getOrCreateTyp >>> ReplaceMap.push_back(std::make_pair(Ty.getAsOpaquePtr(), >>> static_cast<llvm::Value*>(TC))); >>> >>> + // Do not cache the type if it may be incomplete. >>> + if (ObjCInterfaceDecl* Decl = getObjCInterfaceDecl(Ty)) { >>> + // clang::ParseAST handles each TopLevelDecl immediately after it was >>> parsed. >>> + // A subsequent implementation may add more ivars to an interface, >>> which is >>> + // why we cache it together with a checksum to see if it changed. >>> + ObjCInterfaceCache[Ty.getAsOpaquePtr()] = >>> + std::make_pair(Res, Checksum(Decl)); >>> + return Res; >>> + } >>> + >>> // And update the type cache. >>> TypeCache[Ty.getAsOpaquePtr()] = Res; >>> >>> @@ -1833,6 +1864,26 @@ llvm::DIType CGDebugInfo::getOrCreateTyp >>> return Res; >>> } >>> >>> +/// Currently the checksum merely consists of the number of ivars. >>> +unsigned CGDebugInfo::Checksum(const ObjCInterfaceDecl >>> + *InterfaceDecl) { >>> + unsigned IvarNo = 0; >>> + for (const ObjCIvarDecl *Ivar = InterfaceDecl->all_declared_ivar_begin(); >>> + Ivar != 0; Ivar = Ivar->getNextIvar()) ++IvarNo; >>> + return IvarNo; >>> +} >>> + >>> +ObjCInterfaceDecl *CGDebugInfo::getObjCInterfaceDecl(QualType Ty) { >>> + switch (Ty->getTypeClass()) { >>> + case Type::ObjCObjectPointer: >>> + return >>> getObjCInterfaceDecl(cast<ObjCObjectPointerType>(Ty)->getPointeeType()); >>> + case Type::ObjCInterface: >>> + return cast<ObjCInterfaceType>(Ty)->getDecl(); >>> + default: >>> + return 0; >>> + } >>> +} >>> + >>> /// CreateTypeNode - Create a new debug type node. >>> llvm::DIType CGDebugInfo::CreateTypeNode(QualType Ty, llvm::DIFile Unit) { >>> // Handle qualifiers, which recursively handles what they refer to. >>> >>> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.h?rev=176584&r1=176583&r2=176584&view=diff >>> ============================================================================== >>> --- cfe/trunk/lib/CodeGen/CGDebugInfo.h (original) >>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.h Wed Mar 6 16:03:30 2013 >>> @@ -32,6 +32,7 @@ namespace clang { >>> class CXXMethodDecl; >>> class VarDecl; >>> class ObjCInterfaceDecl; >>> + class ObjCIvarDecl; >>> class ClassTemplateSpecializationDecl; >>> class GlobalDecl; >>> >>> @@ -60,6 +61,11 @@ class CGDebugInfo { >>> /// TypeCache - Cache of previously constructed Types. >>> llvm::DenseMap<void *, llvm::WeakVH> TypeCache; >>> >>> + /// ObjCInterfaceCache - Cache of previously constructed interfaces >>> + /// which may change. Storing a pair of DIType and checksum. >>> + llvm::DenseMap<void *, std::pair<llvm::WeakVH, unsigned > > >>> + ObjCInterfaceCache; >>> + >>> /// CompleteTypeCache - Cache of previously constructed complete >>> RecordTypes. >>> llvm::DenseMap<void *, llvm::WeakVH> CompletedTypeCache; >>> >>> @@ -89,6 +95,7 @@ class CGDebugInfo { >>> llvm::DenseMap<const Decl *, llvm::WeakVH> StaticDataMemberCache; >>> >>> /// Helper functions for getOrCreateType. >>> + unsigned Checksum(const ObjCInterfaceDecl *InterfaceDecl); >>> llvm::DIType CreateType(const BuiltinType *Ty); >>> llvm::DIType CreateType(const ComplexType *Ty); >>> llvm::DIType CreateQualifiedType(QualType Ty, llvm::DIFile F); >>> @@ -299,6 +306,10 @@ private: >>> /// CreateTypeNode - Create type metadata for a source language type. >>> llvm::DIType CreateTypeNode(QualType Ty, llvm::DIFile F); >>> >>> + /// getObjCInterfaceDecl - return the underlying ObjCInterfaceDecl >>> + /// if Ty is an ObjCInterface or a pointer to one. >>> + ObjCInterfaceDecl* getObjCInterfaceDecl(QualType Ty); >>> + >>> /// CreateLimitedTypeNode - Create type metadata for a source language >>> /// type, but only partial types for records. >>> llvm::DIType CreateLimitedTypeNode(QualType Ty, llvm::DIFile F); >>> >>> Added: cfe/trunk/test/CodeGenObjC/debug-info-ivars-extension.m >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/debug-info-ivars-extension.m?rev=176584&view=auto >>> ============================================================================== >>> --- cfe/trunk/test/CodeGenObjC/debug-info-ivars-extension.m (added) >>> +++ cfe/trunk/test/CodeGenObjC/debug-info-ivars-extension.m Wed Mar 6 >>> 16:03:30 2013 >>> @@ -0,0 +1,28 @@ >>> +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -g %s -o - | >>> FileCheck %s >>> + >>> +// Make sure we generate debug symbols for ivars added by a class >>> extension. >>> + >>> +@interface I >>> +{ >>> + @public int a; >>> +} >>> +@end >>> + >>> +void foo(I* pi) { >>> + // poking into pi for primary class ivars. >>> + int _a = pi->a; >>> +} >>> + >>> +@interface I() >>> +{ >>> + @public int b; >>> +} >>> +@end >>> + >>> +void gorf (I* pg) { >>> + // poking into pg for ivars for class extension >>> + int _b = pg->b; >>> +} >>> + >>> +// CHECK: metadata !{i32 {{[0-9]*}}, metadata !{{[0-9]*}}, metadata !"a", >>> metadata !{{[0-9]*}}, i32 7, i64 32, i64 32, i64 0, i32 0, metadata >>> !{{[0-9]*}}, null} ; [ DW_TAG_member ] [a] [line 7, size 32, align 32, >>> offset 0] [from int] >>> +// CHECK: metadata !{i32 {{[0-9]*}}, metadata !{{[0-9]*}}, metadata !"b", >>> metadata !{{[0-9]*}}, i32 18, i64 32, i64 32, i64 0, i32 0, metadata >>> !{{[0-9]*}}, null} ; [ DW_TAG_member ] [b] [line 18, size 32, align 32, >>> offset 0] [from int] >>> >>> Added: cfe/trunk/test/CodeGenObjC/debug-info-ivars-indirect.m >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/debug-info-ivars-indirect.m?rev=176584&view=auto >>> ============================================================================== >>> --- cfe/trunk/test/CodeGenObjC/debug-info-ivars-indirect.m (added) >>> +++ cfe/trunk/test/CodeGenObjC/debug-info-ivars-indirect.m Wed Mar 6 >>> 16:03:30 2013 >>> @@ -0,0 +1,32 @@ >>> +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -g %s -o - | >>> FileCheck %s >>> + >>> +// Make sure we generate debug symbols for an indirectly referenced >>> +// extension to an interface. >>> + >>> +@interface I >>> +{ >>> + @public int a; >>> +} >>> +@end >>> + >>> +void foo(I* pi) { >>> + int _a = pi->a; >>> +} >>> + >>> +// another layer of indirection >>> +struct S >>> +{ >>> + I* i; >>> +}; >>> + >>> +@interface I() >>> +{ >>> + @public int b; >>> +} >>> +@end >>> + >>> +void gorf (struct S* s) { >>> + int _b = s->i->b; >>> +} >>> + >>> +// CHECK: metadata !{i32 {{[0-9]*}}, metadata !{{[0-9]*}}, metadata !"b", >>> metadata !{{[0-9]*}}, i32 24, i64 32, i64 32, i64 0, i32 0, metadata >>> !{{[0-9]*}}, null} ; [ DW_TAG_member ] [b] [line 24, size 32, align 32, >>> offset 0] [from int] >>> >>> Added: cfe/trunk/test/CodeGenObjC/debug-info-ivars-private.m >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/debug-info-ivars-private.m?rev=176584&view=auto >>> ============================================================================== >>> --- cfe/trunk/test/CodeGenObjC/debug-info-ivars-private.m (added) >>> +++ cfe/trunk/test/CodeGenObjC/debug-info-ivars-private.m Wed Mar 6 >>> 16:03:30 2013 >>> @@ -0,0 +1,36 @@ >>> +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -g %s -o - | >>> FileCheck %s >>> + >>> +// Debug symbols for private ivars. This test ensures that we are >>> +// generating debug info for ivars added by the implementation. >>> +__attribute((objc_root_class)) @interface NSObject { >>> + id isa; >>> +} >>> +@end >>> + >>> +@protocol Protocol >>> +@end >>> + >>> +@interface Delegate : NSObject<Protocol> { >>> + @protected int foo; >>> +} >>> +@end >>> + >>> +@interface Delegate(NSObject) >>> +- (void)f; >>> +@end >>> + >>> +@implementation Delegate(NSObject) >>> +- (void)f { return; } >>> +@end >>> + >>> +@implementation Delegate { >>> + int bar; >>> +} >>> + >>> +- (void)g:(NSObject*) anObject { >>> + bar = foo; >>> +} >>> +@end >>> + >>> +// CHECK: metadata !{i32 {{[0-9]*}}, metadata !{{[0-9]*}}, metadata >>> !"foo", metadata !{{[0-9]*}}, i32 14, i64 32, i64 32, i64 0, i32 2, >>> metadata !{{[0-9]*}}, null} ; [ DW_TAG_member ] [foo] [line 14, size 32, >>> align 32, offset 0] [protected] [from int] >>> +// CHECK: metadata !{i32 {{[0-9]*}}, metadata !{{[0-9]*}}, metadata >>> !"bar", metadata !{{[0-9]*}}, i32 27, i64 32, i64 32, i64 0, i32 1, >>> metadata !{{[0-9]*}}, null} ; [ DW_TAG_member ] [bar] [line 27, size 32, >>> align 32, offset 0] [private] [from int] >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
