[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez added a comment. In D144870#4225437 , @probinson wrote: > One entirely optional suggestion on the test. LGTM. Thanks! I used `SECONDDUP` and `FIRSTDUP`. And thanks a lot for the reviews! Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

This revision was automatically updated to reflect the committed changes. Closed by commit rG488185cca387: [Clang][DebugInfo][AMDGPU] Emit zero size bitfields in the debug info to… (authored by jmmartinez). Changed prior to commit: https://reviews.llvm.org/D144870?vs=508538=508926#toc

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. One entirely optional suggestion on the test. LGTM. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1563 + + assert(PreviousBitfield->isBitField()); +

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1563 + + assert(PreviousBitfield->isBitField()); + probinson wrote: > Is this true for cases like > ``` > struct nonadjacent { > char a : 8; > char : 0; > int b; > char d :

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez updated this revision to Diff 508538. jmmartinez marked 3 inline comments as done. jmmartinez added a comment. - Took remarks into account Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144870/new/ https://reviews.llvm.org/D144870

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

probinson added a comment. Still one question, and haven't dug into the test in detail yet. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1552 + + auto *PreviousMDEntry = + PreviousFieldsDI.empty() ? nullptr : PreviousFieldsDI.back(); Maybe a comment

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez updated this revision to Diff 506611. jmmartinez added a comment. - Updated to use `elements` array to avoid the complicated loop. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144870/new/ https://reviews.llvm.org/D144870 Files:

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez added a comment. In D144870#4202121 , @probinson wrote: > (Of course it's possible that finding the preceding field can be done only by > iterating, but I'd hope that isn't necessary.) Sadly, this is the case. `field_iterator` and the

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

probinson added a comment. Is it possible you need to look only at the immediately preceding field, and not iterate? For example, struct zero_bitfield { char a : 8; char : 0; char b : 8; char c : 8; }; If processing `b` sees the zero-length bitfield and does the needful,

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1558 + EmitSeparator = FieldIt->isBitField(); + } + probinson wrote: > I might not be following this correctly, but it feels like EmitSeparator will > end up true if the last

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez updated this revision to Diff 505507. jmmartinez added a comment. - Fixed the loop and added test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144870/new/ https://reviews.llvm.org/D144870 Files:

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1558 + EmitSeparator = FieldIt->isBitField(); + } + I might not be following this correctly, but it feels like EmitSeparator will end up true if the last field is a bitfield,

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez updated this revision to Diff 505148. jmmartinez added a comment. Update: - Took into account remarks and got my ideas straight. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144870/new/ https://reviews.llvm.org/D144870 Files:

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

dblaikie added a comment. Haven't followed the patch in detail - but why does this require changes to the RecordLayout code? (I'd expect this to only require charnges to the debug info code - maybe with a shared helper utility function that both the AMDGPU ABI implementation, and the debug

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

jmmartinez created this revision. jmmartinez added reviewers: dblaikie, probinson. Herald added subscribers: kosarev, tpr. Herald added a project: All. jmmartinez requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. I'm looking for some