On Thu, 15 Mar 2012, Richard Guenther wrote: > On Thu, 15 Mar 2012, Eric Botcazou wrote: > > > > Ok, I applied a fix for PR52134 and am preparing a fix for PR52578. > > > It seems we might not be able to rely on > > > > > > tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)), > > > DECL_FIELD_OFFSET (repr)); > > > gcc_assert (host_integerp (maxsize, 1)); > > > > > > but at least until we get a testcase that shows so I won't add > > > (unexercised) code that handles it. Eventually we'd need to treat > > > tail-padding specially for some languages anyway, via a new langhook. > > > > This caused 3 classes of problems in Ada: > > > > 1. failure of the above assertion (pack7.ads) > > 2. ICE in tree_low_cst (pack16.adb, pack16_pkg.ads) > > 3. miscompilation (to be dealt with later). > > > > 1. and 2. appear to come from variable-sized fields (and 3. from record > > types > > with variant part). Testcases attached, they can be installed as: > > > > gnat.dg/pack16.adb > > gnat.dg/pack16_pkg.ads > > gnat.dg/specs/pack7.ads > > > > in the testsuite. > > I'll address these and add the testcases.
1. is easy, see patch below. 2. is much harder - we need to compute the bit-offset relative to the bitfield group start, thus in get_bit_range we do /* 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)); and we cannot generally assume offset is zero (well, maybe we could arrange that though, at least we could assert that for all fields in a bitfield group DECL_FIELD_OFFSET is the same?). Now, first of we lack gimplification of the DECL_BIT_FIELD_REPRESENTATIVE DECL_FIELD_OFFSET (not that we really need it - nothing could later introduce a COMPONENT_REF with that reliably as nothing holds on to the generated expressions). But then even for originally equal DECL_FIELD_OFFSETs we gimplify different stmts and thus have a different decl in DECL_FIELD_OFFSET so the folding above does not return a constant either (this makes me think that we can avoid gimplifying most of the sizepos - we only need to gimplify those that actually appear in COMPONENT_REFs). Any suggestion? Apart from trying to make sure that offset will be zero by construction? Or by simply not handling bitfields properly that start at a variable offset? The finish_bitfield_representative hunk implements the fix for 1., the rest the proposed zero-by-construction solution for 2. Thanks, Richard. Index: gcc/stor-layout.c =================================================================== *** gcc/stor-layout.c (revision 185426) --- gcc/stor-layout.c (working copy) *************** finish_bitfield_representative (tree rep *** 1765,1770 **** --- 1765,1773 ---- - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1) + tree_low_cst (DECL_SIZE (field), 1)); + /* Round up bitsize to multiples of BITS_PER_UNIT. */ + bitsize = (bitsize + BITS_PER_UNIT - 1) & ~(BITS_PER_UNIT - 1); + /* Now nothing tells us how to pad out bitsize ... */ nextf = DECL_CHAIN (field); while (nextf && TREE_CODE (nextf) != FIELD_DECL) *************** finish_bitfield_representative (tree rep *** 1787,1798 **** { /* ??? If you consider that tail-padding of this struct might be re-used when deriving from it we cannot really do the following ! and thus need to set maxsize to bitsize? */ tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)), 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 (repr), 1)); } /* Only if we don't artificially break up the representative in --- 1790,1805 ---- { /* ??? If you consider that tail-padding of this struct might be re-used when deriving from it we cannot really do the following ! and thus need to set maxsize to bitsize? Also we cannot ! generally rely on maxsize to fold to an integer constant, so ! use bitsize as fallback for this case. */ tree maxsize = size_diffop (TYPE_SIZE_UNIT (DECL_CONTEXT (field)), 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 (repr), 1)); ! else ! maxbitsize = bitsize; } /* Only if we don't artificially break up the representative in *************** finish_bitfield_representative (tree rep *** 1801,1809 **** at byte offset. */ gcc_assert (maxbitsize % BITS_PER_UNIT == 0); - /* Round up bitsize to multiples of BITS_PER_UNIT. */ - bitsize = (bitsize + BITS_PER_UNIT - 1) & ~(BITS_PER_UNIT - 1); - /* Find the smallest nice mode to use. */ for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode; mode = GET_MODE_WIDER_MODE (mode)) --- 1808,1813 ---- *************** finish_bitfield_layout (record_layout_in *** 1884,1889 **** --- 1888,1896 ---- } else if (DECL_BIT_FIELD_TYPE (field)) { + gcc_assert (repr != NULL_TREE + && operand_equal_p (DECL_FIELD_OFFSET (repr), + DECL_FIELD_OFFSET (field), 0)); /* Zero-size bitfields finish off a representative and do not have a representative themselves. This is required by the C++ memory model. */ Index: gcc/expr.c =================================================================== *** gcc/expr.c (revision 185426) --- 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,4477 **** } /* 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; --- 4467,4475 ---- } /* 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. */ ! bitoffset = (tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1) - tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)); *bitstart = bitpos - bitoffset;