[PATCH][RFC] Respect alignment of nested bitfields

[Ulrich Weigand]

Richard Smith wrote:

> +  int64_t Align = Info.StorageAlignment;
> +  if (!LV.getAlignment().isZero())
> +    Align = std::min(Align, LV.getAlignment().getQuantity());
>
> This seems pessimistic. If LV is known to be more aligned than we'd
> expect for the bitfield, it seems that we should use its alignment
> in that case too. That is:
>
>   int64_t Align = LV.getAlignment().isZero() ? Info.StorageAlignment
> : LV.getAlignment().getQuantity();

Actually, it turns out this doesn't work.  While it still fixes the
test case I was originally concerned about, it introduces a number
of regressions elsewhere.  For example:

struct S607
{
  unsigned char a;
  long long int b;
  short int c;
  unsigned long long int d:47;
};
struct S607 s607;

void
test607 (void)
{
  s607.d = 89734188644825ULL;
}

now gets compiled to:

%struct.S607 = type { i8, i64, i16, [6 x i8] }

@s607 = common global %struct.S607 zeroinitializer, align 8

define void @test607() {
entry:
  %bf.load = load i48, i48* bitcast ([6 x i8]* getelementptr inbounds
(%struct.S607, %struct.S607* @s607, i32 0, i32 3) to i48*), align 8
  %bf.clear = and i48 %bf.load, 1
  %bf.set = or i48 %bf.clear, -102006599421006
  store i48 %bf.set, i48* bitcast ([6 x i8]* getelementptr inbounds
(%struct.S607, %struct.S607* @s607, i32 0, i32 3) to i48*), align 8
  ret void
}

This is wrong; the i48 load happens from a *2-byte* aligned address,
not an 8-byte aligned one.


Looking at the sources in a bit more detail, it seems that for a
BitField LValue, LV.getAlignment() returns the alignment of the
*surrounding structure*, not the alignment of the bitfield itself
(or the storage backing it).  See e.g. code in EmitLValueForField:

  if (field->isBitField()) {
    const CGRecordLayout &RL =
      CGM.getTypes().getCGRecordLayout(field->getParent());
    const CGBitFieldInfo &Info = RL.getBitFieldInfo(field);
    llvm::Value *Addr = base.getAddress();
    unsigned Idx = RL.getLLVMFieldNo(field);
    if (Idx != 0)
      // For structs, we GEP to the field that the record layout suggests.
      Addr = Builder.CreateStructGEP(nullptr, Addr, Idx, field->getName());
    // Get the access type.
    llvm::Type *PtrTy = llvm::Type::getIntNPtrTy(
      getLLVMContext(), Info.StorageSize,
      CGM.getContext().getTargetAddressSpace(base.getType()));
    if (Addr->getType() != PtrTy)
      Addr = Builder.CreateBitCast(Addr, PtrTy);

    QualType fieldType =
      field->getType().withCVRQualifiers(base.getVRQualifiers());
    return LValue::MakeBitfield(Addr, Info, fieldType, base.getAlignment
());
  }

The alignment operand to MakeBitfield is "base.getAlignment()", which
is the alignment of the surrounding struct.


Therefore, the original approach seems correct after all:
  LV.getAlignment() is the alignment of the surrounding struct
  Info.StorageAlignment is the alignment of the storage backing
                        the bitfield relative to the struct

So the actual alignment to be used for accessing that storage is
indeed the minimum of those two values.


I've updated the patch to add yet more tests to verify we don't
run into this regression.  Does this look OK now?

(See attached file: clang-align-bitfield)

Bye,
Ulrich
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-align-bitfield
Type: application/octet-stream
Size: 4089 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150504/480c2809/attachment.obj>