r247049 - Module Debugging: Emit debug type information into clang modules.
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 9 10:15:51 PDT 2015
On Wed, Sep 9, 2015 at 9:26 AM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Sep 8, 2015, at 8:05 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Sep 8, 2015 at 12:20 PM, Adrian Prantl via cfe-commits <
> cfe-commits at 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
>> + at 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 at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150909/0e4cb2a0/attachment-0001.html>
More information about the cfe-commits
mailing list