This is an archive of the discontinued LLVM Phabricator instance.

[clang][llvm] generate accessibility metadata for type aliases
ClosedPublic

Authored by J-Camilleri on Sep 21 2022, 1:51 AM.

Details

Summary

The accessibility level of a typedef or using declaration in a
struct or class was being lost when producing debug information.

Diff Detail

Event Timeline

J-Camilleri created this revision.Sep 21 2022, 1:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2022, 1:51 AM
J-Camilleri requested review of this revision.Sep 21 2022, 1:51 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 21 2022, 1:51 AM

The premerge checks have failed but the only message I can see is HTTP 28 in the log. I am not sure where to look for more information about the failure, Can someone please point me in the right direction?

dexonsmith resigned from this revision.Sep 21 2022, 7:12 AM
dexonsmith added a reviewer: aprantl.

Looks pretty good to me - few things to clean up/simplify.

clang/lib/CodeGen/CGDebugInfo.cpp
1331–1333 ↗(On Diff #461813)
clang/test/CodeGenCXX/debug-info-access.cpp
23–27 ↗(On Diff #461813)

Arguably/I'd be fine with testing just one case here - since the implementation of choosing when to put an access modifier, which modifier to put, etc, is is already implemented and tested for other cases - so we might not need to duplicate that testing for this case.

But I don't mind too much either way.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
793–795

This piece and the llvm test case should go in a separate commit (it can be committed before or after the clang-centric commit (probably before makes more sense)) but we can review it all together.

llvm/lib/IR/DIBuilder.cpp
351 ↗(On Diff #461813)

While we usually split llvm & clang commits, in this case since it changes an API used by clang, this part of the llvm change should go along with the clang change.

llvm/test/DebugInfo/X86/debug-info-access.ll
12–32

Probably don't need all this test coverage - since most of this is motivated by the features up in clang - on the LLVM side we just need one case that demonstrates that if we put an access specifier in the flags, that gets emitted.

Addressing reviewer comments: Comply to offical style, Simplify test cases

J-Camilleri marked 3 inline comments as done.Sep 22 2022, 12:22 AM
J-Camilleri added inline comments.
clang/test/CodeGenCXX/debug-info-access.cpp
23–27 ↗(On Diff #461813)

I agree with your assesment. I changed the test to just check that type aliases emit the flag not the logic of which flag is emitted.

llvm/lib/IR/DIBuilder.cpp
351 ↗(On Diff #461813)

Regarding this and the previous comment about separating the llvm commit.

Am I to have 2 commits as follows?

  1. change llvm to emit flag + test case
  2. modify llvm interface + change clang to emit flag + test case

I do not have merge rights so I will ask for this when I get to the merge stage.

Thanks for the review.

llvm/test/DebugInfo/X86/debug-info-access.ll
12–32

I left the llvm to be the same as generated by the cpp file but changed the CHECK to just verify that the accessibility flag is emitted for a typedef.

dblaikie accepted this revision.Sep 22 2022, 9:37 AM

I'll commit this shortly.

llvm/lib/IR/DIBuilder.cpp
351 ↗(On Diff #461813)

If you don't have commit rights, no worries - I can sort this out when I commit this on your behalf.
But yes, your list of the two steps/order/pieces is correct.

llvm/test/DebugInfo/X86/debug-info-access.ll
12–32

Ah, makes sense - thanks!

This revision is now accepted and ready to land.Sep 22 2022, 9:37 AM
This revision was landed with ongoing or failed builds.Sep 22 2022, 10:08 AM
This revision was automatically updated to reflect the committed changes.
J-Camilleri marked 2 inline comments as done.Sep 22 2022, 11:59 AM

I'll commit this shortly.

Ok, thanks for your time.