Message ID | 51D0F6A0.4010307@codesourcery.com |
---|---|
State | New |
Headers | show |
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 ;-)
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" } } */