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

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

On Fri, Nov 15, 2013 at 12:38 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>>
>> 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.

I don't think it is supposed to mean that.  It's supposed to mean
"the access is unconstrained".

> 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.

Hopefully insv targets exactly match bitnum/bitsize.

> 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.

It wasn't me ;)

> 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.

Well.  You are looking at the alignment of str_rtx which I'm not sure
whether it's the correct alignment at this point (we at least start
with the alignment of the base).

+  /* Treat unaligned fields like bit regions otherwise we might
+     touch bits outside the field.  */
+  if (MEM_P (str_rtx) && bitregion_start == 0 && bitregion_end == 0
+      && bitsize > 0 && bitsize % BITS_PER_UNIT == 0

bitsize is always > 0, why restrict it to byte-sized accesses?

+      && bitnum % BITS_PER_UNIT == 0

and byte-aligned accesses?

+      && (bitsize % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0
+ || bitnum % MIN (MEM_ALIGN (str_rtx), BITS_PER_WORD) > 0))

and what's this BITS_PER_WORD doing here?  It seems what you
are testing for (and should say so in the comment) is non-power-of-two
size accesses or accesses that are not naturally aligned.

But as said, I fail to see why doing this here instead of at the
point we compute the initial bitregion_start/end is appropriate - you
are after all affecting callers such as

calls.c:            store_bit_field (reg, bitsize, endian_correction, 0, 0,

and not only those that will possibly start from a non-zero bitregion_start/end.

Richard.

+    {
+      bitregion_start = bitnum;
+      bitregion_end = bitnum + bitsize - 1;
+    }



> Bernd.