On Fri, Sep 11, 2015 at 11:55 AM, Adrian Prantl <apra...@apple.com> wrote:
> > On Sep 11, 2015, at 11:21 AM, David Blaikie <dblai...@gmail.com> wrote: > > > > On Fri, Sep 11, 2015 at 10:23 AM, Adrian Prantl via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: adrian >> Date: Fri Sep 11 12:23:08 2015 >> New Revision: 247432 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=247432&view=rev >> Log: >> Module Debugging: Emit forward declarations for types that are defined in >> clang modules, if -dwarf-ext-refs (DebugTypesExtRefs) is specified. >> >> This reimplements r247369 in about a third of the amount of code. >> Thanks to David Blaikie pointing this out in post-commit review! >> >> Added: >> cfe/trunk/test/Modules/ExtDebugInfo.cpp >> cfe/trunk/test/Modules/ExtDebugInfo.m >> Modified: >> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp >> cfe/trunk/lib/CodeGen/CGDebugInfo.h >> >> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=247432&r1=247431&r2=247432&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original) >> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Fri Sep 11 12:23:08 2015 >> @@ -148,7 +148,9 @@ void CGDebugInfo::setLocation(SourceLoca >> } >> >> llvm::DIScope *CGDebugInfo::getDeclContextDescriptor(const Decl *D) { >> - return getContextDescriptor(cast<Decl>(D->getDeclContext()), TheCU); >> + llvm::DIScope *Mod = getParentModuleOrNull(D); >> + return getContextDescriptor(cast<Decl>(D->getDeclContext()), >> + Mod ? Mod : TheCU); >> > > Might've been nice to separate the module-parenting part of the change, > smaller patches (easier to revert, review, etc), the usual stuff. > > >> } >> >> llvm::DIScope *CGDebugInfo::getContextDescriptor(const Decl *Context, >> @@ -1448,6 +1450,9 @@ void CGDebugInfo::completeRequiredType(c >> if (CXXDecl->isDynamicClass()) >> return; >> >> + if (DebugTypeExtRefs && RD->isFromASTFile()) >> + return; >> + >> QualType Ty = CGM.getContext().getRecordType(RD); >> llvm::DIType *T = getTypeOrNull(Ty); >> if (T && T->isForwardDecl()) >> @@ -1478,8 +1483,19 @@ static bool hasExplicitMemberDefinition( >> } >> >> static bool shouldOmitDefinition(CodeGenOptions::DebugInfoKind DebugKind, >> + bool DebugTypeExtRefs, >> const RecordDecl *RD, >> const LangOptions &LangOpts) { >> + // Does the type exist in an imported clang module? >> + if (DebugTypeExtRefs && RD->isFromASTFile()) { >> + if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD)) >> + if (CTSD->isExplicitInstantiationOrSpecialization()) >> + // We may not assume that this type made it into the module. >> > > I guess you mean "we may assume this type made it into the module"? (the > "not" seems erroneous - but perhaps I'm misparsing the sentence?) > > > Yes, I inverted the condition without inverting the comment. > > > >> + return true; >> > > Does this case ^ actually do anything? (aren't all these instantiations > going to be definitions anyway? so if you removed that code the below would > return true anyway, wouldn't it?) > > > Good catch! I removed the condition. > > > >> + if (RD->getDefinition()) >> + return true; >> + } >> + >> if (DebugKind > CodeGenOptions::LimitedDebugInfo) >> return false; >> >> @@ -1513,7 +1529,8 @@ static bool shouldOmitDefinition(CodeGen >> llvm::DIType *CGDebugInfo::CreateType(const RecordType *Ty) { >> RecordDecl *RD = Ty->getDecl(); >> llvm::DIType *T = >> cast_or_null<llvm::DIType>(getTypeOrNull(QualType(Ty, 0))); >> - if (T || shouldOmitDefinition(DebugKind, RD, CGM.getLangOpts())) { >> + if (T || shouldOmitDefinition(DebugKind, DebugTypeExtRefs, RD, >> + CGM.getLangOpts())) { >> if (!T) >> T = getOrCreateRecordFwdDecl(Ty, getDeclContextDescriptor(RD)); >> return T; >> @@ -1616,6 +1633,12 @@ llvm::DIType *CGDebugInfo::CreateType(co >> if (!ID) >> return nullptr; >> >> + // Return a forward declaration if this type was imported from a clang >> module. >> > > What's this extra code needed for? Isn't this what shouldOmitDefinition > already does? It causes calls to getOrCreateRecordFwdDecl and so we should > never get into the CreateTypeDefinition function for the type. > > > An ObjCInterfaceType is not a RecordDecl, so we have to replicate this > here. > > > >> + if (DebugTypeExtRefs && ID->isFromASTFile() && ID->getDefinition()) >> + return DBuilder.createForwardDecl(llvm::dwarf::DW_TAG_structure_type, >> + ID->getName(), >> + getDeclContextDescriptor(ID), >> Unit, 0); >> + >> // Get overall information about the record type for the debug info. >> llvm::DIFile *DefUnit = getOrCreateFile(ID->getLocation()); >> unsigned Line = getLineNumber(ID->getLocation()); >> @@ -1669,9 +1692,9 @@ CGDebugInfo::getOrCreateModuleRef(Extern >> TheCU->getSourceLanguage(), internString(Mod.ModuleName), >> internString(Mod.Path), TheCU->getProducer(), true, StringRef(), 0, >> internString(Mod.ASTFile), llvm::DIBuilder::FullDebug, >> Mod.Signature); >> - llvm::DIModule *M = >> - DIB.createModule(CU, Mod.ModuleName, ConfigMacros, >> internString(Mod.Path), >> - internString(CGM.getHeaderSearchOpts().Sysroot)); >> + llvm::DIModule *M = DIB.createModule( >> + CU, Mod.ModuleName, ConfigMacros, internString(Mod.Path), >> + internString(CGM.getHeaderSearchOpts().Sysroot)); >> > > This is just reformatting, yes? (stared at it for a while & couldn't spot > the difference) > > > Sorry, yes. > > > >> DIB.finalize(); >> ModRef.reset(M); >> return M; >> @@ -1930,6 +1953,7 @@ llvm::DIType *CGDebugInfo::CreateType(co >> >> llvm::DIType *CGDebugInfo::CreateEnumType(const EnumType *Ty) { >> const EnumDecl *ED = Ty->getDecl(); >> + >> uint64_t Size = 0; >> uint64_t Align = 0; >> if (!ED->getTypeForDecl()->isIncompleteType()) { >> @@ -1939,9 +1963,12 @@ llvm::DIType *CGDebugInfo::CreateEnumTyp >> >> SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU); >> >> + bool isImportedFromModule = >> + DebugTypeExtRefs && ED->isFromASTFile() && ED->getDefinition(); >> + >> > > Again, seems like code that shouldn't be here - shouldOmitDefinition > returning true should mean that getOrCreateRecordFwdDecl is called and we > never reach into the concrete CreateXXX functions. > > > Again, an EnumDecl is not a RecordDecl ( > http://clang.llvm.org/doxygen/classclang_1_1TagDecl.html). Am I missing > something obvious here? > Nope, don't think you're missing anything - thanks for the pointer/explanation. Reckon there's anything we can do to unify these bits of code (perhaps over all TagTypes)? (eg: there's a fair bit of overlap between the declaration portions of getOrCreateRecordFwdDecl and CreateEnumType. CreateType(const ObjCInterfaceType*, DIFile*) has some overlap, but it's a bit trickier - checking for implementation as well as definition, etc - dunno. > > > >> // If this is just a forward declaration, construct an appropriately >> // marked node and just return it. >> - if (!ED->getDefinition()) { >> + if (isImportedFromModule || !ED->getDefinition()) { >> llvm::DIScope *EDContext = getDeclContextDescriptor(ED); >> llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation()); >> unsigned Line = getLineNumber(ED->getLocation()); >> @@ -2081,9 +2108,8 @@ llvm::DIType *CGDebugInfo::getOrCreateTy >> if (auto *T = getTypeOrNull(Ty)) >> return T; >> >> - // Otherwise create the type. >> llvm::DIType *Res = CreateTypeNode(Ty, Unit); >> - void *TyPtr = Ty.getAsOpaquePtr(); >> + void* TyPtr = Ty.getAsOpaquePtr(); >> >> // And update the type cache. >> TypeCache[TyPtr].reset(Res); >> @@ -2115,6 +2141,19 @@ ObjCInterfaceDecl *CGDebugInfo::getObjCI >> } >> } >> >> +llvm::DIModule *CGDebugInfo::getParentModuleOrNull(const Decl *D) { >> + if (!DebugTypeExtRefs || !D || !D->isFromASTFile()) >> > > Is a null Decl valid here? What's that for? > > > Removed. > > > >> + return nullptr; >> + >> + llvm::DIModule *ModuleRef = nullptr; >> + auto *Reader = CGM.getContext().getExternalSource(); >> + auto Idx = D->getOwningModuleID(); >> + auto Info = Reader->getSourceDescriptor(Idx); >> + if (Info) >> + ModuleRef = getOrCreateModuleRef(*Info); >> + return ModuleRef; >> +} >> + >> llvm::DIType *CGDebugInfo::CreateTypeNode(QualType Ty, llvm::DIFile >> *Unit) { >> // Handle qualifiers, which recursively handles what they refer to. >> if (Ty.hasLocalQualifiers()) >> @@ -2325,8 +2364,10 @@ void CGDebugInfo::collectFunctionDeclPro >> dyn_cast_or_null<NamespaceDecl>(FD->getDeclContext())) >> FDContext = getOrCreateNameSpace(NSDecl); >> else if (const RecordDecl *RDecl = >> - dyn_cast_or_null<RecordDecl>(FD->getDeclContext())) >> - FDContext = getContextDescriptor(RDecl, TheCU); >> + dyn_cast_or_null<RecordDecl>(FD->getDeclContext())) { >> + llvm::DIScope *Mod = getParentModuleOrNull(RDecl); >> + FDContext = getContextDescriptor(RDecl, Mod ? Mod : TheCU); >> + } >> // Collect template parameters. >> TParamsArray = CollectFunctionTemplateParams(FD, Unit); >> } >> @@ -2374,7 +2415,9 @@ void CGDebugInfo::collectVarDeclProps(co >> // outside the class by putting it in the global scope. >> if (DC->isRecord()) >> DC = CGM.getContext().getTranslationUnitDecl(); >> - VDContext = getContextDescriptor(cast<Decl>(DC), TheCU); >> + >> + llvm::DIScope *Mod = getParentModuleOrNull(VD); >> + VDContext = getContextDescriptor(cast<Decl>(DC), Mod ? Mod : TheCU); >> > > I'm not quite seeing why this (& the other two/three cases) aren't using > the getDeclContextDescriptor helper you added? Could you explain it to me? > > > They are either manually picking a DeclContext different than the one > returned by Decl->GetDeclContext(), or they are using the Decl as its own > DeclContext. > > > >> } >> >> llvm::DISubprogram * >> @@ -3299,7 +3342,8 @@ void CGDebugInfo::EmitGlobalVariable(con >> llvm::DIScope *CGDebugInfo::getCurrentContextDescriptor(const Decl *D) { >> if (!LexicalBlockStack.empty()) >> return LexicalBlockStack.back(); >> - return getContextDescriptor(D, TheCU); >> + llvm::DIScope *Mod = getParentModuleOrNull(D); >> + return getContextDescriptor(D, Mod ? Mod : TheCU); >> } >> >> void CGDebugInfo::EmitUsingDirective(const UsingDirectiveDecl &UD) { >> >> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.h?rev=247432&r1=247431&r2=247432&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CGDebugInfo.h (original) >> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.h Fri Sep 11 12:23:08 2015 >> @@ -397,6 +397,9 @@ private: >> llvm::DIModule * >> getOrCreateModuleRef(ExternalASTSource::ASTSourceDescriptor Mod); >> >> + /// DebugTypeExtRefs: If \p D originated in a clang module, return it. >> + llvm::DIModule *getParentModuleOrNull(const Decl *D); >> + >> /// Get the type from the cache or create a new partial type if >> /// necessary. >> llvm::DICompositeType *getOrCreateLimitedType(const RecordType *Ty, >> >> Added: cfe/trunk/test/Modules/ExtDebugInfo.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/ExtDebugInfo.cpp?rev=247432&view=auto >> >> ============================================================================== >> --- cfe/trunk/test/Modules/ExtDebugInfo.cpp (added) >> +++ cfe/trunk/test/Modules/ExtDebugInfo.cpp Fri Sep 11 12:23:08 2015 >> @@ -0,0 +1,69 @@ >> +// RUN: rm -rf %t >> +// Test that only forward declarations are emitted for types dfined in >> modules. >> + >> +// Modules: >> +// RUN: %clang_cc1 -x objective-c++ -std=c++11 -g -dwarf-ext-refs >> -fmodules \ >> +// RUN: -fmodule-format=obj -fimplicit-module-maps -DMODULES \ >> +// RUN: -fmodules-cache-path=%t %s -I %S/Inputs -I %t -emit-llvm -o >> %t-mod.ll >> +// RUN: cat %t-mod.ll | FileCheck %s >> + >> +// PCH: >> +// RUN: %clang_cc1 -x c++ -std=c++11 -fmodule-format=obj -emit-pch >> -I%S/Inputs \ >> +// RUN: -o %t.pch %S/Inputs/DebugCXX.h >> +// RUN: %clang_cc1 -std=c++11 -g -dwarf-ext-refs -fmodule-format=obj \ >> +// RUN: -include-pch %t.pch %s -emit-llvm -o %t-pch.ll %s >> +// RUN: cat %t-pch.ll | FileCheck %s >> + >> +#ifdef MODULES >> +@import DebugCXX; >> +#endif >> + >> +using DebugCXX::Struct; >> + >> +Struct s; >> +DebugCXX::Enum e; >> +DebugCXX::Template<long> implicitTemplate; >> +DebugCXX::Template<int> explicitTemplate; >> +DebugCXX::FloatInstatiation typedefTemplate; >> +int Struct::static_member = -1; >> +enum { >> + e3 = -1 >> +} conflicting_uid = e3; >> +auto anon_enum = DebugCXX::e2; >> +char _anchor = anon_enum + conflicting_uid; >> + >> +// CHECK: ![[NS:.*]] = !DINamespace(name: "DebugCXX", scope: >> ![[MOD:[0-9]+]], >> +// CHECK: ![[MOD]] = !DIModule(scope: null, name: {{.*}}DebugCXX >> + >> +// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct", >> +// CHECK-SAME: scope: ![[NS]], >> +// CHECK-SAME: flags: DIFlagFwdDecl, >> +// CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE") >> + >> +// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Enum", >> +// CHECK-SAME: scope: ![[NS]], >> +// CHECK-SAME: flags: DIFlagFwdDecl, >> +// CHECK-SAME: identifier: "_ZTSN8DebugCXX4EnumE") >> + >> +// CHECK: !DICompositeType(tag: DW_TAG_class_type, >> + >> +// CHECK: !DICompositeType(tag: DW_TAG_class_type, >> +// CHECK-SAME: name: "Template<int, DebugCXX::traits<int> >", >> +// CHECK-SAME: scope: ![[NS]], >> +// CHECK-SAME: flags: DIFlagFwdDecl, >> +// CHECK-SAME: identifier: >> "_ZTSN8DebugCXX8TemplateIiNS_6traitsIiEEEE") >> + >> +// CHECK: !DICompositeType(tag: DW_TAG_class_type, >> +// CHECK-SAME: name: "Template<float, >> DebugCXX::traits<float> >", >> +// CHECK-SAME: scope: ![[NS]], >> +// CHECK-SAME: flags: DIFlagFwdDecl, >> +// CHECK-SAME: identifier: >> "_ZTSN8DebugCXX8TemplateIfNS_6traitsIfEEEE") >> + >> +// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "static_member", >> +// CHECK-SAME: scope: !"_ZTSN8DebugCXX6StructE" >> + >> +// CHECK: !DIGlobalVariable(name: "anon_enum", {{.*}}, type: >> ![[ANON_ENUM:[0-9]+]] >> +// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, scope: ![[NS]], >> +// CHECK-SAME: line: 16 >> + >> +// CHECK: !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !0, >> entity: !"_ZTSN8DebugCXX6StructE", line: 21) >> >> Added: cfe/trunk/test/Modules/ExtDebugInfo.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/ExtDebugInfo.m?rev=247432&view=auto >> >> ============================================================================== >> --- cfe/trunk/test/Modules/ExtDebugInfo.m (added) >> +++ cfe/trunk/test/Modules/ExtDebugInfo.m Fri Sep 11 12:23:08 2015 >> @@ -0,0 +1,29 @@ >> +// RUN: rm -rf %t >> +// Test that only forward declarations are emitted for types dfined in >> modules. >> > > Typo "dfined". > > I don't see where/how this test is testing for forward declarations - it > doesn't seem to test for any declarations or definitions. (they could be > emitted as either, or not emitted at all?) > > > I made it stricter now. > > thanks! > adrian > > > >> + >> +// Modules: >> +// RUN: %clang_cc1 -x objective-c -g -dwarf-ext-refs -fmodules \ >> +// RUN: -fmodule-format=obj -fimplicit-module-maps -DMODULES \ >> +// RUN: -fmodules-cache-path=%t %s -I %S/Inputs -I %t -emit-llvm -o >> %t-mod.ll >> +// RUN: cat %t-mod.ll | FileCheck %s >> + >> +// PCH: >> +// RUN: %clang_cc1 -x objective-c -fmodule-format=obj -emit-pch >> -I%S/Inputs \ >> +// RUN: -o %t.pch %S/Inputs/DebugObjC.h >> +// RUN: %clang_cc1 -x objective-c -g -dwarf-ext-refs -fmodule-format=obj >> \ >> +// RUN: -include-pch %t.pch %s -emit-llvm -o %t-pch.ll %s >> +// RUN: cat %t-pch.ll | FileCheck %s >> + >> +#ifdef MODULES >> +@import DebugObjC; >> +#endif >> + >> +int foo(ObjCClass *c) { >> + [c instanceMethodWithInt: 0]; >> + return [c property]; >> +} >> + >> +// CHECK: !DICompositeType(tag: DW_TAG_structure_type, >> +// CHECK-SAME: scope: ![[MOD:[0-9]+]], >> +// CHECK-SAME: flags: DIFlagFwdDecl) >> +// CHECK: ![[MOD]] = !DIModule(scope: null, name: {{.*}}DebugObjC > > >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
