On Mon, 19 Mar 2012, Richard Guenther wrote:
> 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.
This is what I have applied after bootstrapping and testing on
x86_64-unknown-linux-gnu.
Richard.
2012-03-20 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,1797 ----
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));
! /* If the group ends within a bitfield nextf does not need to be
! aligned to BITS_PER_UNIT. Thus round up. */
! maxbitsize = (maxbitsize + BITS_PER_UNIT - 1) & ~(BITS_PER_UNIT - 1);
! }
! else
! maxbitsize = bitsize;
}
else
{
*************** finish_bitfield_layout (record_layout_in
*** 1888,1893 ****
--- 1895,1902 ----
}
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 ****
--- 1905,1928 ----
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. */
+ else 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;
