On Fri, 30 Aug 2013 11:47:21, Richard Biener wrote:
> On Tue, Jul 2, 2013 at 7:33 PM, DJ Delorie <d...@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.
