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

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

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

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

Thanks
Bernd.

> Richard.