This is an archive of the discontinued LLVM Phabricator instance.

[llvm][DebugInfo] Introduce new DW_AT_LLVM_preferred_name attribute
AcceptedPublic

Authored by Michael137 on Mar 1 2023, 8:53 AM.

Details

Reviewers
aprantl
dblaikie
Summary

This patch adds a new field to DIComposite metadata that
holds the type of the preferred name for a structure.

This will be used to encode the [[clang::preferred_name]]
attribute into DWARF.

Diff Detail

Event Timeline

Michael137 created this revision.Mar 1 2023, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2023, 8:53 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Michael137 requested review of this revision.Mar 1 2023, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2023, 8:53 AM
Michael137 updated this revision to Diff 501543.Mar 1 2023, 9:10 AM
  • clang-format
aprantl requested changes to this revision.Mar 1 2023, 11:17 AM

Looks pretty good, some nitpicks inside.

llvm/include/llvm/BinaryFormat/Dwarf.def
609

This should be an LLVM attribute. There's no reason why it wouldn't work on FreeBSD which also uses clang+lldb, right?

Can you document this attribute in LangRef.rst or SourceLevelDebugging.rst?

llvm/include/llvm/IR/DebugInfoMetadata.h
1304

Is this actually needed?

llvm/test/Bitcode/attr-preferred_name-dicomposite.ll
1 ↗(On Diff #501543)

this shouldn't be needed.

2 ↗(On Diff #501543)

I think this should be in the IR subdir and do a double-roundtrip (as|dis|as|dis). See the other tests for examples.

4 ↗(On Diff #501543)

I think we also want a test/DebugInfo test that checks dwarfdump output?

This revision now requires changes to proceed.Mar 1 2023, 11:17 AM
Michael137 added inline comments.Mar 1 2023, 11:25 AM
llvm/include/llvm/IR/DebugInfoMetadata.h
1304

Yup, it's used in CGDebugInfo to attach the attribute on a structure definition

llvm/test/Bitcode/attr-preferred_name-dicomposite.ll
4 ↗(On Diff #501543)

Yup, will add

Michael137 updated this revision to Diff 501839.Mar 2 2023, 5:39 AM
  • Add dwarfdump test
Michael137 marked 2 inline comments as done.Mar 2 2023, 5:40 AM
Michael137 marked an inline comment as done.
Michael137 added inline comments.
llvm/test/Bitcode/attr-preferred_name-dicomposite.ll
2 ↗(On Diff #501543)

I don't see an IR subdir. Also the other tests in this directory only do a single as|dis. Could you point me to the ones you are referring to?

Michael137 updated this revision to Diff 501840.Mar 2 2023, 5:43 AM
  • Change vendor to LLVM

(FWIW, reckon this is simple/easy enough to add to DWARF/worth filing DWARF standard issue for - at least once the current DWARF committee turmoil settles out.

Not sure if there's been any discussion with the GNU side of the world about them implementing/adopting/using a source attribute like this.

But, equally, pretty cheap to give this a while & see how it plays out/if it's as valuable/convenient as we hope it is before standardizing. No great cost to getting more implementation experience)

Michael137 updated this revision to Diff 502082.Mar 3 2023, 2:27 AM
  • Double-roundtrip in test

(FWIW, reckon this is simple/easy enough to add to DWARF/worth filing DWARF standard issue for - at least once the current DWARF committee turmoil settles out.

Not sure if there's been any discussion with the GNU side of the world about them implementing/adopting/using a source attribute like this.

But, equally, pretty cheap to give this a while & see how it plays out/if it's as valuable/convenient as we hope it is before standardizing. No great cost to getting more implementation experience)

That's a good idea. I'll file one once this lands.

aprantl accepted this revision.Mar 3 2023, 8:25 AM

Looking good!

This revision is now accepted and ready to land.Mar 3 2023, 8:25 AM

You don't want to use DW_AT_description for this?

You don't want to use DW_AT_description for this?

We were considering this. It does fit the use-case nicely by the sound of it. But for LLDB it was much more convenient for the preferred name to be encoded as a reference to the DIE, not a raw string (I haven't actually tried implementing it purely based on the name of the type. We'd have to do a lookup into the accelerator tables by name at the very least I guess)