Page MenuHomePhabricator

[CodeView] Don't generate dummy unnamed strcut/class/union type.
ClosedPublic

Authored by zequanwu on Nov 18 2022, 1:12 PM.

Details

Reviewers
akhuang
rnk
Summary

This aligns with MSVC's output.

Fix #57999

Diff Detail

Event Timeline

zequanwu created this revision.Nov 18 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 1:12 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
zequanwu requested review of this revision.Nov 18 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 1:12 PM
zequanwu edited the summary of this revision. (Show Details)Nov 18 2022, 1:28 PM
rnk added a subscriber: dblaikie.Nov 18 2022, 1:30 PM

Since Clang creates the member list and only adds types for the benefit of codeview, should we change clang instead?

+@dblaikie

llvm/test/DebugInfo/COFF/nested-types.ll
214 ↗(On Diff #476582)

So the test shows that we don't have NestedType records for unnamed unions & structs, right?

Since Clang creates the member list and only adds types for the benefit of codeview, should we change clang instead?

+@dblaikie

Looks like DWARF wants the types (in DWARF we describe the union and use the DWARF attribute DW_AT_export_symbols (encoded in the DICompositeType's DIFlagExportSymbols flag) to say that the names in the union are visible within the enclosing scope, but doesn't put an artificial name on those types. So probably the naming logic here https://github.com/llvm/llvm-project/blob/2e999b7dd1934a44d38c3a753460f1e5a217e9a5/clang/lib/CodeGen/CGDebugInfo.cpp#L336-L340 might be able to be removed - but I guess you probably still need it for non-nested unnamed types?

zequanwu updated this revision to Diff 476621.Nov 18 2022, 3:46 PM

Reverted previous change and change clang side so that it doesn't emit nested anonymous record type when emitting code-view.

Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 3:46 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Since Clang creates the member list and only adds types for the benefit of codeview, should we change clang instead?

Yes, that's something clang does only for codeview, changed in clang side. It will still emit type info for unnamed records but not for anonymous records.

llvm/test/DebugInfo/COFF/nested-types.ll
214 ↗(On Diff #476582)

Updated. We want NestedType records for unnamed unions & structs, but not for anonymous unions & structs. That's what MSVC does.

0x1004 | LF_FIELDLIST [size = 168]
           - LF_MEMBER [name = `i1`, Type = 0x0074 (int), offset = 0, attrs = public]
           - LF_NESTTYPE [name = `<unnamed-type-unnamed_union>`, parent = 0x1002]
           - LF_MEMBER [name = `unnamed_union`, Type = 0x1002, offset = 4, attrs = public]
           - LF_MEMBER [name = `i3`, Type = 0x0074 (int), offset = 8, attrs = public]
           - LF_NESTTYPE [name = `<unnamed-type-unnamed_struct>`, parent = 0x1003]
           - LF_MEMBER [name = `unnamed_struct`, Type = 0x1003, offset = 12, attrs = public]
rnk resigned from this revision.Nov 21 2022, 3:11 PM

Seems right to me, but I would prefer if someone else can review.

akhuang accepted this revision.Dec 5 2022, 2:51 PM
This revision is now accepted and ready to land.Dec 5 2022, 2:51 PM
zequanwu closed this revision.Dec 5 2022, 4:31 PM