On Wed, Sep 9, 2015 at 9:26 AM, Adrian Prantl <apra...@apple.com> wrote:
> > On Sep 8, 2015, at 8:05 PM, David Blaikie <dblai...@gmail.com> wrote: > > > > On Tue, Sep 8, 2015 at 12:20 PM, Adrian Prantl via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: adrian >> Date: Tue Sep 8 14:20:27 2015 >> New Revision: 247049 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=247049&view=rev >> Log: >> Module Debugging: Emit debug type information into clang modules. >> >> When -fmodule-format is set to "obj", emit debug info for all types >> declared in a module or referenced by a declaration into the module's >> object file container. >> >> This patch adds support for C and C++ types. >> >> Added: >> cfe/trunk/test/Modules/Inputs/DebugCXX.h >> cfe/trunk/test/Modules/ModuleDebugInfo.cpp >> Modified: >> cfe/trunk/lib/CodeGen/ObjectFilePCHContainerOperations.cpp >> cfe/trunk/test/Modules/Inputs/module.map >> >> Modified: cfe/trunk/lib/CodeGen/ObjectFilePCHContainerOperations.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ObjectFilePCHContainerOperations.cpp?rev=247049&r1=247048&r2=247049&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/ObjectFilePCHContainerOperations.cpp (original) >> +++ cfe/trunk/lib/CodeGen/ObjectFilePCHContainerOperations.cpp Tue Sep 8 >> 14:20:27 2015 >> @@ -50,6 +50,34 @@ class PCHContainerGenerator : public AST >> raw_pwrite_stream *OS; >> std::shared_ptr<PCHBuffer> Buffer; >> >> + /// Visit every type and emit debug info for it. >> + struct DebugTypeVisitor : public RecursiveASTVisitor<DebugTypeVisitor> >> { >> + clang::CodeGen::CGDebugInfo &DI; >> + ASTContext &Ctx; >> + DebugTypeVisitor(clang::CodeGen::CGDebugInfo &DI, ASTContext &Ctx) >> + : DI(DI), Ctx(Ctx) {} >> + >> + /// Determine whether this type can be represented in DWARF. >> + static bool CanRepresent(const Type *Ty) { >> + return !Ty->isDependentType() && !Ty->isUndeducedType(); >> + } >> + >> + bool VisitTypeDecl(TypeDecl *D) { >> > > Does this only visit types defined in this module? Or also types otherwise > referenced by this module? (does it only visit definitions, or also > declarations?) > > > Currently this does the wrong thing and emits all types referenced by this > module because the patch that emits fwddecls for external types is not yet > committed (though expected to land soon). > OK > > > >> + QualType QualTy = Ctx.getTypeDeclType(D); >> + if (!QualTy.isNull() && CanRepresent(QualTy.getTypePtr())) >> + DI.getOrCreateStandaloneType(QualTy, D->getLocation()); >> + return true; >> + } >> + >> + bool VisitValueDecl(ValueDecl *D) { >> > > This seems strange - why would we put this type into the module debug > info? (the type may be defined elsewhere, in another module, etc - just > because there's a variable of that type defined in the module wouldn't > imply that the module should contain a definition of the type, would it? > That would seem to result in a lot of duplicate info) > > > CGDebugInfo eventually will only emit type definitions for types defined > in the current module, but that said, I think your argument is correct and > this can safely be removed. > OK - having a multi-module test case that demonstrates why removing this code improves things would be great (& that multi-module test case could also demonstrate the "excess types" issue discussed above with a FIXME of "this type isn't defined in this module, so we should omit it" or somesuch) > > > >> + QualType QualTy = D->getType(); >> + if (!QualTy.isNull() && CanRepresent(QualTy.getTypePtr())) >> + DI.getOrCreateStandaloneType(QualTy, D->getLocation()); >> + return true; >> + } >> + >> + }; >> + >> public: >> PCHContainerGenerator(DiagnosticsEngine &diags, >> const HeaderSearchOptions &HSO, >> @@ -82,6 +110,36 @@ public: >> *Ctx, HeaderSearchOpts, PreprocessorOpts, CodeGenOpts, *M, >> Diags)); >> } >> >> + bool HandleTopLevelDecl(DeclGroupRef D) override { >> + if (Diags.hasErrorOccurred() || >> + (CodeGenOpts.getDebugInfo() == CodeGenOptions::NoDebugInfo)) >> + return true; >> + >> + // Collect debug info for all decls in this group. >> + for (auto *I : D) >> + if (!I->isFromASTFile()) { >> + DebugTypeVisitor DTV(*Builder->getModuleDebugInfo(), *Ctx); >> + DTV.TraverseDecl(I); >> + } >> + return true; >> + } >> + >> + void HandleTagDeclDefinition(TagDecl *D) override { >> + if (Diags.hasErrorOccurred()) >> > > Is it particularly useful to check for errors here (& in > HandleTagDeclRequiredDefinition below)? That seems like a speed > optimization for the slow case (we're emitting diagnostics anyway, the > speed isn't too important - and we'll still have to be able to discard this > work later in case we haven't seen errors yet, but we do see them later/by > the end of the module?) > > > This is not a speed optimization but crash prevention. When compiling > source with errors clang will still build an AST and continue as far as > possible and the type system will contain unresolved types which are not > handled by CGDebugInfo so we need to bail out here. > Ah, OK - got a test case for that? > > > >> + return; >> + >> + Builder->UpdateCompletedType(D); >> + } >> + >> + void HandleTagDeclRequiredDefinition(const TagDecl *D) override { >> + if (Diags.hasErrorOccurred()) >> + return; >> + >> + if (CodeGen::CGDebugInfo *DI = Builder->getModuleDebugInfo()) >> + if (const RecordDecl *RD = dyn_cast<RecordDecl>(D)) >> + DI->completeRequiredType(RD); >> + } >> + >> /// Emit a container holding the serialized AST. >> void HandleTranslationUnit(ASTContext &Ctx) override { >> assert(M && VMContext && Builder); >> >> Added: cfe/trunk/test/Modules/Inputs/DebugCXX.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/DebugCXX.h?rev=247049&view=auto >> >> ============================================================================== >> --- cfe/trunk/test/Modules/Inputs/DebugCXX.h (added) >> +++ cfe/trunk/test/Modules/Inputs/DebugCXX.h Tue Sep 8 14:20:27 2015 >> > > A test case involving two modules (where one module depends on the other > and uses its types) would be useful to demonstrate that we aren't > redundantly emitting types into modules that didn't define them, perhaps? > > > Yes, such a test will come in the aforementioned next patch that > introduces forward declarations for AST-imported types in CGDebugInfo. > > > >> @@ -0,0 +1,52 @@ >> +/* -*- C++ -*- */ >> +namespace DebugCXX { >> + // Records. >> + struct Struct { >> + int i; >> + static int static_member; >> + }; >> + >> + // Enums. >> + enum Enum { >> + Enumerator >> + }; >> + enum { >> + e1 = '1' >> + }; >> + enum { >> + e2 = '2' >> + }; >> + >> + // Templates (instatiations). >> + template<typename T> struct traits {}; >> + template<typename T, >> + typename Traits = traits<T> >> + > class Template { >> + T member; >> + }; >> + extern template class Template<int>; >> + >> + extern template struct traits<float>; >> + typedef class Template<float> FloatInstatiation; >> + >> + inline void fn() { >> + Template<long> invisible; >> + } >> + >> + // Non-template inside a template. >> + template <class> struct Outer { >> + Outer(); >> + struct Inner { >> + Inner(Outer) {} >> + }; >> + }; >> + template <class T> Outer<T>::Outer() { >> + Inner a(*this); >> + }; >> + >> + // Partial template specialization. >> + template <typename...> class A; >> + template <typename T> class A<T> {}; >> + typedef A<void> B; >> + void foo(B) {} >> +} >> >> Modified: cfe/trunk/test/Modules/Inputs/module.map >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/module.map?rev=247049&r1=247048&r2=247049&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/Modules/Inputs/module.map (original) >> +++ cfe/trunk/test/Modules/Inputs/module.map Tue Sep 8 14:20:27 2015 >> @@ -332,6 +332,10 @@ module DebugModule { >> header "DebugModule.h" >> } >> >> +module DebugCXX { >> + header "DebugCXX.h" >> +} >> + >> module ImportNameInDir { >> header "ImportNameInDir.h" >> export * >> >> Added: cfe/trunk/test/Modules/ModuleDebugInfo.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/ModuleDebugInfo.cpp?rev=247049&view=auto >> >> ============================================================================== >> --- cfe/trunk/test/Modules/ModuleDebugInfo.cpp (added) >> +++ cfe/trunk/test/Modules/ModuleDebugInfo.cpp Tue Sep 8 14:20:27 2015 >> @@ -0,0 +1,41 @@ >> +// Test that (the same) debug info is emitted for an Objective-C++ >> +// module and a C++ precompiled header. >> + >> +// REQUIRES: asserts, shell >> + >> +// Modules: >> +// RUN: rm -rf %t >> +// RUN: %clang_cc1 -x objective-c++ -std=c++11 -g -fmodules >> -fmodule-format=obj -fimplicit-module-maps -DMODULES >> -fmodules-cache-path=%t %s -I %S/Inputs -I %t -emit-llvm -o %t.ll -mllvm >> -debug-only=pchcontainer &>%t-mod.ll >> +// RUN: cat %t-mod.ll | FileCheck %s >> + >> +// PCH: >> +// RUN: %clang_cc1 -x c++ -std=c++11 -emit-pch -fmodule-format=obj -I >> %S/Inputs -o %t.pch %S/Inputs/DebugCXX.h -mllvm -debug-only=pchcontainer >> &>%t-pch.ll >> +// RUN: cat %t-pch.ll | FileCheck %s >> + >> +#ifdef MODULES >> +@import DebugCXX; >> +#endif >> + >> > > If practical, it'd be nice to have these checks near the things tehy're > checking for - it'd make the code easier to read/understand. (haven't > looked at related tests in the same directory to see what the convention's > like) > > > The other tests all have their imported modules in Inputs/. The module is > supposed to be shared among several testcases (one for testiung the debug > info inside the module, one testing the debug info referring to the module) > so separating this out may even be somewhat more readable than having > everything inside a single file. I debated this with myself, too :-) > > > >> +// CHECK: distinct !DICompileUnit(language: DW_LANG_{{.*}}C_plus_plus, >> +// CHECK-SAME: isOptimized: false, >> +// CHECK-SAME: splitDebugFilename: >> +// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Enum" >> +// CHECK-SAME: identifier: "_ZTSN8DebugCXX4EnumE") >> +// CHECK: !DINamespace(name: "DebugCXX" >> +// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct" >> +// CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE") >> +// CHECK: !DICompositeType(tag: DW_TAG_class_type, >> +// CHECK-SAME: name: "Template<int, DebugCXX::traits<int> >" >> +// CHECK-SAME: identifier: >> "_ZTSN8DebugCXX8TemplateIiNS_6traitsIiEEEE") >> +// CHECK: !DICompositeType(tag: DW_TAG_class_type, >> +// CHECK-SAME: name: "Template<float, >> DebugCXX::traits<float> >" >> +// CHECK-SAME: identifier: >> "_ZTSN8DebugCXX8TemplateIfNS_6traitsIfEEEE") >> +// CHECK: !DICompositeType(tag: DW_TAG_class_type, >> +// CHECK-SAME: name: "Template<long, DebugCXX::traits<long> >> >" >> +// CHECK-SAME: identifier: >> "_ZTSN8DebugCXX8TemplateIlNS_6traitsIlEEEE") >> +// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>" >> +// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE") >> +// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstatiation" >> +// no mangled name here yet. >> > > Is this a 'fixme'? (might be good to phrase it that way) - any idea why > they're missing? > > > DIDerivedType doesn’t have a UID field. My uneducated guess is that the > C++ name mangling schemes don’t represent typedef sugar? > > -- adrian > > > >> +// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B", >> +// no mangled name here yet. >> >> >> _______________________________________________ >> 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
