Re: [patch 4/4] -fstrict-volatile-bitfields cleanup v3: remove from defaults on all targets

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

On Sun, Sep 1, 2013 at 3:10 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On Fri, 30 Aug 2013 11:47:21, Richard Biener wrote:
>> On Tue, Jul 2, 2013 at 7:33 PM, DJ Delorie <dj@redhat.com> wrote:
>>>
>>>> The choice appears to be to continue to have broken volatile bitfields
>>>> on ARM with no way for users to make them conform to the ABI, or to
>>>> change things so that they conform to the ABI if you specify
>>>> -fstrict-volatile-bitfields explicitly and to the C/C++ standard by
>>>> default, without that option.
>>>
>>> I can't speak for ARM, but for the other targets (for which I wrote
>>> the original patch), the requirement is that volatile bitfield
>>> accesses ALWAYS be in the mode of the type specified by the user.  If
>>> the user says "int x:8;" then SImode (assuming 32-bit ints) must
>>> always be used, even if QImode could be used.
>>>
>>> The reason for this is that volatile bitfields are normally used for
>>> memory-mapped peripherals, and accessing peripheral registers in the
>>> wrong mode leads to incorrect operation.
>>
>> I've argued in the past that this part of the semantics can be easily
>> implemented in the existing C++ memory model code (well, in
>> get-bit-range) for the cases where it doesn't conflict with the C++
>> memory model.  For the cases where it conflicts a warning can
>> be emitted, Like when you do
>>
>> struct {
>> volatile int x:8;
>> char c;
>> } x;
>>
>> where 'c' would be within the SImode you are supposed to use
>> with -fstrict-volatile-bitfields.
>>
>> Which raises the question ... how does
>>
>> x.x = 1;
>>
>> actually work?  IMHO it must be sth horribly inefficient like
>>
>>  tem = x.c; // QImode
>>  tem = tem << 8 | 1; // combine into SImode value
>>  x.x = tem; // SImode store
>>
>
> AAPCS is very explicit what should happen here:
>  tem = x.x; // SImode
>  tem = (tem & ~0xFF) | 1;
>  x.x = tem; // SImode
>
>  struct x should be 4 bytes large, and 4 bytes aligned (int x)
>  the member c is at offset 1, and gets overwritten which is
>  forbidden in C++ memory model, but required by AAPCS

Ok, so what I think is weird here is that the volatile store to x.x is replaced
with a read and a store - for hardware I/O that's exactly what I would not
have expected if I qualify sth with volatile.  In my simple mind 'volatile'
preserves the exact number of loads and stores.

Of course I was wrong here ;)

>> hoping that no sane ABI makes 'x' size 2.  Oh, I _can_ make it size 2:
>>
>> struct {
>> volatile int x:8;
>> char c;
>> } __attribute__((packed)) x;
>> char y;
>>
>
> IMHO the AAPCS forbids packed structures. Therefore we need not
> interfere with the C++ memory model if we have unaligned data.
>
>> note the fancy global object 'y' I placed after 'x'.  Now the store will
>> clobber y(?)  So the only 'valid' way is
>>
>>   tem = x.x; // SImode read(!)
>>   tem = tem & 0xff..00 | 1; // manipulate value
>>   x.x = tem; // SImode store
>>
>> but then this doesn't work either because that 1-byte aligned object
>> could reside at a page boundary and so the read and write would trap.
>>
>
> That is exactly what happened see bug#56341, and it is fixed with
> parts 1 & 2 of Sandra's patch. Here because if the 1-byte alignment
> the function strict_volatile_bitfield_p() returns false and thus the
> C++ memory model is used.
>
>> Makes me ask who designed that crap ;)
>>
>> But my point was that for all these special cases that likely do not
>> happen in practice (fingers crossed) the C++ memory model way
>> doesn't interfere with -fstrict-volatile-bitfields and the code can be
>> perfectly used to make the code as close as possible to the
>> -fstrict-volatile-bitifeld ABI.
>>
>
> Actually the C++ memory model and -fstrict-volatile-bitfields have
> some significant differences, your first example is one of them.
>
> And since part 4 changes the default of -fstrict-volatile-bitfields,
> I thought it would be good to have a warning when such a construct
> is used, which generates inherently different code if
> -fstrict-volatile-bitfields is used or not.
>
> I personally could live with or without part 4 of the patch,
> and I do not insist on the warnings part either. That was only
> my try to find a way how part 4 of Sandra's patch could be generally
> accepted.
>
> Note: If you want I can re-post the warnings patch in a new thread.

Can someone, in a new thread, ping the patches that are still in
flight?  ISTR having approved bits of some patches before my leave.

Thanks,
Richard.

> Thanks
> Bernd.
>
>> Richard.