Re: [PATCH] Fix PRs 52080, 52097 and 48124, rewrite bitfield expansion, enable the C++ memory model wrt bitfields everywhere

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Mar 22, 2012 at 12:47 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> But the bitfield group _does_ start on a byte boundary. ?At least
>> that's what the new code in stor-layout ensures.
>
> No, it doesn't, since it does the computation on a record by record basis.
> Here we have a bitfield group that starts in a record and ends up in a nested
> record.
>
>> It's ok to assume the group starts on a byte boundary. ?But it's not
>> ok to modify bits outside of the access, so the RMW cycle has to mask
>> them properly.
>
> We have 2 options:
> ?1. the GCC 4.7 approach where get_bit_range returns 0 for bitstart, which
> doesn't make much sense but is in keeping with store_bit_field.
> ?2. the GCC 4.8 approach where get_bit_range attempts to return a more correct
> value, but is currently wrong (bitregion_start=11, bitregion_end=18) because
> the representative of the bitfield is wrong. ?The real representative of the
> bitfield is outside the record type.

bitregion_start == 11 looks bogus.  The representative is starting at

  DECL_FIELD_BIT_OFFSET (repr)
    = size_binop (BIT_AND_EXPR,
                  DECL_FIELD_BIT_OFFSET (field),
                  bitsize_int (~(BITS_PER_UNIT - 1)));

which looks ok, the size of the representative is (at minimum)

  size = size_diffop (DECL_FIELD_OFFSET (field),
                      DECL_FIELD_OFFSET (repr));
  gcc_assert (host_integerp (size, 1));
  bitsize = (tree_low_cst (size, 1) * BITS_PER_UNIT
             + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
             - 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);

that looks ok to me as well.  Is the issue that we, in get_bit_range,
compute bitregion_start relative to the byte-aligned offset of the
representative?

At least I still can't see where things go wrong from inspecting the
code ...

Richard.


> --
> Eric Botcazou