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

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

Hi,

On Mon, 2 Dec 2013 15:55:08Richard Biener wrote:
>
> 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).
>

Good question. Most of the time the expansion can not know if it expands 
Ada, C, or Fortran. In this case we know it can only be Ada, so the C++ memory
model is not mandatory. Maybe Eric can tell, if a data store race condition
may be an issue in Ada if  structure is laid out like __attribute((packed,aligned(1)))
I mean, if that is at all possible.

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

Ok, applied as-is.

Thanks
Bernd.

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