This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • add support for structs, unions, and enums

For C++, I'd imagine:

class foo {};
using my_foo = foo;
template <typename bar>
struct baz {
  bar my_bar;
};

but it seems that g++ doesn't emit debug info the for the templated struct.

  • add C++ using and class support

CXXRecordDecl is a funny case, the AST looks like:

|-CXXRecordDecl 0x5c661a8 <clang/baz.cpp:1:1, col:12> col:7 class foo definition
| |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_
| `-CXXRecordDecl 0x5c662d0 <col:1, col:7> col:7 implicit class foo
...

There's probably a bunch more C++ cases I'm not thinking of. Reading through gcc's test suite for -fno-eliminate-unused-debug-types, this feature doesn't have tests for anything with templates.

clang/lib/CodeGen/CodeGenModule.cpp
5401–5403

The difference between using DebugInfo vs getModuleDebugInfo in this method is *killing* me. Same with return vs break.

probably should add a test for enum class in C++

clang/lib/CodeGen/CodeGenModule.cpp
5596

I think it makes sense at this point to rename EmitExplicitCastType to RetainType or EmitType. WDYT?

  • add enum class test case

Bumping for review

dblaikie added inline comments.May 28 2020, 6:58 PM
clang/lib/CodeGen/CodeGenModule.cpp
5344

Since this is only for top-level decls, have you checked this works for types inside namespaces, local types in functions (well, that one might not be supported by GCC - so probably good to test/compare GCC's behavior here too), etc?

5400–5403

Perhaps this could be modified to use the same general purpose utility like the other type emissions (as speculated in another comment "EmitUnusedType" or the like)

5401–5403

Feel free to send separate patches to clean these things up.

5598

Rather than testing DebugUnusedType for every call - might be best to add a "EmitUnusedType" function that tests the flag and does the emission? (same way EmitExplicitCastType already checks for a certain level of debug info before emitting)

clang/lib/Frontend/CompilerInvocation.cpp
806

Could this be rolled into the debug-info-kind? (a kind beyond "standalone")

clang/lib/Frontend/CompilerInvocation.cpp
806

That sounds like a good idea. Looking at the definition of DebugInfoKind (llvm/llvm-project/clang/include/clang/Basic/DebugInfoOptions.h), it seems that DebugInfoKind is an enum that defines a "level" of debug info to emit? Looking at the guard in CGDebugInfo::EmitExplicitCastType (llvm/llvm-project/clang/lib/CodeGen/CGDebugInfo.cpp), it calls CodeGenOptions::hasReducedDebugInfo() which does a comparison against a certain level. That seems to indicate the order of the enumerations is important. Do you have a suggestion what order I should add the new enum?

I'm guessing after LimitedDebugInfo or DebugInfoConstructor, but before FullDebugInfo? (I find the name of the method hasReducedDebugInfo confusing in that regard).

nickdesaulniers marked 3 inline comments as done.May 29 2020, 2:04 PM
nickdesaulniers added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
5401–5403
nickdesaulniers marked an inline comment as done.May 29 2020, 2:55 PM
nickdesaulniers added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
5598

It can be; what I don't like about that approach is that we have to determine the QualType here to pass in, at which point such function might just return without doing anything. (ie. we have to do slightly more work in the case where the debugging of unused types was *not* requested). But that's a minor nit and we can live with it?

clang/lib/Frontend/CompilerInvocation.cpp
806

Ok, so I have a diff that implements this approach. I feel like I should maybe publish it as a child commit to this one, to be able to more easily discuss it?

Two problems I run into:

  1. as alluded to in my previous response in this thread, DebugInfoKind is an enum that specifies a level. It tends to "drag" other debug flags in based on the ordering. Looking at extending the switch in CGDebugInfo::CreateCompileUnit (clang/lib/CodeGen/CGDebugInfo.cpp), it's not at all clear to me which we existing case should we choose?
  2. we want the flag -fno-eliminate-unused-debug-types to match GCC for compatibility. We can additionally add a new debug info kind like "standalone" (clang/lib/Frontend/CompilerInvocation.cpp), but it's not clear how the two flags together should interact.

The suggestion for a new debug info kind feels like a recommendation to add a new "level" of debug info, but -fno-eliminate-unused-debug-types feels like it should be mutually exclusive of debug info kind? (I guess GCC does *require* -g with -fno-eliminate-unused-debug-types)

@dblaikie maybe you have more recommendations on this?

dblaikie added inline comments.May 29 2020, 3:05 PM
clang/lib/CodeGen/CodeGenModule.cpp
5596

Yep - now that I understand the confusingly named hasReducedDebugInfo

"RetainType" sounds good to me, maybe "EmitAndRetainType" might be worthwhile for the extra clarity/symmetry with other "Emit" functions.

clang/lib/Frontend/CompilerInvocation.cpp
806

This value would probably go "after" "full" (full isn't full enough, as you've found - you need a mode that's even "fullerer")

Perhaps renaming "Full" to "Referenced" and then introducing this new kind as the new "Full" or maybe under a somewhat different name to avoid ambiguity.

Any suggestions on a less confusing name for "hasReducedDebugInfo"? (I think it was basically "Has less than full debug info"... but, yep, it doesn't do that at all - it's "HasTypeAndVariableDebugInfo" by the looks of it/by the comment)

dblaikie added inline comments.May 30 2020, 12:22 PM
clang/lib/CodeGen/CodeGenModule.cpp
5598

I don't believe getting the QualType from a TypeDecl is an expensive operation/enough to try to avoid it here. If it is I'd rather have a "EmitUnusedType(TypeDecl*)" function, and then the conditional QualType retrieval could be in there, written once rather than several times.

clang/lib/Frontend/CompilerInvocation.cpp
806

Ok, so I have a diff that implements this approach. I feel like I should maybe publish it as a child commit to this one, to be able to more easily discuss it?

Two problems I run into:

  1. as alluded to in my previous response in this thread, DebugInfoKind is an enum that specifies a level. It tends to "drag" other debug flags in based on the ordering. Looking at extending the switch in CGDebugInfo::CreateCompileUnit (clang/lib/CodeGen/CGDebugInfo.cpp), it's not at all clear to me which we existing case should we choose?
  2. we want the flag -fno-eliminate-unused-debug-types to match GCC for compatibility. We can additionally add a new debug info kind like "standalone" (clang/lib/Frontend/CompilerInvocation.cpp), but it's not clear how the two flags together should interact.

I'm not suggesting adding a new driver-level flag for this, but implementing the GCC flag name in terms of the debug-info-kind. Probably by having "-fno-eliminate-unused-debug-types" override "-fstandalone-debug" - whichever comes later wins. Because they are part of a progression. Though admittedly that might get a smidge confusing about exactly whit no/yes versions of these two flags override each other - but I think that's inevitable confusion with the nature of these flags.

What does GCC do for its -f[no-]emit-class-debug-always (which is somewhat similar to -fstandalone-debug) V -f[no-]eliminate-unused-debug-types? I'm not sure we'll want to emulate the behavior exxactly, but it's probably a good place to start to see if there's an existing model that looks OK here.

The suggestion for a new debug info kind feels like a recommendation to add a new "level" of debug info, but -fno-eliminate-unused-debug-types feels like it should be mutually exclusive of debug info kind? (I guess GCC does *require* -g with -fno-eliminate-unused-debug-types)

Sorry, I'm not following here - perhaps you could explain in different words? At the driver/user level, "debug info kind" doesn't exist - there's flags like "-g", "-gmlt", "-fstandalone-debug", etc. That are mapped to debug-info-kind on the -cc1 command line. I'm suggesting "-fno-eliminate-unused-debug-types" is another of those driver flags that is taken into account when mapping down to the cc1 "debug info kind".

MaskRay added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
291

This flag is now more than "typedefs"

clang/include/clang/Driver/Options.td
3325–3326

This group is for ignored options. Add this near other f options.

I fixed the FIXME about BooleanFFlag with TableGen special identifier NAME. The way we define boolean options still look verbose to me, so I just created D80883.

For C++, I'd imagine:

class foo {};
using my_foo = foo;
template <typename bar>
struct baz {
  bar my_bar;
};

but it seems that g++ doesn't emit debug info the for the templated struct.

There's no debug info for templates, only for instantiations of templates. HTH.

  • missed one part of the rebase
nickdesaulniers added inline comments.Jun 2 2020, 1:11 PM
clang/lib/Frontend/CompilerInvocation.cpp
806

Are debug info kinds a progression or mutually exclusive? I think they're a progression, but I have concerns with modeling -fno-eliminate-unused-debug-types as part of that progression.

I don't think we should consider -fno-eliminate-unused-debug-types to be part of the progression of debug info levels via debug info kind. Users must specify -g in addition to -fno-eliminate-unused-debug-types to get any debug info, at which point they'll get both full debug info via -g and unused type debug info via -fno-eliminate-unused-debug-types.

To me, -fno-eliminate-unused-debug-types and -fno-eliminate-unused-debug-symbols seem orthogonal to debug info level. For example, how would you order -fno-eliminate-unused-debug-types vs -fno-eliminate-unused-debug-symbols in a progression? One does not imply the other; they are mutually exclusive.

I agree in a progression -fno-eliminate-unused-debug-types should come after full debug info, but then it seems weird for -fno-eliminate-unused-debug-symbols if we ever wanted to add support for that (YAGNI?) So maybe a progression isn't the best representation?


I should probably fix the documentation in this commit for -fstandalone-debug to mention this, and add documentation for this flag in the first place.


Regarding -femit-class-debug-always g++ with -femit-class-debug-always for the test case in this CL doesn't produce any debug info. Looks like clang does not currently support -femit-class-debug-always.

  • add documentation
nickdesaulniers planned changes to this revision.Jun 2 2020, 2:15 PM
nickdesaulniers added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
5344

class declared in namespace works with this patch. I'll add a test.

local type in function, ex.

void foo(void) {
  struct bar {};
}

is not in the current diff, but is supported by gcc. Let me fix that and add a test. Great suggestion.

  • handle non top level decls, add tests

Bumping for review. Are there additional reviewers we can entrust code review to, to help share the code review responsibility?

+debug-info tag

Needs a Driver test to show that the Right Thing gets passed to cc1.

clang/docs/CommandGuide/clang.rst
438

The change to the command-line reference says "defined" not "declared", these should be consistent.

clang/docs/UsersManual.rst
2357

"declared" vs "defined" again.

clang/lib/Driver/ToolChains/Clang.cpp
5190

Looks like this makes the "no" option take precedence, even if it comes before "yes" option. Should check for both in one call, so you really do take the last one:

Args.AddLastArg(CmdArgs, options::OPT_feliminate_unused_debug_types, options::OPT_fno_eliminate_unused_debug_types);
  • rebase
  • fix docs (use declare consistently, avoid define)
  • add driver test
  • fix command line check
nickdesaulniers marked 3 inline comments as done.Jun 10 2020, 3:04 PM
nickdesaulniers added a reviewer: probinson.
  • update comment to CODEGENOPT
  • avoid BooleanFFlag group
nickdesaulniers marked 2 inline comments as done.Jun 10 2020, 3:22 PM
nickdesaulniers marked 2 inline comments as done.
  • fix docs (use declare consistently, avoid define)

Defined would be the right word here (declaring a type is code like "struct foo;" - and this change probably doesn't/shouldn't emit debug info for type declarations, yes?)

clang/include/clang/Driver/Options.td
1541–1543

I /think/ @MaskRay added some kind of helper to make it easier to define the f/fno pair together, perhaps? Ah, right, mentioned in another part of this review, D80883 - though that'll only be applicable if this doesn't become a debug-info-kind.

clang/lib/CodeGen/CodeGenModule.cpp
5400–5403

Ping on this ^

5596

Ping on this ^

5598

Ping on this^

clang/lib/Frontend/CompilerInvocation.cpp
806

Are debug info kinds a progression or mutually exclusive?

Both? But yes, generally considered a progression.

I don't think we should consider -fno-eliminate-unused-debug-types to be part of the progression of debug info levels via debug info kind. Users must specify -g in addition to -fno-eliminate-unused-debug-types to get any debug info, at which point they'll get both full debug info via -g and unused type debug info via -fno-eliminate-unused-debug-types.

The same is true of the existing -fstandalone-debug (similar to GCC's -femit-class-debug-always)

To me, -fno-eliminate-unused-debug-types and -fno-eliminate-unused-debug-symbols seem orthogonal to debug info level. For example, how would you order -fno-eliminate-unused-debug-types vs -fno-eliminate-unused-debug-symbols in a progression? One does not imply the other; they are mutually exclusive.

I'd probably implement -fno-eliminate-unused-debug-symbols as a separate thing, not in that progression & would be happy to design the current feature without considering that future possibility in any case.

I should probably fix the documentation in this commit for -fstandalone-debug to mention this, and add documentation for this flag in the first place.

Mention what, sorry?

Regarding -femit-class-debug-always g++ with -femit-class-debug-always for the test case in this CL doesn't produce any debug info.

Sorry, more specifically I meant "-g -femit-class-debug-always" and "-g -fno-eliminate-unused-debug-types" and I meant on more than only this test case.

Basically: how do those two flags interact in GCC? Are they both considered separately (in the no-X and X variants) and then the "more verbose" of the two wins (ie: -femit-class-debug-always -fno-eliminate-unused-debug-types -feliminate-unused-debug-types still produces "Class debug always" behavior?)?

Looks like clang does not currently support -femit-class-debug-always.

clang/test/CodeGen/debug-info-unused-types.c
21–30

Might be a smidge easier to read without the enumerators (& this doesn't check the enumerators are in the "elements" list of the enumeration anyway) - presumably the type emission codepath is well enough tested that we only need to to briefly test that the types were emitted at all? (might even be easy enough to stop after the tag and name))

36–45

Could this be simplified down to:

NODBG-NOT: DICompositeType

?

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

Similar feedback as for the C example above

  • fix docs (use declare consistently, avoid define)

Defined would be the right word here (declaring a type is code like "struct foo;" - and this change probably doesn't/shouldn't emit debug info for type declarations, yes?)

Oh, looks like that's a difference between my current implementation and GCC. GCC does not emit debug info with -g -fno-eliminate-unused-debug-types for type declarations; the current version of my patch does. Let me fix that+add tests+update docs.

Ok, I think I have all feedback addresses locally. One issue I'm hitting is that I'm regressing clang/test/OpenMP/parallel_codegen.cpp. Specifically, it seems it has code like:

template <typename T>                                                                                                                                                                                                                                               
int tmain(T argc) {                                                                                                                                                                                                                                                 
  typedef double (*chunk_t)[argc[0][0]];
...

I'm not sure what type chunk_t is exactly, but my brain can't parse it. Function pointer that returns double (no parameter list?) array? WTF

  • rebase
  • make feature progression of debug-info-kind
  • fix docs (define, not declare)
  • add driver tests
  • use OptOutFFlag tablegen helper
  • rename helper to EmitAndRetainType
  • dont emit debug info for declarations
  • rewrite tests
nickdesaulniers planned changes to this revision.Jun 18 2020, 5:08 PM
nickdesaulniers marked 22 inline comments as done.
  • git clang-format HEAD~
dblaikie added inline comments.Jun 18 2020, 6:21 PM
clang/lib/CodeGen/CGDecl.cpp
102–115

All of these might be able to be collapsed together - using "cast<TagDecl>(D).hasDefinition()" ... to have a common implementation.

104–106

Any particular reason this one's handled differently from the others?

111

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))

clang/lib/CodeGen/CodeGenModule.cpp
5404

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.

nickdesaulniers marked an inline comment as done.
  • make cases more uniform, cleanup some missed fixes
clang/lib/CodeGen/CGDecl.cpp
102–115

TagDecl doesn't have a method hasDefinition. Only CXXRecordDecl does.

111

TagDecl doesn't have a method hasDefinition. Only CXXRecordDecl does.

clang/lib/CodeGen/CodeGenModule.cpp
5404

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?

clang/lib/CodeGen/CGDecl.cpp
102–115

Also, how to get the QualType differs between Decl::Enum an Decl::Record; getEnumType vs getRecordType respectively.

  • 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
2159

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
947

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
102–115

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.

111

Ah, fair enough!

clang/lib/CodeGen/CodeGenModule.cpp
5404

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
3834–3837

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
5404

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
3834–3837

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
5404

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
3834–3837

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
3834–3837

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
3834–3838

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
3834–3838

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
3786–3794

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

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

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

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

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