nickdesaulniers added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5385
+ if (getCodeGenOpts().hasMaybeUnusedDebugInfo() && CRD->hasDefinition())
+ DI->completeUnusedClass(*CRD);
+ else if (auto *ES = D->getASTContext().getExternalSource())
----------------
dblaikie wrote:
> 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;
> ```
Cool, thanks for the example. For your example:
`clang -g -emit-llvm -S foo.cpp -o -`:
```
...
DIGlobalVariable(name: "v1"
...
DICompositeType(tag: DW_TAG_structure_type, name: "t1"
...
```
`clang -g -fno-eliminate-unused-debug-types -emit-llvm -S foo.cpp -o -`:
```
...
DIGlobalVariable(name: "v1"
...
DICompositeType(tag: DW_TAG_structure_type, name: "t1"
...
DIDerivedType(tag: DW_TAG_member, name: "_vptr$t1"
DIDerivedType(tag: DW_TAG_pointer_type
DIDerivedType(tag: DW_TAG_pointer_type, name: "__vtbl_ptr_type"
DIBasicType(name: "int"
DISubprogram(name: "f1"
DISubroutineType
DIDerivedType(tag: DW_TAG_pointer_type
...
```
So even though `f1` was not defined, we still emit debug info for it.
Comparing the output of `llvm-dwarfdump` on `$CC -g
-fno-eliminate-unused-debug-types -c <example.cpp>` for `g++` vs `clang++`;
we're definitely producing more debug info (the vtable info).
That said, the creation of `v1` technically means that `t1` is no longer an
"unused type." If we comment out its creation, then compare `$CC -g
-fno-eliminate-unused-debug-types -c <example.cpp>` for `g++` vs `clang++`,
`g++` produces no debug info (That seems like a bug in `g++` to me), while
`clang++` with this patch produces the debug info for the `CXXRecord`, and
additional info for the vtable ptr and signature of `f1`.
I'm not sure what's expected in this case (we should at least emit the
`DW_TAG_structure_type` for `t1`, but I'm not sure about the
`DW_TAG_subprogram` for `f1`. I assume that's because we've enabled
more-than-full-debuginfo behavior? WDYT?
================
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;
----------------
dblaikie wrote:
> 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)
GCC does not yet support `-gmlt`.
https://godbolt.org/z/Wrv6sK
IIUC, `-gmlt` ("minimum line tables") is meant to minimize the amount of debug
info produced. I'm not sure what the user expects if they simultaneous ask for
more debug info (via `-fno-eliminate-unused-types`) and less debug info (via
`-gmlt`).
The current behavior is that `-fno-eliminate-unused-types` "wins" out over
`-gmlt`, resulting in the "fullest" amount of debug info being produced.
(Would you like me to add tests for this behavior to
clang/test/Driver/debug-options.c for this behavior?)
If we don't want that behavior, we should reconsider whether we want
`UnusedTypeInfo` to be part of the `DebugInfoKind` "progression." I also don't
understand what the suggested added condition to the predicate is supposed to
do; if `EmitDebugInfo` is set, doesn't the default value of
`DebugInfoConstructor` (after 227db86a1b7dd6f96f7df14890fcd071bc4fe1f5, rebased
this patch on top of) mean that we wouldn't use `UnusedTypeInfo` with `-g
-fno-eliminate-unused-type-info? Or am I misunderstanding the suggestion?
================
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"
----------------
dblaikie wrote:
> Probably good to check these end up in the retained types list
What's the best way to check this? In particular, I worry about the
`!<number>` number changing in the future, resulting in this test spuriously
failing. For this test case, we have:
```
!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1,
producer: "Nick Desaulniers clang version 12.0.0
(g...@github.com:llvm/llvm-project.git
e541558d11ca82b3664b907b350da5187f23c0c9)", isOptimized: false, runtimeVersion:
0, emissionKind: FullDebug, enums: !2, retainedTypes: !15, splitDebugInlining:
false, nameTableKind: None)
```
which tells us the `retainedTypes` is `!15`. `!15` looks like:
```
!15 = !{!16, !17, !3, !5, !18, !8}
```
In this case, should I just add the numbers? If we used regex patterns
`[0-9]`, I worry that we couldn't ensure the list would match. Is it ok to
just hardcode these?
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
