This is an archive of the discontinued LLVM Phabricator instance.

[clang][DebugInfo] Emit DW_AT_type of preferred name if available
ClosedPublic

Authored by Michael137 on Mar 10 2023, 8:56 AM.

Details

Summary

With this patch, whenever we emit a DW_AT_type for some declaration
and the type is a template class with a clang::PreferredNameAttr, we
will emit the typedef that the attribute refers to instead. I.e.,

0x123 DW_TAG_variable
DW_AT_name "var"
DW_AT_type (0x123 "basic_string<char>")

0x124 DW_TAG_structure_type
DW_AT_name "basic_string<char>"

...becomes

0x123 DW_TAG_variable
DW_AT_name "var"
DW_AT_type (0x124 "std::string")

0x124 DW_TAG_structure_type
DW_AT_name "basic_string<char>"

0x125 DW_TAG_typedef
DW_AT_name "std::string"
DW_AT_type (0x124 "basic_string<char>")

We do this by returning the preferred name typedef DIType when
we create a structure definition. In some cases, e.g., with -gmodules,
we don't complete the structure definition immediately but do so later
via completeClassData, which overwrites the TypeCache. In such cases
we don't actually want to rewrite the cache with the preferred name. We
handle this by returning both the definition and the preferred typedef
from CreateTypeDefinition and let the callee decide what to do with
it.

Essentially we set up the types as:

TypeCache[Record] => DICompositeType
ReplaceMap[Record] => DIDerivedType(baseType: DICompositeType)

For now we keep this behind LLDB tuning.

Testing

  • Added clang unit-test
  • check-llvm, check-clang pass
  • Confirmed that this change correctly repoints

basic_string references in some of my test programs.

  • Will add follow-up LLDB API tests

Diff Detail

Event Timeline

Michael137 created this revision.Mar 10 2023, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 8:56 AM

Yet another alternative for emitting preferred_names.

Makes the attribute introduced in https://reviews.llvm.org/D145077 redundant. Also doesn't require any changes to LLDB (making https://reviews.llvm.org/D145078 redundant, which got a bit hairy)

This seems to be a much simpler implementation that just automatically works. I think I'd prefer that over adding new DWARF attributes.

  • Update commit message
Michael137 published this revision for review.Mar 10 2023, 1:59 PM
Michael137 edited the summary of this revision. (Show Details)
Michael137 added a reviewer: dblaikie.
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 1:59 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Yeah, can't say this had occurred to me - but totally makes sense/reckon it's OK. Any reason to limit this to lldb? I'd expect it'd probably "Just Work(tm)" on any DWARF consumer?

it doesn't hit any recursion issues? (I guess maybe skirts it due to the existing recursion handling in the decl/def structure type stuff - so it creates a declaration for the type, then creates the typedef, which can find that existing declaration, then return the typedef from the create type query?)

I guess it means that references to the type even for like, the type of the "this" parameter - would refer to the typedef? That's /probably/ OK if a bit surprising to some people/consumers sometimes?

Yeah, can't say this had occurred to me - but totally makes sense/reckon it's OK. Any reason to limit this to lldb? I'd expect it'd probably "Just Work(tm)" on any DWARF consumer?

No particular reason other than being on the safe side and get field experience before enabling it for all consumers. But I agree, I don't see why this couldn't be enabled always.

it doesn't hit any recursion issues? (I guess maybe skirts it due to the existing recursion handling in the decl/def structure type stuff - so it creates a declaration for the type, then creates the typedef, which can find that existing declaration, then return the typedef from the create type query?)

Yup that's the intention

I guess it means that references to the type even for like, the type of the "this" parameter - would refer to the typedef? That's /probably/ OK if a bit surprising to some people/consumers sometimes?

Good point, it does also repoint the this pointer to the typedef. LLDB seems to handle this fine. Can add some test cases for this (in Clang and LLDB)

Apparently some Objective-C/gmodules LLDB tests aren't happy with this change
Investigating...

  • Fix cycle in DIDerivedType when we deal with forward declarations and complete explicitly via completeClassData (e.g., with -gmodules)
  • Add tests for -gmodules
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2023, 7:59 AM
Michael137 edited the summary of this revision. (Show Details)Mar 20 2023, 8:00 AM
aprantl added inline comments.Mar 21 2023, 9:15 AM
clang/lib/CodeGen/CGDebugInfo.cpp
2717

I'm curious if this works if we encounter a forward declaration, early exit here, then encounter a use of the type, and then the definition, since completeClass effectively also ignores the preferred name?

Michael137 added inline comments.Mar 23 2023, 10:54 AM
clang/lib/CodeGen/CGDebugInfo.cpp
2717

Good point. If we ever take this branch we won't end up emitting the preferred name. For my -gmodules test cases this works out fine because the modules that contain the instantiations would see the preferred name. But I can indeed construct a non-modules test case where we end up using a forward declared structure where the preferred name gets ignored here. Not sure we can do much here for such cases.

The alternative I considered where we create some sort of PreferredNameCache[TagType*] => DIDerivedType and use it when replacing forward declarations in finalize() doesn't work for the normal case because we don't have any forward declarations to replace

This is pretty different from the "always desugar to the canonical type" habit that has always been in place. Sony has done some downstream things to try to work around that in the past. @wolfgangp will remember it better than I do, but I think we make some effort to preserve the type-as-written. This goes in completely the other direction.

clang/test/CodeGen/preferred_name.cpp
50

This doesn't become Foo<BarInt> ?

This is pretty different from the "always desugar to the canonical type" habit that has always been in place. Sony has done some downstream things to try to work around that in the past. @wolfgangp will remember it better than I do, but I think we make some effort to preserve the type-as-written. This goes in completely the other direction.

The Sony solution does not rely on a user-specified attribute, but uniformly preserves all typedefs, though only in template argument types. Similar to this solution, it adds a second type to keep track of the preferred type, though it does so in the TemplateArgument structure as a separate QualType. A client can then either print the preferred type or the canonical type, depending on a printing policy, which is controlled by an option. This leads to a similar result in the resulting DWARF, i.e. the DW_AT_type node gets the preferred type string. The preferred type can also be used for diagnostic messages.

clang/lib/CodeGen/CGDebugInfo.h
289

This comment seems a bit garbled. Also, this is just a helper function, no?

Michael137 added inline comments.Mar 26 2023, 2:06 PM
clang/test/CodeGen/preferred_name.cpp
50

The name stays as Foo<Foo<int>> but the type of the template parameter becomes BarInt. So the dwarf would look like:

DW_TAG_structure_type
  DW_AT_name      ("Foo<Foo<int> >")

  DW_TAG_template_type_parameter
    DW_AT_type    (0x00000095 "BarInt")
    DW_AT_name    ("T")

Will add the parameter metadata to the test

  • Refine tests:
    • Check for type of template parameter
    • Add test for chained typedefs.
Michael137 marked an inline comment as done.Mar 27 2023, 3:47 AM
  • Fix function docstring
Michael137 marked an inline comment as done.Mar 27 2023, 3:49 AM
dblaikie added inline comments.Mar 27 2023, 11:38 AM
clang/test/CodeGen/preferred_name.cpp
50

Hmm, that seems buggy - why doesn't Foo<Foo<int> > become Foo<BarInt>? (is the preferred name ever used in the DW_AT_name? I'd have thought it should be unless it's explicitly avoided to ensure type compatibility/collapsing with debug info built without this feature enabled?)

Michael137 marked an inline comment as not done.Mar 27 2023, 11:50 AM
Michael137 added inline comments.
clang/test/CodeGen/preferred_name.cpp
50

I suspect it's because the name is constructed using the clang TypePrinter. And we turn off PrintingPolicy::UsePreferredNames by default to avoid divergence from GCC.

Will double check though

dblaikie added inline comments.Mar 27 2023, 4:30 PM
clang/test/CodeGen/preferred_name.cpp
50

Yeah, that sounds right/OK to me. Sorry for the diversion.

Michael137 added a comment.EditedMar 28 2023, 2:04 PM

This is pretty different from the "always desugar to the canonical type" habit that has always been in place. Sony has done some downstream things to try to work around that in the past. @wolfgangp will remember it better than I do, but I think we make some effort to preserve the type-as-written. This goes in completely the other direction.

The Sony solution does not rely on a user-specified attribute, but uniformly preserves all typedefs, though only in template argument types. Similar to this solution, it adds a second type to keep track of the preferred type, though it does so in the TemplateArgument structure as a separate QualType. A client can then either print the preferred type or the canonical type, depending on a printing policy, which is controlled by an option. This leads to a similar result in the resulting DWARF, i.e. the DW_AT_type node gets the preferred type string. The preferred type can also be used for diagnostic messages.

Neat, thanks for outlining your approach.

@probinson Sounds like Sony's solution also changes the DW_AT_type to a non-canonical form. Do you still have concerns with the direction of this patch? Would it cause any problems for you downstream?

@probinson Sounds like Sony's solution also changes the DW_AT_type to a non-canonical form. Do you still have concerns with the direction of this patch? Would it cause any problems for you downstream?

Sorry for the late reply. Looking at the patch I don't think it will cause us any trouble. The 2 solutions should be able to live happily side by side.

I'll submit later today then if nobody has any other objections

Will add a release note

This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2023, 5:38 PM
This revision was automatically updated to reflect the committed changes.
chapuni added a subscriber: chapuni.Apr 9 2023, 1:20 AM
chapuni added inline comments.
clang/test/Modules/gmodules-preferred-name-alias.cpp
6

I guess it made difficult to find out the actual error since it hid stderr.
In fact, I had to reproduce failures locally.

In addition, mixing stdout and stderr sometimes confuse line order due to buffering.

btw, did it really require stderr?

The changes in this patch seem to have caused https://lab.llvm.org/buildbot/#/builders/clang-ppc64-aix to go down for the past 4 days -- can you revert and investigate?

Michael137 added a comment.EditedApr 10 2023, 7:22 AM

Looks like gmodules isn't fully supported on AIX (based on other tests that use -dwarf-ext-refs. So I'll just disable the new tests

Thanks for notifying @aaron.ballman , email notifications got lost in the inbox

Oh looks like @Jake-Egan already did