From: John McCall Date: Wed, 29 Jan 2014 07:53:44 +0000 (+0000) Subject: Extensively comment bitfield layout, rearrange some X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4e88652a34dca760f366a2bf5356c8e7d0159464;p=clang Extensively comment bitfield layout, rearrange some code for legibility, and fix a bug with bitfields in packed ms_structs. rdar://15926990 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@200379 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/AST/RecordLayoutBuilder.cpp b/lib/AST/RecordLayoutBuilder.cpp index 9183e5c66f..903f973e61 100644 --- a/lib/AST/RecordLayoutBuilder.cpp +++ b/lib/AST/RecordLayoutBuilder.cpp @@ -1450,127 +1450,191 @@ void RecordLayoutBuilder::LayoutBitField(const FieldDecl *D) { uint64_t TypeSize = FieldInfo.first; unsigned FieldAlign = FieldInfo.second; + // UnfilledBitsInLastUnit is the difference between the end of the + // last allocated bitfield (i.e. the first bit offset available for + // bitfields) and the end of the current data size in bits (i.e. the + // first bit offset available for non-bitfields). The current data + // size in bits is always a multiple of the char size; additionally, + // for ms_struct records it's also a multiple of the + // LastBitfieldTypeSize (if set). + + // The basic bitfield layout rule for ms_struct is to allocate an + // entire unit of the bitfield's declared type (e.g. 'unsigned + // long'), then parcel it up among successive bitfields whose + // declared types have the same size, making a new unit as soon as + // the last can no longer store the whole value. + + // The standard bitfield layout rule for non-ms_struct is to place + // bitfields at the next available bit offset where the entire + // bitfield would fit in an aligned storage unit of the declared + // type (even if there are also non-bitfields within that same + // unit). However, some targets (those that !useBitFieldTypeAlignment()) + // don't require this storage unit to be aligned, and therefore + // always put the bit-field at the next available bit offset. + // Such targets generally do interpret zero-width bitfields as + // forcing the use of a new storage unit. + + // First, some simple bookkeeping to perform for ms_struct structs. if (IsMsStruct) { - // The field alignment for integer types in ms_struct structs is - // always the size. + // The field alignment for integer types is always the size. FieldAlign = TypeSize; - // Ignore zero-length bitfields after non-bitfields in ms_struct structs. - if (!FieldSize && !LastBitfieldTypeSize) - FieldAlign = 1; - // If a bitfield is followed by a bitfield of a different size, don't - // pack the bits together in ms_struct structs. + + // If the previous field was not a bitfield, or was a bitfield + // with a different storage unit size, we're done with that + // storage unit. if (LastBitfieldTypeSize != TypeSize) { + // Also, ignore zero-length bitfields after non-bitfields. + if (!LastBitfieldTypeSize && !FieldSize) + FieldAlign = 1; + UnfilledBitsInLastUnit = 0; LastBitfieldTypeSize = 0; } } - uint64_t UnpaddedFieldOffset = getDataSizeInBits() - UnfilledBitsInLastUnit; - uint64_t FieldOffset = IsUnion ? 0 : UnpaddedFieldOffset; - - bool ZeroLengthBitfield = false; - if (!Context.getTargetInfo().useBitFieldTypeAlignment() && - Context.getTargetInfo().useZeroLengthBitfieldAlignment() && - FieldSize == 0) { - // The alignment of a zero-length bitfield affects the alignment - // of the next member. The alignment is the max of the zero - // length bitfield's alignment and a target specific fixed value. - ZeroLengthBitfield = true; - unsigned ZeroLengthBitfieldBoundary = - Context.getTargetInfo().getZeroLengthBitfieldBoundary(); - if (ZeroLengthBitfieldBoundary > FieldAlign) - FieldAlign = ZeroLengthBitfieldBoundary; - } - + // If the field is wider than its declared type, it follows + // different rules in all cases. if (FieldSize > TypeSize) { LayoutWideBitField(FieldSize, TypeSize, FieldPacked, D); return; } - // The align if the field is not packed. This is to check if the attribute - // was unnecessary (-Wpacked). + // Compute the next available bit offset. + uint64_t FieldOffset = + IsUnion ? 0 : (getDataSizeInBits() - UnfilledBitsInLastUnit); + + // Handle targets that don't honor bitfield type alignment. + if (!Context.getTargetInfo().useBitFieldTypeAlignment()) { + // Some such targets do honor it on zero-width bitfields. + if (FieldSize == 0 && + Context.getTargetInfo().useZeroLengthBitfieldAlignment()) { + // The alignment to round up to is the max of the field's natural + // alignment and a target-specific fixed value (sometimes zero). + unsigned ZeroLengthBitfieldBoundary = + Context.getTargetInfo().getZeroLengthBitfieldBoundary(); + FieldAlign = std::max(FieldAlign, ZeroLengthBitfieldBoundary); + + // If that doesn't apply, just ignore the field alignment. + } else { + FieldAlign = 1; + } + } + + // Remember the alignment we would have used if the field were not packed. unsigned UnpackedFieldAlign = FieldAlign; - uint64_t UnpackedFieldOffset = FieldOffset; - if (!Context.getTargetInfo().useBitFieldTypeAlignment() && !ZeroLengthBitfield) - UnpackedFieldAlign = 1; - if (FieldPacked || - (!Context.getTargetInfo().useBitFieldTypeAlignment() && !ZeroLengthBitfield)) + // Ignore the field alignment if the field is packed. + if (FieldPacked) FieldAlign = 1; - FieldAlign = std::max(FieldAlign, D->getMaxAlignment()); - UnpackedFieldAlign = std::max(UnpackedFieldAlign, D->getMaxAlignment()); - // The maximum field alignment overrides the aligned attribute. - if (!MaxFieldAlignment.isZero() && FieldSize != 0) { + // But, if there's an 'aligned' attribute on the field, honor that. + if (unsigned ExplicitFieldAlign = D->getMaxAlignment()) { + FieldAlign = std::max(FieldAlign, ExplicitFieldAlign); + UnpackedFieldAlign = std::max(UnpackedFieldAlign, ExplicitFieldAlign); + } + + // But, if there's a #pragma pack in play, that takes precedent over + // even the 'aligned' attribute, for non-zero-width bitfields. + if (!MaxFieldAlignment.isZero() && FieldSize) { unsigned MaxFieldAlignmentInBits = Context.toBits(MaxFieldAlignment); FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits); UnpackedFieldAlign = std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits); } - // ms_struct bitfields always have to start at a round alignment. - if (IsMsStruct && !LastBitfieldTypeSize) { - FieldOffset = llvm::RoundUpToAlignment(FieldOffset, FieldAlign); - UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset, - UnpackedFieldAlign); - } + // For purposes of diagnostics, we're going to simultaneously + // compute the field offsets that we would have used if we weren't + // adding any alignment padding or if the field weren't packed. + uint64_t UnpaddedFieldOffset = FieldOffset; + uint64_t UnpackedFieldOffset = FieldOffset; - // Check if we need to add padding to give the field the correct alignment. - if (FieldSize == 0 || - (MaxFieldAlignment.isZero() && - (FieldOffset & (FieldAlign-1)) + FieldSize > TypeSize)) - FieldOffset = llvm::RoundUpToAlignment(FieldOffset, FieldAlign); + // Check if we need to add padding to fit the bitfield within an + // allocation unit with the right size and alignment. The rules are + // somewhat different here for ms_struct structs. + if (IsMsStruct) { + // If it's not a zero-width bitfield, and we can fit the bitfield + // into the active storage unit (and we haven't already decided to + // start a new storage unit), just do so, regardless of any other + // other consideration. Otherwise, round up to the right alignment. + if (FieldSize == 0 || FieldSize > UnfilledBitsInLastUnit) { + FieldOffset = llvm::RoundUpToAlignment(FieldOffset, FieldAlign); + UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset, + UnpackedFieldAlign); + UnfilledBitsInLastUnit = 0; + } - if (FieldSize == 0 || - (MaxFieldAlignment.isZero() && - (UnpackedFieldOffset & (UnpackedFieldAlign-1)) + FieldSize > TypeSize)) - UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset, - UnpackedFieldAlign); + } else { + // #pragma pack, with any value, suppresses the insertion of padding. + bool AllowPadding = MaxFieldAlignment.isZero(); + + // Compute the real offset. + if (FieldSize == 0 || + (AllowPadding && + (FieldOffset & (FieldAlign-1)) + FieldSize > TypeSize)) { + FieldOffset = llvm::RoundUpToAlignment(FieldOffset, FieldAlign); + } - // Padding members don't affect overall alignment, unless zero length bitfield - // alignment is enabled. - if (!D->getIdentifier() && - !Context.getTargetInfo().useZeroLengthBitfieldAlignment() && - !IsMsStruct) - FieldAlign = UnpackedFieldAlign = 1; + // Repeat the computation for diagnostic purposes. + if (FieldSize == 0 || + (AllowPadding && + (UnpackedFieldOffset & (UnpackedFieldAlign-1)) + FieldSize > TypeSize)) + UnpackedFieldOffset = llvm::RoundUpToAlignment(UnpackedFieldOffset, + UnpackedFieldAlign); + } + // If we're using external layout, give the external layout a chance + // to override this information. if (ExternalLayout) FieldOffset = updateExternalFieldOffset(D, FieldOffset); - // Place this field at the current location. + // Okay, place the bitfield at the calculated offset. FieldOffsets.push_back(FieldOffset); + // Bookkeeping: + + // Anonymous members don't affect the overall record alignment, + // except on targets where they do. + if (!IsMsStruct && + !Context.getTargetInfo().useZeroLengthBitfieldAlignment() && + !D->getIdentifier()) + FieldAlign = UnpackedFieldAlign = 1; + + // Diagnose differences in layout due to padding or packing. if (!ExternalLayout) CheckFieldPadding(FieldOffset, UnpaddedFieldOffset, UnpackedFieldOffset, UnpackedFieldAlign, FieldPacked, D); // Update DataSize to include the last byte containing (part of) the bitfield. + + // For unions, this is just a max operation, as usual. if (IsUnion) { // FIXME: I think FieldSize should be TypeSize here. setDataSize(std::max(getDataSizeInBits(), FieldSize)); - } else { - if (IsMsStruct && FieldSize) { - // Under ms_struct, a bitfield always takes up space equal to the size - // of the type. We can't just change the alignment computation on the - // other codepath because of the way this interacts with #pragma pack: - // in a packed struct, we need to allocate misaligned space in the - // struct to hold the bitfield. - if (!UnfilledBitsInLastUnit) { - setDataSize(FieldOffset + TypeSize); - UnfilledBitsInLastUnit = TypeSize - FieldSize; - } else if (UnfilledBitsInLastUnit < FieldSize) { - setDataSize(getDataSizeInBits() + TypeSize); - UnfilledBitsInLastUnit = TypeSize - FieldSize; - } else { - UnfilledBitsInLastUnit -= FieldSize; - } - LastBitfieldTypeSize = TypeSize; - } else { - uint64_t NewSizeInBits = FieldOffset + FieldSize; - uint64_t BitfieldAlignment = Context.getTargetInfo().getCharAlign(); - setDataSize(llvm::RoundUpToAlignment(NewSizeInBits, BitfieldAlignment)); - UnfilledBitsInLastUnit = getDataSizeInBits() - NewSizeInBits; - LastBitfieldTypeSize = 0; + + // For non-zero-width bitfields in ms_struct structs, allocate a new + // storage unit if necessary. + } else if (IsMsStruct && FieldSize) { + // We should have cleared UnfilledBitsInLastUnit in every case + // where we changed storage units. + if (!UnfilledBitsInLastUnit) { + setDataSize(FieldOffset + TypeSize); + UnfilledBitsInLastUnit = TypeSize; } + UnfilledBitsInLastUnit -= FieldSize; + LastBitfieldTypeSize = TypeSize; + + // Otherwise, bump the data size up to include the bitfield, + // including padding up to char alignment, and then remember how + // bits we didn't use. + } else { + uint64_t NewSizeInBits = FieldOffset + FieldSize; + uint64_t CharAlignment = Context.getTargetInfo().getCharAlign(); + setDataSize(llvm::RoundUpToAlignment(NewSizeInBits, CharAlignment)); + UnfilledBitsInLastUnit = getDataSizeInBits() - NewSizeInBits; + + // The only time we can get here for an ms_struct is if this is a + // zero-width bitfield, which doesn't count as anything for the + // purposes of unfilled bits. + LastBitfieldTypeSize = 0; } // Update the size. diff --git a/test/CodeGen/ms_struct-pack.c b/test/CodeGen/ms_struct-pack.c index da94f54c1f..6486f2975f 100644 --- a/test/CodeGen/ms_struct-pack.c +++ b/test/CodeGen/ms_struct-pack.c @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -emit-llvm-only -triple i386-apple-darwin9 %s +// RUN: %clang_cc1 -emit-llvm-only -triple i386-apple-darwin9 -fdump-record-layouts %s | FileCheck %s // rdar://8823265 #pragma pack(1) @@ -123,3 +123,22 @@ typedef struct _eight_ms eight_ms; static int a8[(sizeof(eight_ms) == 48) - 1]; +// rdar://15926990 +#pragma pack(2) +struct test0 { + unsigned long a : 8; + unsigned long b : 8; + unsigned long c : 8; + unsigned long d : 10; + unsigned long e : 1; +} __attribute__((__ms_struct__)); + +// CHECK: Type: struct test0 +// CHECK-NEXT: Record: +// CHECK-NEXT: Layout: +// CHECK-NEXT: Size:64 +// CHECK-NEXT: DataSize:64 +// CHECK-NEXT: Alignment:16 +// CHECK-NEXT: FieldOffsets: [0, 8, 16, 32, 42]> + +static int test0[(sizeof(struct test0) == 8) ? 1 : -1];