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

Since my previous idea of resolving conflicts between 
-fstrict-volatile-bitfields behavior and the new C/C++ memory model 
depending on the selected -std= option was shot down

http://gcc.gnu.org/ml/gcc-patches/2013-06/msg01163.html

it appears that the only way to make -fstrict-volatile-bitfields do 
something remotely resembling what it is supposed to do in a way that is 
acceptable to the maintainers is to make it not be the default on any 
target any more, so that the new memory model always takes precedence 
over ABI specifications by default.  So, here is a patch to do it.

The affected targets are aarch64, arm, h8300, m32c, rx, and sh.  The 
code change is identical on all targets, but I have only been able to 
test on arm (arm-none-eabi).  I have included tweaks to pass 
-fstrict-volatile-bitfields explicitly on the test cases that failed 
when the default was removed.  It might be that some tests for other 
targets need similar changes as well.

OK to commit in conjunction with the other patches in this series?

-Sandra

Given how much trouble I went through to make it the default, I'd
rather not revert all that work...  especially since the flag is
*required* for proper operation of the hardware on many of these
targets.

This patch will, or course, silently and obscurely break existing
code.

On Sun, Jun 30, 2013 at 8:32 PM, DJ Delorie <dj@redhat.com> wrote:
>
> Given how much trouble I went through to make it the default, I'd
> rather not revert all that work...  especially since the flag is
> *required* for proper operation of the hardware on many of these
> targets.
>
> This patch will, or course, silently and obscurely break existing
> code.

And without the patch will break silently existing valid C11/C++11
code on many targets.  This is the whole point of the patch to follow
the C/C++ standard here rather than breaking valid code.

I rather see volatile on bitfields becoming an error rather than
either of these patches.

Thanks,
Andrew Pinski

On 06/30/2013 09:32 PM, DJ Delorie wrote:
>
> Given how much trouble I went through to make it the default, I'd
> rather not revert all that work...  especially since the flag is
> *required* for proper operation of the hardware on many of these
> targets.
>
> This patch will, or course, silently and obscurely break existing
> code.

My previous argument to this effect fell on deaf ears.

The sad truth is that strict volatile bitfields are already broken on 
ARM and other targets, and have been broken ever since the bit range 
support was added to support the new C/C++ memory model in gcc 4.7-ish. 
  The current default behavior on ARM does not conform to either the ABI 
or the C/C++ model, so it's the worst of both worlds.

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.

-Sandra

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

On Mon, 1 Jul 2013, Andrew Pinski wrote:

> On Sun, Jun 30, 2013 at 8:32 PM, DJ Delorie <dj@redhat.com> wrote:
> >
> > Given how much trouble I went through to make it the default, I'd
> > rather not revert all that work...  especially since the flag is
> > *required* for proper operation of the hardware on many of these
> > targets.
> >
> > This patch will, or course, silently and obscurely break existing
> > code.
>
> And without the patch will break silently existing valid C11/C++11
> code on many targets.  This is the whole point of the patch to follow
> the C/C++ standard here rather than breaking valid code.
>
> I rather see volatile on bitfields becoming an error rather than
> either of these patches.

Or - maybe more acceptable - an optional warning, say
-Wportable-volatility, to warn about code for which separate
incompatbile definitions on different platforms (or between C
and C++) exist even within gcc.  It would be usable for driver
code you want to be usable on different architectures, say, in
an OS commonly compiled with gcc, if you can think of some. :)

brgds, H-P
PS. Sorry, not currently planning on this.

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

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;

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.

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.

Richard.

In all the real-world cases I've seen, the vendor/programmer was
careful to lay out the structs properly.  I think if the struct does
not reflect a reasonable (or physically possible) layout, oh well ;-)