dblaikie added inline comments.
================
Comment at: clang/lib/CodeGen/CGDecl.cpp:103-118
+ case Decl::CXXRecord: // struct/union/class X; [C++]
+ if (CGDebugInfo *DI = getDebugInfo())
+ if (CGM.getCodeGenOpts().hasMaybeUnusedDebugInfo() &&
+ cast<CXXRecordDecl>(D).hasDefinition())
+ DI->completeUnusedClass(cast<CXXRecordDecl>(D));
+ return;
case Decl::Record: // struct/union/class X;
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > dblaikie wrote:
> > > All of these might be able to be collapsed together - using
> > > "cast<TagDecl>(D).hasDefinition()" ... to have a common implementation.
> > `TagDecl` doesn't have a method `hasDefinition`. Only `CXXRecordDecl` does.
> Also, how to get the `QualType` differs between `Decl::Enum` an
> `Decl::Record`; `getEnumType` vs `getRecordType` respectively.
Looks like ASTContext::getTypeDeclType could be used, if it makes things
especially tidier. But perhaps the TagDecl/hasDefinition difference is enough
for it not to be especially significant.
================
Comment at: clang/lib/CodeGen/CGDecl.cpp:116
+ if (CGDebugInfo *DI = getDebugInfo())
+ if (cast<EnumDecl>(&D)->getDefinition())
+ DI->EmitAndRetainType(getContext().getEnumType(cast<EnumDecl>(&D)));
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > I think the right thing to call here (& the other places in this patch) is
> > "hasDefinition" rather than "getDefinition" (this will avoid the definition
> > being deserialized when it's not needed yet (then if we don't end up
> > emitting the type because the flag hasn't been given, it won't be
> > deserialized unnecessarily))
> `TagDecl` doesn't have a method `hasDefinition`. Only `CXXRecordDecl` does.
Ah, fair enough!
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5385
+ if (getCodeGenOpts().hasMaybeUnusedDebugInfo() && CRD->hasDefinition())
+ DI->completeUnusedClass(*CRD);
+ else if (auto *ES = D->getASTContext().getExternalSource())
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > I think this might not work as intended if the type trips over the other
> > class deduplication optimizations (try manually testing with a type that
> > has an external VTable?) - and perhaps this codepath should use the
> > EmitAndRetain functionality.
> completeUnusedClass eventually calls into CGDebugInfo::completeClass which
> makes use of the TypeCache. What does it mean for a class to have "an
> external vtable?" Can you give me an example?
Here's some sample code that uses a type in the DWARF but due to the vtable
being external (an external vtable is one that's guaranteed to be emitted in
some other object file - due to the key function optimization in the Itanium
ABI - since all virtual functions must be defined if a type is ODR used, the
compiler can make a shared assumption that the vtable will only be emitted with
a single consistently chosen virtual function (ideally: the first non-inline
one) and emit the vtable only in that object and nowhere else) the type is
emitted as a declaration (when using "-fno-standalone-debug"):
```
struct t1 {
virtual void f1();
};
t1 v1;
```
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3825-3828
+ if (EmitDwarf &&
+ Args.hasFlag(options::OPT_fno_eliminate_unused_debug_types,
+ options::OPT_feliminate_unused_debug_types, false))
+ DebugInfoKind = codegenoptions::UnusedTypeInfo;
----------------
How does GCC compose this with -gmlt, for instance? I'd suspect that the
desired behavior might be for -gmlt to override this -f flag? So maybe this
should be predicated on "if (EmitDwarf && DebugInfoKind >=
codegenoptions::LimitedDebugInfo && ... "?
(so if someone wants to use only -gmlt in certain parts of their build that
doesn't get upgraded by the presence of this -f flag)
================
Comment at: clang/test/CodeGen/debug-info-unused-types.c:46-60
+struct aaaaaaaaaaaa;
+enum bbbbbbbbbbbb;
+union cccccccccccc;
+void b0(void) {
+ struct dddddddddddd;
+ enum eeeeeeeeeeee;
+ union ffffffffffff;
----------------
Maybe give these more specific names (similarly in the other test) - and/or you
could use a pattern in the naming that might make the -NOT lines easier?
eg:
```
struct decl_struct;
enum decl_enum;
...
// NODBG-NOT: name: "decl_
```
(also best to include a bit more context in the -NOT checks, otherwise there's
risk that a users build directory (even with these longer names) or other paths
might accidentally fail the checks)
================
Comment at: clang/test/CodeGen/debug-info-unused-types.cpp:15-21
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "baz"
+// CHECK: !DIEnumerator(name: "BAZ"
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "z"
+// CHECK: !DIEnumerator(name: "Z"
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "foo"
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "bar"
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "y"
----------------
Probably good to check these end up in the retained types list
================
Comment at: clang/test/CodeGen/debug-info-unused-types.cpp:23-29
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_enumeration_type, name: "baz"
+// NODBG-NOT: !DIEnumerator(name: "BAZ"
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_enumeration_type, name: "z"
+// NODBG-NOT: !DIEnumerator(name: "Z"
+// NODBG-NOT: !DIDerivedType(tag: DW_TAG_typedef, name: "foo"
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_class_type, name: "bar"
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_class_type, name: "y"
----------------
With a uniform naming scheme, perhaps this could be `// NODBG-NOT: name:
"unused_` ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80242/new/
https://reviews.llvm.org/D80242
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
