Page MenuHomePhabricator

[Clang] implement -fno-eliminate-unused-debug-types
ClosedPublic

Authored by nickdesaulniers on May 19 2020, 2:03 PM.

Diff Detail

Unit TestsFailed

TimeTest
80 msClang.CodeGen::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\prod\llvm-project\build\bin\clang.exe -fno-eliminate-unused-debug-types -g -emit-llvm -S -o - C:\ws\prod\llvm-project\clang\test\CodeGen\debug-info-unused-types.c | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\clang\test\CodeGen\debug-info-unused-types.c
40 msClang.CodeGen::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\prod\llvm-project\build\bin\clang.exe++ -fno-eliminate-unused-debug-types -g -emit-llvm -S -o - C:\ws\prod\llvm-project\clang\test\CodeGen\debug-info-unused-types.cpp | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\clang\test\CodeGen\debug-info-unused-types.cpp

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • fix clang/test/OpenMP/parallel_codegen.cpp

Bumping for review.

bumping for review

probinson added inline comments.Jun 30 2020, 3:36 PM
clang/docs/ClangCommandLineReference.rst
2119

I think this description goes counter to how other options are described, which basically is about the positive form of the option. If that's the case, this wants to be something more like "Suppress (or emit) debug info ..."

clang/include/clang/Driver/Options.td
930

The positive form of the option suppresses info, so "Emit " and "Do not emit " are reversed here.

  • fix documentation double negation
nickdesaulniers marked 2 inline comments as done.Jun 30 2020, 4:16 PM

Bumping for review

dblaikie added inline comments.Jul 6 2020, 1:36 PM
clang/lib/CodeGen/CGDecl.cpp
103–120

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.

116

Ah, fair enough!

clang/lib/CodeGen/CodeGenModule.cpp
5377

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;
clang/lib/Driver/ToolChains/Clang.cpp
3823–3826

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)

clang/test/CodeGen/debug-info-unused-types.c
47–61

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)

clang/test/CodeGen/debug-info-unused-types.cpp
16–22

Probably good to check these end up in the retained types list

24–30

With a uniform naming scheme, perhaps this could be // NODBG-NOT: name: "unused_ ?

nickdesaulniers marked 9 inline comments as done.
clang/lib/CodeGen/CodeGenModule.cpp
5377

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?

clang/lib/Driver/ToolChains/Clang.cpp
3823–3826

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?

clang/test/CodeGen/debug-info-unused-types.cpp
16–22

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 (git@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?

dblaikie added inline comments.Jul 17 2020, 1:07 PM
clang/lib/CodeGen/CodeGenModule.cpp
5377

I think it'd be nice to handle this the same as GCC (though technically we don't offer the same customization that GCC offers - I imagine the GCC behavior of -femit-class-debug-always, which Clang doesn't have a configuration option for (well, not specifically - again, comes back to that "independent configuration" or "overriding behavior" implementation choice) - I'd sort of think of it like that flag is always enabled)

But I guess if we think of it as a series of escalations, then, yeah, it makes sense in the Clang way of thinking of it, as -fno-eliminate-unused-debug-types as overriding the default -fno-emit-class-debug-always... *shrug* don't feel /super/ strongly either way I guess. If the current implementation is such that -fno-eliminate-unused-debug-types overrides all other type homing techniques (vtable based, explicit template specialization decl/def, ctor based, and not-required-to-be-complete (define a type but only use pouinters to that type and never dereference them)) that's fine by me.

clang/lib/Driver/ToolChains/Clang.cpp
3823–3826

GCC's nearest equivalent is -g1 by the looks of it - I'd hazard a guess that -fno-eliminate-unused-types has no effect when compiling with -g1 (as -fstandalone-debug has no effect when -gmlt is specified)

I thin it's probably appropriate to implement similar behavior in Clang - that -fstandalone-debug/no-standalone-debug/no-eliminate-unused-types only applies to users that already asked for some kind of class debug info. It looks like that functionality's already implemented correctly (at least in one or two small cases I tested) for -fstandalone-debug and -fno-eliminate-unused-types should follow that.

What should happen between -fstandalone-debug and -fno-eliminate-unused-types is an open question I don't feel too strongly about which order they work in.

clang/test/CodeGen/debug-info-unused-types.cpp
16–22

You can use captures in FileCheck to make references a little more flexible. Surprised there aren't /any/ tests in LLVM testing the retainedTypes list... test/CodeGen/debug-info-extern-call.c used to use the retained types list & still has some testing remnants of it you could draw from, something like::

// CHECK: !DICompileUnit({{.*}}retainedTypes: [[RETTYPES:![0-9]+]]
// CHECK: [[RETTYPES]] = !{[[T1:![0-9]+]], [[T2:![0-9]+]], ...}
...
// CHECK: [[T1]] = !DICompositeType(...

It doesn't protect the test from changes in the order of the retained types list but otherwise resilient to extraneous metadata nodes being added or removed.

nickdesaulniers marked 4 inline comments as done.
  • rebase, add -g1 test case, don't emit unless -g2 or higher
clang/lib/Driver/ToolChains/Clang.cpp
3823–3826

Oh! Indeed!

gcc -g1 -fno-eliminate-unused-debug-types foo.c -c -o foo.o

produces no additional debug info. -g2 does. Let me fix that and add tests.

clang/test/CodeGen/debug-info-unused-types.cpp
16–22

So one issue I'm running into with this approach is that the retained type list is emitted in between the other debug nodes (some before it, some after it). The capture logic seems to have an issue referring to captures that have yet to be defined yet.

note: uses undefined variable(s): "TYPE7"

but if I move the CHECK for the retained types last, then FileCheck errors because it seems it only doesn't one pass for the CHECK statements (IIUC), so it's "too late" to parse.

Is there another way to test this, or am I holding it wrong?

dblaikie added inline comments.Jul 21 2020, 4:15 PM
clang/lib/Driver/ToolChains/Clang.cpp
3823–3827

Would this be tidier if it were rolled into the if/checking around 380 since it's a very similar option?

clang/test/CodeGen/debug-info-unused-types.cpp
16–22

Order dependence is a currently unavoidable part of FileCheck tests, so if the content you're trying to test (I'm not entirely clear from your description - so guessing a bit) looks something like:

!1 = DICompileUnit ... retainedTypes: !3
!2 = DICompositeType "x"
!3 = !{!2, !4}
!4 = DICompositeType "y"

Then you'd CHECK this something like:

; CHECK: DICompileUnit {{.*}} retainedTypes: [[RET_TY:![0-9]*]]
; CHECK: [[X:![0-9]*]] = DICompositeType "x"
; CHECK: [[RET_TY]] = !{[[X]], [[Y:![0-9]*]]}
; CHECK: [[Y]] = DICompositeType "y"

Yes, this does mean that if the types were emitted in a different order, the test would fail - but that's a limitation we're living with for pretty much all LLVM and Clang testing at the moment.

25–31

Maybe simpler to test the NODBG by NODBG-NOT: DI{{[a-zA-Z]*}}Type ? Not sure if that'd quite work, but might be adequate.

nickdesaulniers marked 4 inline comments as done.
  • rebase, add captures to tests, simplify NODBG-NOT cases
nickdesaulniers marked an inline comment as not done.Jul 31 2020, 1:42 PM
nickdesaulniers added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
3823–3827

Sorry, I recently rebased, is 380 still where you were thinking...or...? (Maybe you can add some code context in case it moves again)? Maybe 490 (DebugLevelToInfoKind)? Otherwise I don't really see other patterns that check EmitDwarf.

clang/test/CodeGen/debug-info-unused-types.cpp
25–31

DISubroutineType unfortunately would match. !DI{{CompositeType|Enumerator|DerivedType}} does work though!

nickdesaulniers removed a subscriber: srhines.

bumping for review

dblaikie added inline comments.Aug 7 2020, 12:34 PM
clang/lib/Driver/ToolChains/Clang.cpp
3782–3783

Re: https://reviews.llvm.org/D80242#inline-783632

This is the spot that seems like the place to handle UnusedTypeInfo, with something like:

if (DebugInfoKind == codegenoptions::LimitedDebugInfo) {
  if (Args.hasFlag(options::OPT_fno_eliminate_unused_debug_types,
                   options::OPT_feliminate_unused_debug_types, false))
    DebugInfoKind = codegenoptions::UnusedTypeInfo;
 else if (NeedFullDebug)
    DebugInfoKind = codegenoptions::FullDebugInfo;
}
clang/test/CodeGen/debug-info-unused-types.cpp
25–31

Sweet - thanks!

  • rebase, centralize DebugInfoKind update as per @dblaikie
nickdesaulniers marked 2 inline comments as done.Aug 7 2020, 1:25 PM
dblaikie accepted this revision.Aug 7 2020, 1:38 PM

Looks good - thanks!

This revision is now accepted and ready to land.Aug 7 2020, 1:38 PM
  • git clang-format
This revision was landed with ongoing or failed builds.Aug 7 2020, 2:14 PM
This revision was automatically updated to reflect the committed changes.
nickdesaulniers reopened this revision.Aug 7 2020, 4:13 PM
This revision is now accepted and ready to land.Aug 7 2020, 4:13 PM
nickdesaulniers planned changes to this revision.Aug 7 2020, 4:13 PM

So it looks like %clang++ should be %clangxx for windows, then for OSX the default value of debug-info-kind is different, while the tests also seem to fail there.

  • rebase, move Codegen tests to _cc1, move -g1 test to driver test, use more regex for ![0-9]+, manually test --target=x86_64-apple-macosx10.15.0 which has a different default debug info kind.
This revision is now accepted and ready to land.Aug 10 2020, 12:30 PM
  • squash and reupload

Hi @dblaikie , sorry for messing up the upload, I think it's good now, but to see the latest change, you'll need to look at the diff between the 2 prior versions (https://reviews.llvm.org/D80242?vs=284053&id=284477#toc).

I reverted this on Friday. It looks like it failed the windows tests because %clang++ is not a thing on windows. I moved the two tests under clang/tests/Codegen to use %clang_cc1 instead and moved the -g1 test to the clang/test/Driver/ test.

This also failed osx builds, which default to --target=x86_64-apple-macosx10.15.0. It looks like the default -debug-info-kind on that target is standalone, not limited. I made the NO_DEBUG_UNUSED_TYPES-NOT use a regex for other values for this to be, since we only really care that it's not unused-types. I manually tested all added test cases with --target=x86_64-apple-macosx10.15.0 or --triple=x86_64-apple-macosx10.15.0 added. I also used your suggestions from here to use more general regex for two of the debug info nodes.

Please take a look.

dblaikie added inline comments.Aug 10 2020, 2:31 PM
clang/test/Driver/debug-options.c
371–373 ↗(On Diff #284477)

Why does this test -g1 in particular? (that, I think, would always be line-tables-only, on all Clang platforms?) Rather than -g like the positive test?

clang/test/Driver/debug-options.c
371–373 ↗(On Diff #284477)

From the previous diff (https://lists.llvm.org/pipermail/llvm-dev/2020-August/144082.html) we added a test to clang/test/CodeGen/ tests for -fno... and -g1 together. Since I moved those to %clang_cc1, I moved the -fno... + -g1 tests here.

dblaikie accepted this revision.Aug 10 2020, 2:43 PM

Looks good, thanks!

clang/test/Driver/debug-options.c
371–373 ↗(On Diff #284477)

Ah, right right (link might be incorrect though - so including the one where it was discussed: https://reviews.llvm.org/D80242#inline-774452 ) - thanks!

nickdesaulniers marked 2 inline comments as done.Aug 10 2020, 2:47 PM
nickdesaulniers added inline comments.
clang/test/Driver/debug-options.c
371–373 ↗(On Diff #284477)

LOL, yeah sorry, thanks!

This revision was automatically updated to reflect the committed changes.
nickdesaulniers marked an inline comment as done.
davide added a subscriber: davide.Aug 18 2020, 2:09 PM