llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.53k stars 11.31k forks source link

Missing Debug Info for Typedefs Inside Class Definitions #41273

Open ormris opened 5 years ago

ormris commented 5 years ago
Bugzilla Link 41928
Version trunk
OS Linux
CC @dwblaikie,@DougGregor,@jmorse,@pogo59,@zygoloid,@rnk,@wjristow

Extended Description

No debug info is currently emitted for typedefs nested inside of class definitions. For example:

$ cat test.cpp
struct A {
    int a;
    A(int a) : a(a) {}
};
struct B : A {
    using BA = A;
    int b;
    B(int b1, int b2) : BA(b1), b(b2) {}
};
int main() {
    B b{1,2};
    return b.b + b.a;
}
$ clang --version | grep version
clang version 9.0.0 (https://github.com/llvm/llvm-project.git f672b6170ce...
$ clang -g -S -emit-llvm test.cpp
$ grep BA test.ll
$
dwblaikie commented 5 years ago

This surprised me as well. We ended up needing this for codeview, so we added it here: ...

Easy enough to make that not codeview-specific.

Though I wouldn't necessarily encourage it. Or at least not without some size impact analysis.

I think it would be reasonable to get size numbers just by re-building LLVM with DWARF and comparing obj size and linked binary size.

Yep.

As for the below cases. My general take on them is that if a consumer has to search for templates - it's already got the logic to search & can use it for other things too.

If we are going to add those extra optional things (pending size analysis) - we might want some sort of attribute on the type or the CU that says "hey, we made sure all these optional things are there, so if you don't find it here, don't go searching for it elsewhere - we guarantee you won't find it anywhere else". Otherwise it's of limited value - the consumer can't short-circuit the searches because they might find it elsewhere.

1) Nested classes (as above, but especially if the nested class is not defined in all translation units - so one TU will have "struct foo { struct bar; }; struct foo::bar { ... };" and another just has the declaration of bar - LLVM's current DWARF emission doesn't render this as an out of line class definition - just one translation unit has a 'bar' definition nested inside the 'foo' definition, and one does not)

This isn't an issue for codeview, all we record is that there is a name 'bar' inside 'class foo' that refers to some other type, which is emitted following the normal -f[no-]standalone logic. Codeview type records don't nest, they just refer to each other by index, so there's no inconsistency.

Would we want to do the same thing for DWARF? So, emit DWARF that looks like the user wrote:

struct foo { struct bar; ... }; struct foo::bar { ... }; // only present if required or standalone

Yeah, if it's necessary/worthwhile - we should probably always model it this way (same way "struct foo { ... }; foo *f;" we still model as a declaration of foo (non-standalone), even though there was never a pure declaration of foo in the source - and similarly for templates (where you basically /have/ to model them as declarations if the definition hasn't been instantiated, etc))

2) implicit special members

I believe it should be possible to test for the presence or absence of all non-templated special members, which I've been wanting to do for some time. I think for CodeView we just have bugs here, and we emit different types definitions in different files depending on which implicit special members get defined where. It would be good to fix this, and I think the same fixes would apply equally well to DWARF (ignoring templated special members).

Yep, certainly seems possible - just not sure if it's especially beneficial.

3) This is the unavoidable one: Member template instantiations (member

This is the big one, and we found that MSVC simply doesn't list member templates (of any kind, function, variable, class) as class members, so we don't either.

So I guess the template specializations still list the class as their enclosing scope? But the class doesn't list the template specializations in its member list?

DWARF being hierarchical doesn't have a separation there - but the LLVM IR encoding does have that separation & that's what we use for theis optional things - we don't put them in the DICompositeType's member list, but we do list the DICompositeType as their enclosing scope.

rnk commented 5 years ago

This surprised me as well. We ended up needing this for codeview, so we added it here: ...

Easy enough to make that not codeview-specific.

Though I wouldn't necessarily encourage it. Or at least not without some size impact analysis.

I think it would be reasonable to get size numbers just by re-building LLVM with DWARF and comparing obj size and linked binary size.

1) Nested classes (as above, but especially if the nested class is not defined in all translation units - so one TU will have "struct foo { struct bar; }; struct foo::bar { ... };" and another just has the declaration of bar - LLVM's current DWARF emission doesn't render this as an out of line class definition - just one translation unit has a 'bar' definition nested inside the 'foo' definition, and one does not)

This isn't an issue for codeview, all we record is that there is a name 'bar' inside 'class foo' that refers to some other type, which is emitted following the normal -f[no-]standalone logic. Codeview type records don't nest, they just refer to each other by index, so there's no inconsistency.

Would we want to do the same thing for DWARF? So, emit DWARF that looks like the user wrote:

struct foo { struct bar; ... }; struct foo::bar { ... }; // only present if required or standalone

2) implicit special members

I believe it should be possible to test for the presence or absence of all non-templated special members, which I've been wanting to do for some time. I think for CodeView we just have bugs here, and we emit different types definitions in different files depending on which implicit special members get defined where. It would be good to fix this, and I think the same fixes would apply equally well to DWARF (ignoring templated special members).

3) This is the unavoidable one: Member template instantiations (member

This is the big one, and we found that MSVC simply doesn't list member templates (of any kind, function, variable, class) as class members, so we don't either.

dwblaikie commented 5 years ago

This surprised me as well. We ended up needing this for codeview, so we added it here: ...

Easy enough to make that not codeview-specific.

Though I wouldn't necessarily encourage it. Or at least not without some size impact analysis.

This behavior is, I think, consistent with GCC - so DWARF consumers are probably used to coping with it at least.

I guess -fno-standalone-debug would probably limit the impact a fair bit - ensuring that complex types used in typedefs don't pull in huge new DWARF graphs, but terminate relatively quickly if the typedef/types it references are unused.

(& this CodeView change accounts for other forms of nested types too, by the looks of it, like this: "struct foo { struct bar; };" - currently DWARF for 'foo' has no mention of 'bar' if bar is unused)

But classes are necessarily open/potentially never fully enumerated - through 3 means, one of which is unavoidable:

1) Nested classes (as above, but especially if the nested class is not defined in all translation units - so one TU will have "struct foo { struct bar; }; struct foo::bar { ... };" and another just has the declaration of bar - LLVM's current DWARF emission doesn't render this as an out of line class definition - just one translation unit has a 'bar' definition nested inside the 'foo' definition, and one does not)

2) implicit special members - Clang doesn't pre-emptively test for the validity of all special members and include declarations of them in the class descriptions in debug info (perhaps, again, there's some support for this for CodeVieW? Not sure). So in TUs that end up calling, say, the implicit copy ctor - there's a DWARF subprogram declaration/definition/etc for that - but where it isn't called, it isn't described by DWARF

3) This is the unavoidable one: Member template instantiations (member function templates, nested class templates, static member variable templates, etc) - since DWARF doesn't have an abstraction over the template itself, the only thing is to put the template specializations/instantiations into the class directly (eg: "struct foo { void bar(T t) { ... } void bar(T t) { ... } }" etc... ).

And we've generally made the determination that skipping names is acceptable - it's unreasonable to emit all (even unreferenced) names across the source code - so name lookups based on DWARF will always have the possibility of returning things the user didn't intend (because they should've been shadowed by some other entity with the same name that was unused in the source)

All that is: If there's a use for this, sure - measure the impact and implement it if it seems OK. But "this is missing" isn't a strong justification in my mind at least.

pogo59 commented 5 years ago

This surprised me as well. We ended up needing this for codeview, so we added it here: ...

Easy enough to make that not codeview-specific.

rnk commented 5 years ago

This surprised me as well. We ended up needing this for codeview, so we added it here: https://github.com/llvm/llvm-project/blob/c72aaf62d3f92c0c6d33b4df2253505f6eb22996/clang/lib/CodeGen/CGDebugInfo.cpp#L1417

llvmbot commented 1 month ago

@llvm/issue-subscribers-debuginfo

Author: Matthew Voss (ormris)

| | | | --- | --- | | Bugzilla Link | [41928](https://llvm.org/bz41928) | | Version | trunk | | OS | Linux | | CC | @dwblaikie,@DougGregor,@jmorse,@pogo59,@zygoloid,@rnk,@wjristow | ## Extended Description No debug info is currently emitted for typedefs nested inside of class definitions. For example: $ cat test.cpp struct A { int a; A(int a) : a(a) {} }; struct B : A { using BA = A; int b; B(int b1, int b2) : BA(b1), b(b2) {} }; int main() { B b{1,2}; return b.b + b.a; } $ clang --version | grep version clang version 9.0.0 (https://github.com/llvm/llvm-project.git f672b6170ce... $ clang -g -S -emit-llvm test.cpp $ grep BA test.ll $
jmorse commented 1 month ago

NB, this replicates with todays main 00c622e596f918. @OCHyams recently added support for DW_TAG_typedef IIRC so maybe it'll be easier to solve.

Michael137 commented 1 month ago

Funnily enough, @dwblaikie and I were talking about this just yesterday (and this came up with @adrian-prantl in the past before too). In some of LLDB's STL data-formatters it would be nice if we could rely on class-level typedefs being consistently emitted (even if unused). Also some of our users internally would like typedefs to stick around so they can be referenced in LLDB's expression evaluator. Was about to try and measure how much of a debug-info size increase we would incur if we just emitted them unconditionally, regardless of how they're used in the source.

Endilll commented 1 month ago

Consistently emitting typedefs would also help with formatters for Clang.

Michael137 commented 1 month ago

for a local clang build, emitting all class-level typedefs unconditionally:

$ bloaty `find lib -name *.o`                      
    FILE SIZE        VM SIZE                       
 --------------  --------------                    
  55.0%  1.84Gi  60.5%  1.84Gi    ,__debug_str     
  19.7%   674Mi  21.6%   674Mi    ,__debug_info    
   6.0%   205Mi   0.0%       0    String Table     
   4.2%   145Mi   4.7%   145Mi    ,__text          
   3.9%   133Mi   4.3%   133Mi    ,__debug_names   
   3.2%   108Mi   3.5%   108Mi    ,__debug_str_offs
   2.1%  70.5Mi   0.0%       0    [Unmapped]       
   1.8%  63.2Mi   2.0%  63.2Mi    ,__debug_line    
   1.6%  56.6Mi   1.8%  56.6Mi    ,__compact_unwind
   1.0%  34.2Mi   0.0%       0    Symbol Table     
   0.5%  17.0Mi   0.5%  17.0Mi    ,__debug_addr    
   0.3%  10.9Mi   0.3%  10.9Mi    ,__debug_line_str
   0.2%  8.47Mi   0.3%  8.47Mi    ,__cstring       
   0.2%  6.78Mi   0.2%  6.78Mi    ,__debug_abbrev  
   0.2%  5.96Mi   0.2%  5.96Mi    ,__const         
   0.1%  2.02Mi   0.0%       0    [Mach-O Headers] 
   0.0%   526Ki   0.0%   526Ki    ,__debug_rnglists
   0.0%       0   0.0%   428Ki    ,__bss           
   0.0%   407Ki   0.0%   407Ki    ,__StaticInit    
   0.0%   319Ki   0.0%   319Ki    ,__data          
   0.0%  16.9Ki   0.0%  49.7Ki    [10 Others]      
 100.0%  3.35Gi 100.0%  3.05Gi    TOTAL            

versuse baseline,

$ bloaty `find lib -name *.o`
    FILE SIZE        VM SIZE    
 --------------  -------------- 
  55.4%  1.83Gi  61.1%  1.83Gi    ,__debug_str
  18.8%   633Mi  20.7%   633Mi    ,__debug_info
   6.1%   205Mi   0.0%       0    String Table
   4.3%   145Mi   4.7%   145Mi    ,__text
   3.8%   127Mi   4.2%   127Mi    ,__debug_names
   3.2%   107Mi   3.5%   107Mi    ,__debug_str_offs
   2.1%  70.5Mi   0.0%       0    [Unmapped]
   1.9%  63.2Mi   2.1%  63.2Mi    ,__debug_line
   1.7%  56.6Mi   1.8%  56.6Mi    ,__compact_unwind
   1.0%  34.2Mi   0.0%       0    Symbol Table
   0.5%  17.8Mi   0.6%  17.8Mi    ,__debug_line_str
   0.5%  17.0Mi   0.6%  17.0Mi    ,__debug_addr
   0.3%  8.51Mi   0.3%  8.51Mi    ,__cstring
   0.2%  6.43Mi   0.2%  6.43Mi    ,__debug_abbrev
   0.2%  5.96Mi   0.2%  5.96Mi    ,__const
   0.1%  2.02Mi   0.0%       0    [Mach-O Headers]
   0.0%   526Ki   0.0%   526Ki    ,__debug_rnglists
   0.0%       0   0.0%   428Ki    ,__bss
   0.0%   407Ki   0.0%   407Ki    ,__StaticInit
   0.0%   319Ki   0.0%   319Ki    ,__data
   0.0%  16.9Ki   0.0%  49.7Ki    [10 Others]
 100.0%  3.29Gi 100.0%  2.99Gi    TOTAL

So a ~45 MB increase .debug_info section.

llvmbot commented 1 month ago

@llvm/issue-subscribers-clang-codegen

Author: Matthew Voss (ormris)

| | | | --- | --- | | Bugzilla Link | [41928](https://llvm.org/bz41928) | | Version | trunk | | OS | Linux | | CC | @dwblaikie,@DougGregor,@jmorse,@pogo59,@zygoloid,@rnk,@wjristow | ## Extended Description No debug info is currently emitted for typedefs nested inside of class definitions. For example: ``` $ cat test.cpp struct A { int a; A(int a) : a(a) {} }; struct B : A { using BA = A; int b; B(int b1, int b2) : BA(b1), b(b2) {} }; int main() { B b{1,2}; return b.b + b.a; } $ clang --version | grep version clang version 9.0.0 (https://github.com/llvm/llvm-project.git f672b6170ce... $ clang -g -S -emit-llvm test.cpp $ grep BA test.ll $ ```
dwblaikie commented 1 month ago

Doesn't sound /so/ bad...

(btw, bloaty has a comparison mode which might be easier to read - if you run bloaty new_file -- old_file it'll compare section sizes so you don't have to eyeball them manually. And for file size comparisons, adding --domain=file will remove the vm address space part of the report, which isn't relevant)

Michael137 commented 1 month ago

Doesn't sound /so/ bad...

(btw, bloaty has a comparison mode which might be easier to read - if you run bloaty new_file -- old_file it'll compare section sizes so you don't have to eyeball them manually. And for file size comparisons, adding --domain=file will remove the vm address space part of the report, which isn't relevant)

Nice! Re-ran:

$ bloaty --domain=file  `find ./lib -name *.o`  --  `find ../baseline/lib -name *.o`                                          
    FILE SIZE   
 -------------- 
  +6.6% +41.9Mi    ,__debug_info
  +0.9% +16.6Mi    ,__debug_str
  +4.7% +6.05Mi    ,__debug_names
  +1.0% +1.08Mi    ,__debug_str_offs
  +5.4%  +359Ki    ,__debug_abbrev
  +0.1% +76.1Ki    ,__debug_line
  +0.0%     +91    ,__cstring
  +0.0%     +45    ,__debug_addr
  -0.0%    -235    [Unmapped]
 -44.7% -8.77Mi    ,__debug_line_str
  +1.7% +57.3Mi    TOTAL

My main motivator here is to make sure the LLDB STL formatters are able to access typedefs that are currently not emitted -- and also make sure that the typedefs that LLDB currently uses won't disappear because they become unused...though not sure how often this would really happen in practice). One example of a typedef that's currently not emitted is __hash_iterator::__node_pointer. We could of course try making source changes that ensure the typedef is used, though that feels a bit brittle (and perhaps will get some pushback from the libc++ folks). Also, from a different thread with @dwblaikie, there's currently a bug where we incorrectly scope some using aliases: https://godbolt.org/z/918h6rb6K (probably should split this out into a separate issue). Another alternative that was floated is to support something like a [[used]] on typedefs that we want to have debug-info for.

dwblaikie commented 1 month ago

6/5% increase to debug_info and debug_names seems a bit rough - I'd say maybe too expensive.

As for changes to to source code and possibl epushback from libc++ - think we should entertain/have that discussion, rather than avoid that path because the discussion /might/ be difficult.

I had/have a similar issue with an internal data structure and pretty printer missing a nested type I think... (though I've been juggling a few too many things, so not sure which type I was looking at) - oh, nope, that was related to a nested static constexpr variable, different problem & different solution/problems.

So, nested typedef. The easiest thing would be an unused nested function or static variable that references the typedef... try to find the most compact option in terms of DWARF cost.

An attribute is a somewhat difficult conversation with clang folks, but sort of feels like a pretty reasonable one. In some ways I liked that we named [[standalone_debug]] the same as the flag, and can explain it as "treat this type the same as -fstandalone-debug would" but means it'd be unsuitable to apply in this "unused typedef" case & mean introducing a new name, maybe reuseing some other name like [[used]] but that probably confuses things a bit much between its other uses/meanings and the debug info meaning... but maybe other folks disagree - I'm certainly not confident.

Endilll commented 1 month ago

Sometimes typedefs are required to write formatters, but even when they are not, the workaround often is "Nth template argument", which is less maintainable, because it doesn't have a name.

[[standalone_debug]] takes us only so far. Users would either need to track which typedefs are not emitted in the debug info, and mark the rest with such attribute, or (more scalable and maintainable) put it on every single typedef just to be safe. At that point forgetting about the attribute and using -fstandalone-debug would make more sense.

While working on formatters for Clang and LLVM ADT types, I basically had to partially reimplement them in a debugger plugin to interpret their memory correctly, and having access to names that original code can access is very helpful, at times even essential.

dwblaikie commented 1 month ago

Sometimes typedefs are required to write formatters, but even when they are not, the workaround often is "Nth template argument", which is less maintainable, because it doesn't have a name.

[[standalone_debug]] takes us only so far. Users would either need to track which typedefs are not emitted in the debug info, and mark the rest with such attribute, or (more scalable and maintainable) put it on every single typedef just to be safe. At that point forgetting about the attribute and using -fstandalone-debug would make more sense.

Every typedef they need for a pretty printer is probably still a lot less debug info than -fstandalone-debug produces. (& standalone-debug doesn't actually help here, as such - it doesn't produce unreferenced entities, it mostly just makes entities that would already be emitted as a declaration in the DWARF, get emitted as a definition - my comment about standalone-debug was intended to be a discussion about what to name an attribute that would force a member typedef to be emitted into the DWARF when the outer class was emitted)

(oh, and reminds me that @Michael137's numbers are probably from Apple, which has standalone-debug enabled by default, and probably need to measure on Linux too - since it'd have different tradeoffs (maybe worse, maybe better))

While working on formatters for Clang and LLVM ADT types, I basically had to partially reimplement them in a debugger plugin to interpret their memory correctly, and having access to names that original code can access is very helpful, at times even essential.

Indeed, sorry - not meaning to suggest this information is never needed, but that it might be too expensive to emit in /all/ cases, and requiring some code changes might be a suitable tradeoff on the size/convenience spectrum.