The accessibility level of a typedef or using declaration in a
struct or class was being lost when producing debug information.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
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. |
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?
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. |
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. |
llvm/test/DebugInfo/X86/debug-info-access.ll | ||
12–32 | Ah, makes sense - thanks! |
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.