This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jmmartinez on Feb 27 2023, 6:29 AM.

Details

Summary

I'm looking for some feedback, this is definitively not the final solution.
I was wondering if there was an alternative to pass this information in the dwarf debug-info.

Consider the following sturctures when targetting:

struct foo {
  int space[4];
  char a : 8;
  char b : 8;
  char x : 8;
  char y : 8;
};

struct bar {
  int space[4];
  char a : 8;
  char b : 8;
  char : 0;
  char x : 8;
  char y : 8;
};

Even if both structs have the same layout in memory, they are handled
differenlty by the AMDGPU ABI.

With the following code:

// clang --target=amdgcn-amd-amdhsa -g -O1 example.c -S
char use_foo(struct foo f) { return f.y; }
char use_bar(struct bar b) { return b.y; }

For use_foo, the 'y' field is passed in v4
; v_ashrrev_i32_e32 v0, 24, v4
; s_setpc_b64 s[30:31]

For use_bar, the 'y' field is passed in v5
; v_bfe_i32 v0, v5, 8, 8
; s_setpc_b64 s[30:31]

To make this distinction, we record a single 0-size bitfield for every member that is preceded
by it.

Diff Detail

Event Timeline

jmmartinez created this revision.Feb 27 2023, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 6:29 AM
Herald added subscribers: kosarev, tpr. · View Herald Transcript
jmmartinez requested review of this revision.Feb 27 2023, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 6:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 info can use to observe this property, if it's non-trivial code to duplicate in both places? (but doesn't look like it's sharing code with the ABI implementation, or moving the logic out of the ABI implementation into the record layout code for reuse there))

Update:

  • Took into account remarks and got my ideas straight.
probinson added inline comments.Mar 14 2023, 10:42 AM
clang/lib/CodeGen/CGDebugInfo.cpp
1558

I might not be following this correctly, but it feels like EmitSeparator will end up true if the last field is a bitfield, even if there are no zero-length bitfields in front of it. The test does not cover this case (to show that the no-zero-bitfields case is handled properly).

  • Fixed the loop and added test case
jmmartinez added inline comments.Mar 15 2023, 8:44 AM
clang/lib/CodeGen/CGDebugInfo.cpp
1558

Ouch! I feel ashamed about that loop I originally wrote.

I've added a case covering the non-zero-bitfields case and fixed the code.

jmmartinez marked an inline comment as done.Mar 17 2023, 6:28 AM

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, then when processing c it's sufficient to see that b is a preceding bitfield and know that the needful has been done.

I'm now also curious whether this ABI aspect affects non-bitfields. For example:

struct non_adjacent_bitfields {
  char d;
  char : 0;
  char e;
  char f : 8;
};

(1) Is e affected by the presence of the zero-length bitfield? (2) is f affected? (3) If the answers are "no" and "yes" then you do need to iterate looking for the zero-length bitfield, but otherwise I think it's sufficient to look only at the preceding field.

(Of course it's possible that finding the preceding field can be done only by iterating, but I'd hope that isn't necessary.)

(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 underlying decl_iterator used to implement it are forward iterators.

However, your comment made me realize that we could forward the elements array from CGDebugInfoCollectRecordNormalFields which contains the already created debug-info for the previous fields. By using this array I could get pretty much the same effect. What do you think?

struct non_adjacent_bitfields {
  char d;
  char : 0;
  char e;
  char f : 8;
};

(1) Is e affected by the presence of the zero-length bitfield? (2) is f affected? (3) If the answers are "no" and "yes" then you do need to iterate looking for the zero-length bitfield, but otherwise I think it's sufficient to look only at the preceding field.

(1) and (2): They do not get affected by the precense of the annonymous bitfield. Only sequences of bitfields get "squashed" in the same integer.

  • Updated to use elements array to avoid the complicated loop.

Still one question, and haven't dug into the test in detail yet.

clang/lib/CodeGen/CGDebugInfo.cpp
1552

Maybe a comment here,
// If we already emitted metadata for a 0-length bitfield, nothing to do here.
(I was briefly confused by "if the previous field is a 0-length bitfield, do nothing" until I realized this is looking at the metadata not the decl.

1563

Is this true for cases like

struct nonadjacent {
  char a : 8;
  char : 0;
  int b;
  char d : 8;
};

where the field d has a predecessor that is not a bitfield? (This might be my ignorance of how Decls are put together, but asserting that advance is guaranteed to get you a bitfield just seems a little odd.)

clang/lib/CodeGen/CGDebugInfo.h
325–333

Please keep that final .

331
jmmartinez marked 3 inline comments as done.
  • Took remarks into account
clang/lib/CodeGen/CGDebugInfo.cpp
1563

In that case the assert is never reached.

When emiting the debug-info for d, when looking at the metadata generated for the previous field the function should exit early on this condition:

if (!PreviousMDField || !PreviousMDField->isBitField() ||
    PreviousMDField->getSizeInBits() == 0)
  return nullptr;
probinson accepted this revision.Mar 27 2023, 1:14 PM

One entirely optional suggestion on the test. LGTM.

clang/lib/CodeGen/CGDebugInfo.cpp
1563

Ah, you are right. Thanks!

clang/test/CodeGen/debug-info-bitfield-0-struct.c
41

Maybe use something like SECONDDUP instead of SECONDD which can look like a typo.

This revision is now accepted and ready to land.Mar 27 2023, 1:14 PM

One entirely optional suggestion on the test. LGTM.

Thanks! I used SECONDDUP and FIRSTDUP.

And thanks a lot for the reviews!