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

login
register
mail settings
Submitter Sandra Loosemore
Date July 1, 2013, 3:25 a.m.
Message ID <51D0F6A0.4010307@codesourcery.com>
Download mbox | patch
Permalink /patch/256000/
State New
Headers show

Comments

Sandra Loosemore - July 1, 2013, 3:25 a.m.
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
DJ Delorie - July 1, 2013, 3:32 a.m.
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.
Andrew Pinski - July 1, 2013, 7:33 a.m.
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
Sandra Loosemore - July 1, 2013, 2:58 p.m.
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
DJ Delorie - July 2, 2013, 5:33 p.m.
> 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.
Hans-Peter Nilsson - July 5, 2013, 12:14 p.m.
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.
Richard Guenther - Aug. 30, 2013, 9:47 a.m.
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.
DJ Delorie - Aug. 30, 2013, 6:13 p.m.
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 ;-)

Patch

Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c	(revision 199963)
+++ gcc/config/aarch64/aarch64.c	(working copy)
@@ -4890,10 +4890,6 @@  aarch64_override_options (void)
 
   aarch64_build_bitmask_table ();
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least (2))
-    flag_strict_volatile_bitfields = 1;
-
   /* If the user did not specify a processor, choose the default
      one for them.  This will be the CPU set during configuration using
      --with-cpu, otherwise it is "generic".  */
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 199963)
+++ gcc/config/arm/arm.c	(working copy)
@@ -2136,11 +2136,6 @@  arm_option_override (void)
 			   global_options.x_param_values,
 			   global_options_set.x_param_values);
 
-  /* ARM EABI defaults to strict volatile bitfields.  */
-  if (TARGET_AAPCS_BASED && flag_strict_volatile_bitfields < 0
-      && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   /* Enable sw prefetching at -O3 for CPUS that have prefetch, and we have deemed
      it beneficial (signified by setting num_prefetch_slots to 1 or more.)  */
   if (flag_prefetch_loop_arrays < 0
Index: gcc/config/h8300/h8300.c
===================================================================
--- gcc/config/h8300/h8300.c	(revision 199963)
+++ gcc/config/h8300/h8300.c	(working copy)
@@ -437,10 +437,6 @@  h8300_option_override (void)
 	 restore er6 though, so bump up the cost.  */
       h8300_move_ratio = 6;
     }
-
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
 }
 
 /* Return the byte register name for a register rtx X.  B should be 0
Index: gcc/config/m32c/m32c.c
===================================================================
--- gcc/config/m32c/m32c.c	(revision 199963)
+++ gcc/config/m32c/m32c.c	(working copy)
@@ -416,10 +416,6 @@  m32c_option_override (void)
   if (TARGET_A24)
     flag_ivopts = 0;
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   /* r8c/m16c have no 16-bit indirect call, so thunks are involved.
      This is always worse than an absolute call.  */
   if (TARGET_A16)
Index: gcc/config/rx/rx.c
===================================================================
--- gcc/config/rx/rx.c	(revision 199963)
+++ gcc/config/rx/rx.c	(working copy)
@@ -2691,10 +2691,6 @@  rx_option_override (void)
 	  }
       }
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   rx_override_options_after_change ();
 
   if (align_jumps == 0 && ! optimize_size)
Index: gcc/config/sh/sh.c
===================================================================
--- gcc/config/sh/sh.c	(revision 199963)
+++ gcc/config/sh/sh.c	(working copy)
@@ -1020,10 +1020,6 @@  sh_option_override (void)
   if (sh_fixed_range_str)
     sh_fix_range (sh_fixed_range_str);
 
-  /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
-    flag_strict_volatile_bitfields = 1;
-
   /* Parse atomic model option and make sure it is valid for the current
      target CPU.  */
   selected_atomic_model_
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 199963)
+++ gcc/doc/invoke.texi	(working copy)
@@ -20895,8 +20895,11 @@  attribute, the access is done honoring t
 GCC assumes that the user knows something about the target hardware
 that it is unaware of.
 
-The default value of this option is determined by the application binary
-interface for the target processor.
+Note that in some cases this option overrides the memory model
+specified in recent versions of the C and C++ standards.  For this
+reason, @option{-fstrict-volatile-bitfields} is not enabled by default
+on any target, even those where the application binary interface for
+the target processor requires this behavior.
 
 @item -fsync-libcalls
 @opindex fsync-libcalls
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-1.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-1.c	(revision 199963)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-1.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 
 typedef struct {
   char a:1;
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-2.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-2.c	(revision 199963)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-2.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 
 typedef struct {
   volatile unsigned long a:8;
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-3.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-3.c	(revision 199963)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-3.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 
 typedef struct {
   volatile unsigned long a:8;
Index: gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c
===================================================================
--- gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c	(revision 199963)
+++ gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* { dg-require-effective-target arm_eabi } */
 /* { dg-do compile } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
 /* { dg-final { scan-assembler-times "ldr\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" 2 } } */
 /* { dg-final { scan-assembler-times "str\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" 2 } } */
 /* { dg-final { scan-assembler-not "strb" } } */