On Mon, 19 Mar 2012, Eric Botcazou wrote: > > But it's only ever computed for RECORD_TYPEs where DECL_QUALIFIER is > > unused. > > OK, that could work indeed. > > > For now giving up seems to be easiest (just give up when > > DECL_FIELD_OFFSET is not equal for all of the bitfield members). > > That will at most get you the miscompiles for the PRs back, for > > languages with funny structure layout. > > I have another variant of the DECL_FIELD_OFFSET problem: > > FAIL: gnat.dg/specs/pack8.ads (test for excess errors) > Excess errors: > +===========================GNAT BUG DETECTED==============================+ > | 4.8.0 20120314 (experimental) [trunk revision 185395] (i586-suse-linux) GCC > error:| > | in finish_bitfield_representative, at stor-layout.c:1762 | > | Error detected at pack8.ads:17:4 > > Testcase attached: > > gnat.dg/specs/pack8.ads > gnat.dg/specs/pack8_pkg.ads
Thanks. That one indeed has different DECL_FIELD_OFFSET, ((sizetype) MAX_EXPR <(integer) pack8__R1s, 0> + (sizetype) MAX_EXPR <(integer) pack8__R1s, 0>) + 1 vs. (sizetype) MAX_EXPR <(integer) pack8__R1s, 0> + (sizetype) MAX_EXPR <(integer) pack8__R1s, 0> we're not putting the 1 byte offset into DECL_FIELD_BIT_OFFSET because DECL_OFFSET_ALIGN is 8 in this case. Eventually we should be able to relax how many bits we push into DECL_FIELD_BIT_OFFSET. > I agree that giving up (for now) is a sensible option. Thanks. Done with the patch below. We're actually not going to generate possibly wrong-code again but sub-optimal code. Bootstrap & regtest pending on x86_64-unknown-linux-gnu. Richard. 2012-03-19 Richard Guenther <rguent...@suse.de> * stor-layout.c (finish_bitfield_representative): Fallback to conservative maximum size if the padding up to the next field cannot be computed as a constant. (finish_bitfield_layout): If we cannot compute the distance between the start of the bitfield representative and the bitfield member start a new representative. * expr.c (get_bit_range): The distance between the start of the bitfield representative and the bitfield member is zero if the field offsets are not constants. * gnat.dg/pack16.adb: New testcase. * gnat.dg/pack16_pkg.ads: Likewise. * gnat.dg/specs/pack8.ads: Likewise. * gnat.dg/specs/pack8_pkg.ads: Likewise. Index: gcc/stor-layout.c =================================================================== *** gcc/stor-layout.c (revision 185518) --- gcc/stor-layout.c (working copy) *************** finish_bitfield_representative (tree rep *** 1781,1790 **** return; maxsize = size_diffop (DECL_FIELD_OFFSET (nextf), DECL_FIELD_OFFSET (repr)); ! gcc_assert (host_integerp (maxsize, 1)); ! maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT ! + tree_low_cst (DECL_FIELD_BIT_OFFSET (nextf), 1) ! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); } else { --- 1781,1792 ---- return; maxsize = size_diffop (DECL_FIELD_OFFSET (nextf), DECL_FIELD_OFFSET (repr)); ! if (host_integerp (maxsize, 1)) ! maxbitsize = (tree_low_cst (maxsize, 1) * BITS_PER_UNIT ! + tree_low_cst (DECL_FIELD_BIT_OFFSET (nextf), 1) ! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); ! else ! maxbitsize = bitsize; } else { *************** finish_bitfield_layout (record_layout_in *** 1888,1893 **** --- 1890,1897 ---- } else if (DECL_BIT_FIELD_TYPE (field)) { + gcc_assert (repr != NULL_TREE); + /* Zero-size bitfields finish off a representative and do not have a representative themselves. This is required by the C++ memory model. */ *************** finish_bitfield_layout (record_layout_in *** 1896,1901 **** --- 1900,1923 ---- finish_bitfield_representative (repr, prev); repr = NULL_TREE; } + + /* We assume that either DECL_FIELD_OFFSET of the representative + and each bitfield member is a constant or they are equal. + This is because we need to be able to compute the bit-offset + of each field relative to the representative in get_bit_range + during RTL expansion. + If these constraints are not met, simply force a new + representative to be generated. That will at most + generate worse code but still maintain correctness with + respect to the C++ memory model. */ + if (!((host_integerp (DECL_FIELD_OFFSET (repr), 1) + && host_integerp (DECL_FIELD_OFFSET (field), 1)) + || operand_equal_p (DECL_FIELD_OFFSET (repr), + DECL_FIELD_OFFSET (field), 0))) + { + finish_bitfield_representative (repr, prev); + repr = start_bitfield_representative (field); + } } else continue; Index: gcc/expr.c =================================================================== *** gcc/expr.c (revision 185518) --- gcc/expr.c (working copy) *************** get_bit_range (unsigned HOST_WIDE_INT *b *** 4452,4458 **** HOST_WIDE_INT bitpos) { unsigned HOST_WIDE_INT bitoffset; ! tree field, repr, offset; gcc_assert (TREE_CODE (exp) == COMPONENT_REF); --- 4452,4458 ---- HOST_WIDE_INT bitpos) { unsigned HOST_WIDE_INT bitoffset; ! tree field, repr; gcc_assert (TREE_CODE (exp) == COMPONENT_REF); *************** get_bit_range (unsigned HOST_WIDE_INT *b *** 4467,4478 **** } /* Compute the adjustment to bitpos from the offset of the field ! relative to the representative. */ ! offset = size_diffop (DECL_FIELD_OFFSET (field), ! DECL_FIELD_OFFSET (repr)); ! bitoffset = (tree_low_cst (offset, 1) * BITS_PER_UNIT ! + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) ! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); *bitstart = bitpos - bitoffset; *bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1; --- 4467,4483 ---- } /* Compute the adjustment to bitpos from the offset of the field ! relative to the representative. DECL_FIELD_OFFSET of field and ! repr are the same by construction if they are not constants, ! see finish_bitfield_layout. */ ! if (host_integerp (DECL_FIELD_OFFSET (field), 1) ! && host_integerp (DECL_FIELD_OFFSET (repr), 1)) ! bitoffset = (tree_low_cst (DECL_FIELD_OFFSET (field), 1) ! - tree_low_cst (DECL_FIELD_OFFSET (repr), 1)) * BITS_PER_UNIT; ! else ! bitoffset = 0; ! bitoffset += (tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) ! - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); *bitstart = bitpos - bitoffset; *bitend = *bitstart + tree_low_cst (DECL_SIZE (repr), 1) - 1; Index: gcc/testsuite/gnat.dg/pack16.adb =================================================================== *** gcc/testsuite/gnat.dg/pack16.adb (revision 0) --- gcc/testsuite/gnat.dg/pack16.adb (revision 0) *************** *** 0 **** --- 1,26 ---- + -- { dg-do compile } + -- { dg-options "-gnatws" } + + with Pack16_Pkg; use Pack16_Pkg; + + procedure Pack16 is + + type Sample_Table_T is array (1 .. N) of Integer; + + type Clock_T is record + N_Ticks : Integer := 0; + end record; + + type Sampling_Descriptor_T is record + Values : Sample_Table_T; + Valid : Boolean; + Tstamp : Clock_T; + end record; + + pragma Pack (Sampling_Descriptor_T); + + Sampling_Data : Sampling_Descriptor_T; + + begin + null; + end; Index: gcc/testsuite/gnat.dg/pack16_pkg.ads =================================================================== *** gcc/testsuite/gnat.dg/pack16_pkg.ads (revision 0) --- gcc/testsuite/gnat.dg/pack16_pkg.ads (revision 0) *************** *** 0 **** --- 1,5 ---- + package Pack16_Pkg is + + N : Natural := 16; + + end Pack16_Pkg; Index: gcc/testsuite/gnat.dg/specs/pack8.ads =================================================================== *** gcc/testsuite/gnat.dg/specs/pack8.ads (revision 0) --- gcc/testsuite/gnat.dg/specs/pack8.ads (revision 0) *************** *** 0 **** --- 1,19 ---- + with Pack8_Pkg; + + package Pack8 is + + subtype Index_Type is Integer range 1 .. Pack8_Pkg.N; + + subtype Str is String( Index_Type); + + subtype Str2 is String (1 .. 11); + + type Rec is record + S1 : Str; + S2 : Str; + B : Boolean; + S3 : Str2; + end record; + pragma Pack (Rec); + + end Pack8; Index: gcc/testsuite/gnat.dg/specs/pack8_pkg.ads =================================================================== *** gcc/testsuite/gnat.dg/specs/pack8_pkg.ads (revision 0) --- gcc/testsuite/gnat.dg/specs/pack8_pkg.ads (revision 0) *************** *** 0 **** --- 1,5 ---- + package Pack8_Pkg is + + N : Natural := 1; + + end Pack8_Pkg;