Skip to content

Commit

Permalink
[CIR][CodeGen][Bugfix] Fix storage size for bitfields (llvm#462)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
gitoleg authored and lanza committed Mar 20, 2024
1 parent a9dec0f commit 91e6ba5
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 5 deletions.
18 changes: 14 additions & 4 deletions clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -500,9 +500,14 @@ void CIRRecordLowering::accumulateBitFields(
// bitfield a separate storage component so as it can be accessed directly
// with lower cost.
auto IsBetterAsSingleFieldRun = [&](uint64_t OffsetInRecord,
uint64_t StartBitOffset) {
if (OffsetInRecord >= 64) // See IntType::verify
uint64_t StartBitOffset,
uint64_t nextTail = 0) {
if (OffsetInRecord >= 64 ||
(nextTail > StartBitOffset &&
nextTail - StartBitOffset >= 64)) { // See IntType::verify
return true;
}

if (!cirGenTypes.getModule().getCodeGenOpts().FineGrainedBitfieldAccesses)
return false;
llvm_unreachable("NYI");
Expand Down Expand Up @@ -545,13 +550,18 @@ void CIRRecordLowering::accumulateBitFields(
// field is inconsistent with the offset of previous field plus its offset,
// skip the block below and go ahead to emit the storage. Otherwise, try to
// add bitfields to the run.
uint64_t nextTail = Tail;
if (Field != FieldEnd)
nextTail += Field->getBitWidthValue(astContext);

if (!StartFieldAsSingleRun && Field != FieldEnd &&
!IsBetterAsSingleFieldRun(Tail - StartBitOffset, StartBitOffset) &&
!IsBetterAsSingleFieldRun(Tail - StartBitOffset, StartBitOffset,
nextTail) &&
(!Field->isZeroLengthBitField(astContext) ||
(!astContext.getTargetInfo().useZeroLengthBitfieldAlignment() &&
!astContext.getTargetInfo().useBitFieldTypeAlignment())) &&
Tail == getFieldBitOffset(*Field)) {
Tail += Field->getBitWidthValue(astContext);
Tail = nextTail;
++Field;
continue;
}
Expand Down
27 changes: 26 additions & 1 deletion clang/test/CIR/CodeGen/bitfields.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,29 @@ typedef struct {
unsigned b;
} T;

typedef struct {
char a;
char b;
char c;

// startOffset 24 bits, new storage from here
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; // need to be a part of the new storage
// because (tail - startOffset) is 65 after 'l' field
} U;

// CHECK: !ty_22D22 = !cir.struct<struct "D" {!cir.int<u, 16>, !cir.int<s, 32>}>
// CHECK: !ty_22S22 = !cir.struct<struct "S" {!cir.int<u, 32>, !cir.int<u, 32>, !cir.int<u, 16>, !cir.int<u, 32>}>
// CHECK: !ty_22T22 = !cir.struct<struct "T" {!cir.int<u, 8>, !cir.int<u, 32>} #cir.record.decl.ast>
// 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>}>
// CHECK: !ty_22anon2E122 = !cir.struct<struct "anon.1" {!cir.int<u, 32>} #cir.record.decl.ast>
// CHECK: !ty_anon_struct = !cir.struct<struct {!cir.int<u, 8>, !cir.int<u, 8>, !cir.int<s, 32>}>
// 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>>}>
Expand Down Expand Up @@ -107,6 +127,11 @@ int load_one_bitfield(T* t) {
return t->a;
}

// CHECK: cir.func {{.*@createU}}
void createU() {
U u;
}

// for this struct type we create an anon structure with different storage types in initialization
// CHECK: cir.func {{.*@createD}}
// CHECK: %0 = cir.alloca !ty_22D22, cir.ptr <!ty_22D22>, ["d"] {alignment = 4 : i64}
Expand All @@ -115,4 +140,4 @@ int load_one_bitfield(T* t) {
// CHECK: cir.store %2, %1 : !ty_anon_struct, cir.ptr <!ty_anon_struct>
void createD() {
D d = {1,2,3};
}
}

0 comments on commit 91e6ba5

Please sign in to comment.