RE: [PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

>
> Err, well. This just means that the generic C++ memory model
> handling isn't complete. We do
>
> if (TREE_CODE (to) == COMPONENT_REF
> && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
> get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);
>
> and thus restrict ourselves to bitfields to initialize the region we may touch.
> This just lacks computing bitregion_start / bitregion_end for other
> fields.
>
> Btw, what does the C++ memory model say for
>
> struct { char x; short a; short b; } a __attribute__((packed));
>
> short *p = &a.a;
>
> *p = 1;
>

this is not allowed. It should get at least a warning, that p is assigned
a value, which violates the alignment restrictions.
with packed structures you always have to access as "a.a".

> I suppose '*p' may not touch bits outside of a.a either? Or are
> memory accesses via pointers less constrained? What about
> array element accesses?
>

of course, *p may not access anything else than a short.
but the code generation may assume the pointer is aligned.

> That is, don't we need
>
> else
> {
> bitregion_start = 0;
> bitregion_end = bitsize;
> }
>
> ?
>
> Richard.
>

That looks like always pretending it is a bit field.
But it is not a bit field, and bitregion_start=bitregion_end=0
means it is an ordinary value.

It is just the bottom half of the expansion in expmed.c that does
not handle that really well, most of that code seems to ignore the
C++ memory model. For instance store_split_bit_field only handles
bitregion_end and not bitregion_start. And I do not see, why it
tries to write beyond a packed field at all. But that was OK in the past.

And then there is this:

      if (get_best_mem_extraction_insn (&insv, EP_insv, bitsize, bitnum,
                                        fieldmode)
          && store_bit_field_using_insv (&insv, op0, bitsize, bitnum, value))
        return true;

which dies not even care about bitregion_start/end.

An look at that code in store_bit_field:

  if (MEM_P (str_rtx) && bitregion_start> 0)
    {
      enum machine_mode bestmode;
      HOST_WIDE_INT offset, size;

      gcc_assert ((bitregion_start % BITS_PER_UNIT) == 0);

      offset = bitregion_start / BITS_PER_UNIT;
      bitnum -= bitregion_start;
      size = (bitnum + bitsize + BITS_PER_UNIT - 1) / BITS_PER_UNIT;
      bitregion_end -= bitregion_start;
      bitregion_start = 0;
      bestmode = get_best_mode (bitsize, bitnum,
                                bitregion_start, bitregion_end,
                                MEM_ALIGN (str_rtx), VOIDmode,
                                MEM_VOLATILE_P (str_rtx));
      str_rtx = adjust_bitfield_address_size (str_rtx, bestmode, offset, size);
    }

I'd bet 1 Euro, that the person who wrote that, did that because it was too difficult and error-prone
to re-write all the code below, and found it much safer to change the bitregion_start/end
here instead.

My patch is just layered on this if-block to accomplish the task. And that is why I think it is the
right place here.

Bernd.