Skip to content

Commit 72eccae

Browse files
gitoleglanza
authored andcommitted
[CIR][CodeGen][Bugfix] Fix storage size for bitfields (#462)
This PR fixes a bug caused by `IntType` size limitations in CIR (and by some magic of numbers as well). As you know, we need to create a storage for bit fields that usually contain several of them. There next code fails with `IntType` size check which exceeds 64 bits. ``` typedef struct { uint8_t a; uint8_t b; uint8_t c; int d: 2; int e: 2; int f: 4; int g: 25; int h: 3; int i: 4; int j: 3; int k: 8; int l: 14; } D; void foo() { D d; } ``` Note, if we remove first three fields (or even one) everything will be fine even without this fix, because [this](https://github.com/llvm/clangir/blob/main/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp#L553) check won't pass. The bug is kind of hard to reproduce and I would say it's a rare case. I mean the problem is not only in the number of bit fields that form together something bigger than 64 bits. ### Several details Well, while iterating over the bit fields in some struct type, we need to stop accumulating bit fields in one storage and start to do the same in another one. Basically, we operate with `Tail` and `StartBitOffset` where the former is an offset of the next field. And once `Tail - StartBitOffset >= 64` we say that it's not possible to create a storage of such size due to `IntType` size limitation. Sounds reasonable. But it can be a case when we can not afford to take the next field because its `Tail` in turn leads to a storage of the size bigger then 64. Thus, we want to check it as well. From the implementation point of view I added one more check to the `IsBetterAsSingleFieldRun` in order to have all these checks for size in a single place. And the check I mentioned before were saving us from hitting this issue.
1 parent e846fd2 commit 72eccae

File tree

2 files changed

+40
-5
lines changed

2 files changed

+40
-5
lines changed

clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -500,9 +500,14 @@ void CIRRecordLowering::accumulateBitFields(
500500
// bitfield a separate storage component so as it can be accessed directly
501501
// with lower cost.
502502
auto IsBetterAsSingleFieldRun = [&](uint64_t OffsetInRecord,
503-
uint64_t StartBitOffset) {
504-
if (OffsetInRecord >= 64) // See IntType::verify
503+
uint64_t StartBitOffset,
504+
uint64_t nextTail = 0) {
505+
if (OffsetInRecord >= 64 ||
506+
(nextTail > StartBitOffset &&
507+
nextTail - StartBitOffset >= 64)) { // See IntType::verify
505508
return true;
509+
}
510+
506511
if (!cirGenTypes.getModule().getCodeGenOpts().FineGrainedBitfieldAccesses)
507512
return false;
508513
llvm_unreachable("NYI");
@@ -545,13 +550,18 @@ void CIRRecordLowering::accumulateBitFields(
545550
// field is inconsistent with the offset of previous field plus its offset,
546551
// skip the block below and go ahead to emit the storage. Otherwise, try to
547552
// add bitfields to the run.
553+
uint64_t nextTail = Tail;
554+
if (Field != FieldEnd)
555+
nextTail += Field->getBitWidthValue(astContext);
556+
548557
if (!StartFieldAsSingleRun && Field != FieldEnd &&
549-
!IsBetterAsSingleFieldRun(Tail - StartBitOffset, StartBitOffset) &&
558+
!IsBetterAsSingleFieldRun(Tail - StartBitOffset, StartBitOffset,
559+
nextTail) &&
550560
(!Field->isZeroLengthBitField(astContext) ||
551561
(!astContext.getTargetInfo().useZeroLengthBitfieldAlignment() &&
552562
!astContext.getTargetInfo().useBitFieldTypeAlignment())) &&
553563
Tail == getFieldBitOffset(*Field)) {
554-
Tail += Field->getBitWidthValue(astContext);
564+
Tail = nextTail;
555565
++Field;
556566
continue;
557567
}

clang/test/CIR/CodeGen/bitfields.c

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,29 @@ typedef struct {
3434
unsigned b;
3535
} T;
3636

37+
typedef struct {
38+
char a;
39+
char b;
40+
char c;
41+
42+
// startOffset 24 bits, new storage from here
43+
int d: 2;
44+
int e: 2;
45+
int f: 4;
46+
int g: 25;
47+
int h: 3;
48+
int i: 4;
49+
int j: 3;
50+
int k: 8;
51+
52+
int l: 14; // need to be a part of the new storage
53+
// because (tail - startOffset) is 65 after 'l' field
54+
} U;
55+
3756
// CHECK: !ty_22D22 = !cir.struct<struct "D" {!cir.int<u, 16>, !cir.int<s, 32>}>
3857
// CHECK: !ty_22S22 = !cir.struct<struct "S" {!cir.int<u, 32>, !cir.int<u, 32>, !cir.int<u, 16>, !cir.int<u, 32>}>
3958
// CHECK: !ty_22T22 = !cir.struct<struct "T" {!cir.int<u, 8>, !cir.int<u, 32>} #cir.record.decl.ast>
59+
// CHECK: !ty_22U22 = !cir.struct<struct "U" {!cir.int<s, 8>, !cir.int<s, 8>, !cir.int<s, 8>, !cir.int<u, 64>, !cir.int<u, 16>}>
4060
// CHECK: !ty_22anon2E122 = !cir.struct<struct "anon.1" {!cir.int<u, 32>} #cir.record.decl.ast>
4161
// CHECK: !ty_anon_struct = !cir.struct<struct {!cir.int<u, 8>, !cir.int<u, 8>, !cir.int<s, 32>}>
4262
// CHECK: !ty_22__long22 = !cir.struct<struct "__long" {!cir.struct<struct "anon.1" {!cir.int<u, 32>} #cir.record.decl.ast>, !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>}>
@@ -107,6 +127,11 @@ int load_one_bitfield(T* t) {
107127
return t->a;
108128
}
109129

130+
// CHECK: cir.func {{.*@createU}}
131+
void createU() {
132+
U u;
133+
}
134+
110135
// for this struct type we create an anon structure with different storage types in initialization
111136
// CHECK: cir.func {{.*@createD}}
112137
// CHECK: %0 = cir.alloca !ty_22D22, cir.ptr <!ty_22D22>, ["d"] {alignment = 4 : i64}
@@ -115,4 +140,4 @@ int load_one_bitfield(T* t) {
115140
// CHECK: cir.store %2, %1 : !ty_anon_struct, cir.ptr <!ty_anon_struct>
116141
void createD() {
117142
D d = {1,2,3};
118-
}
143+
}

0 commit comments

Comments
 (0)