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

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

On Mon, Nov 25, 2013 at 1:07 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hello,
>
> I had forgotten to run the Ada test suite when I submitted the previous version of this patch.
> And indeed there were some Ada test cases failing because in Ada packed structures are
> like bit fields, but without the DECL_BIT_FIELD_TYPE attribute.

I think they may have DECL_BIT_FIELD set though, not sure.

> Please find attached the updated version of this patch.
>
> Boot-strapped and regression-tested on x86_64-linux-gnu.
> Ok for trunk?

So you mimic what Eric added in get_bit_range?  Btw, I'm not sure
the "conservative" way of failing get_bit_range with not limiting the
access at all is good.

That is, we may want to do

+      /* The C++ memory model naturally applies to byte-aligned fields.
+        However, if we do not have a DECL_BIT_FIELD_TYPE but BITPOS or
+        BITSIZE are not byte-aligned, there is no need to limit the range
+        we can access.  This can occur with packed structures in Ada.  */
+      if (bitregion_start == 0 && bitregion_end == 0
+          && bitsize > 0
+          && bitsize % BITS_PER_UNIT == 0
+          && bitpos % BITS_PER_UNIT == 0)
+       {
+         bitregion_start = bitpos;
+         bitregion_end = bitpos + bitsize - 1;
+       }

thus not else if but also apply it when get_bit_range "failed" (as it may
fail for other reasons).  A better fallback would be to track down
the outermost byte-aligned handled-component and limit the access
to that (though I guess Ada doesn't care at all about the C++ memory
model and only Ada has bit-aligned aggregates).

That said, the patch looks ok as-is to me, let's see if we can clean
things up for the next stage1.

Thanks,
Richard.

> Bernd.
>
>> On Mon, 18 Nov 2013 11:37:05, Bernd Edlinger wrote:
>>
>> Hi,
>>
>> On Fri, 15 Nov 2013 13:30:51, Richard Biener wrote:
>>>> 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".
>>>
>>
>> Ok, agreed, I did not regard that as a feature.
>> And apparently only the code path in expand_assigment
>> really has a problem with it.
>>
>> So here my second attempt at fixing this.
>>
>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>
>> OK for trunk?
>>
>>
>> Thanks
>> Bernd.