diff mbox

-fstrict-volatile-bitfields fixes

Message ID 4CF7F356.8020506@t-online.de
State New
Headers show

Commit Message

Bernd Schmidt Dec. 2, 2010, 7:28 p.m. UTC 
The patch below fixes two problems with -fstrict-volatile-bitfields.

As the ARM testcase shows, writing to an 8-bit bitfield at source level
can cause the compiler to emit a plain 32-bit store, rather than a
read-modify-write operation. This is fixed by the patch in expr.c,
detecting when the mode we want to use for the access is larger than the
bit field.

The other problem is that the option only has an effect on bitfields
declared as volatile. However, when an object is cast to volatile, it
has no effect. We have a customer who thinks that is undesirable, and I
agree. This is fixed in stor-layout.c.

The second fix caused a failure in g++/abi/bitfield12.C, where a warning
message is missing, since the C++ frontend apparently expects to see a
different mode. I discussed this with Mark, and the end result was to
disallow the combination of -fabi-version=1 and
-fstrict-volatile-bitfields. This makes sense in that when an old ABI
version is explicitly selected, we should really try not to change
behaviour.

Bootstrapped and regression tested on i686-linux. Also tested in an
internal 4.5-based tree on arm-linux. Ok?


Bernd
gcc/
	* expr.c (store_field): Avoid a direct store if the mode is larger
	than the size of the bit field.
	* stor-layout.c (layout_decl): If flag_strict_volatile_bitfields,
	treat non-volatile bit fields like volatile ones.
	* toplev.c (process_options): Disallow combination of
	-fstrict-volatile-bitfields and ABI versions less than 2.
	* config/arm/arm.c (arm_option_override): Don't enable
	flag_strict_volatile_bitfields if the ABI version is less than 2.
	* config/h8300/h8300.c (h8300_option_override): Likewise.
	* config/rx/rx.c (rx_option_override): Likewise.
	* config/m32c/m32c.c (m32c_option_override): Likewise.
	* config/sh/sh.c (sh_option_override): Likewise.

	gcc/testsuite/
	* gcc.target/arm/volatile-bitfields-4.c: New test.
	* c-c++-common/abi-bf.c: New test.

Comments

DJ Delorie Dec. 3, 2010, 12:35 a.m. UTC | #1 
I see no reason to flag RX or M32C with "abi_version_at_least(2)"
since those ports are newer than abi version 2.

For sh, not conforming to the volatile bitfields' mode causes a
hardware problem.  If specifying the ABI changes the silicon to no
longer have this problem, I suppose we could check for it.  Otherwise,
sh shouldn't check the ABI either.

Also, why is the C++ ABI version determining the behavior of non-C++
programs?
Mark Mitchell Dec. 5, 2010, 6:38 p.m. UTC | #2 
On 12/2/2010 6:35 PM, DJ Delorie wrote:

> I see no reason to flag RX or M32C with "abi_version_at_least(2)"
> since those ports are newer than abi version 2.

In that case, the solution for those ports is to refuse -fabi-version=2
(or less); just issue an error message if that option is used.

> For sh, not conforming to the volatile bitfields' mode causes a
> hardware problem.  If specifying the ABI changes the silicon to no
> longer have this problem, I suppose we could check for it.  Otherwise,
> sh shouldn't check the ABI either.

Similarly, here -- if the SH hardware can't work with the old ABI then
we should probably just error out.

> Also, why is the C++ ABI version determining the behavior of non-C++
> programs?

That's a valid question -- but after we make it an error to try to use
an old ABI version, this impact will go away -- except on ARM, and only
on ARM if the ABI version is explicitly specified, which would be very
odd if not compiling C++.  (I don't even know if we accept the ABI
option when compiling C.)
Bernd Schmidt Dec. 6, 2010, 12:14 p.m. UTC | #3 
On 12/05/2010 07:38 PM, Mark Mitchell wrote:
> On 12/2/2010 6:35 PM, DJ Delorie wrote:
> 
>> I see no reason to flag RX or M32C with "abi_version_at_least(2)"
>> since those ports are newer than abi version 2.
> 
> In that case, the solution for those ports is to refuse -fabi-version=2
> (or less); just issue an error message if that option is used.
> 
>> For sh, not conforming to the volatile bitfields' mode causes a
>> hardware problem.  If specifying the ABI changes the silicon to no
>> longer have this problem, I suppose we could check for it.  Otherwise,
>> sh shouldn't check the ABI either.
> 
> Similarly, here -- if the SH hardware can't work with the old ABI then
> we should probably just error out.

I don't think so. If a program requests the old ABI, and volatile
bitfields do not work with the old ABI, we can conclude that the program
does not use volatile bitfields.

In any case, I think the patch is a reasonable starting point, and
target maintainers can change behavior if they see fit. Ok to commit, or
what specific changes should I make?


Bernd
Mark Mitchell Dec. 7, 2010, 1:06 a.m. UTC | #4 
On 12/6/2010 6:14 AM, Bernd Schmidt wrote:

> I don't think so. If a program requests the old ABI, and volatile
> bitfields do not work with the old ABI, we can conclude that the program
> does not use volatile bitfields.

Well, maybe so.  But, support for a couple of the architectures that DJ
mentioned post-dates the older ABI versions.  My feeling is that older
versions of the ABI were buggy, so the only reason to support them is
backwards compatibility.  If GCC support post-dates the ABI, there's no
reason to provide that backwards compatibility.

> In any case, I think the patch is a reasonable starting point, and
> target maintainers can change behavior if they see fit. Ok to commit, or
> what specific changes should I make?

I guess we need to settle the question above before we can figure that out?
DJ Delorie Dec. 7, 2010, 6:04 a.m. UTC | #5 
> I guess we need to settle the question above before we can figure
> that out?

My concern was about adding code that isn't neccessary, and perhaps
irrelevent, and perhaps even misleading.  I don't think the proposed
patch would *break* things for an unwary user, as they just wouldn't
use the -fabi option.
diff mbox

Patch

Index: src/gcc-4.5-2010.09/gcc/toplev.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/toplev.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/toplev.c	(working copy)
@@ -1851,6 +1851,13 @@ 
     sorry ("Graphite loop optimizations cannot be used");
 #endif
 
+  if (flag_strict_volatile_bitfields > 0 && !abi_version_at_least (2))
+    {
+      warning (0, "-fstrict-volatile-bitfield disabled; "
+	       "it is incompatible with ABI versions < 2");
+      flag_strict_volatile_bitfields = 0;
+    }
+
   /* Unrolling all loops implies that standard loop unrolling must also
      be done.  */
   if (flag_unroll_all_loops)
Index: src/gcc-4.5-2010.09/gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c	(revision 0)
+++ src/gcc-4.5-2010.09/gcc/testsuite/gcc.target/arm/volatile-bitfields-4.c	(revision 0)
@@ -0,0 +1,30 @@ 
+/* { dg-require-effective-target arm_eabi } */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { 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" } } */
+
+struct thing {
+  unsigned a: 8;
+  unsigned b: 8;
+  unsigned c: 8;
+  unsigned d: 8;
+};
+
+struct thing2 {
+  volatile unsigned a: 8;
+  volatile unsigned b: 8;
+  volatile unsigned c: 8;
+  volatile unsigned d: 8;
+};
+
+void test1(volatile struct thing *t)
+{
+  t->a = 5;
+}
+
+void test2(struct thing2 *t)
+{
+  t->a = 5;
+}
Index: src/gcc-4.5-2010.09/gcc/testsuite/c-c++-common/abi-bf.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/testsuite/c-c++-common/abi-bf.c	(revision 0)
+++ src/gcc-4.5-2010.09/gcc/testsuite/c-c++-common/abi-bf.c	(revision 0)
@@ -0,0 +1,3 @@ 
+/* { dg-warning "incompatible" } */
+/* { dg-do compile } */
+/* { dg-options "-fstrict-volatile-bitfields -fabi-version=1" } */
Index: src/gcc-4.5-2010.09/gcc/expr.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/expr.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/expr.c	(working copy)
@@ -5848,6 +5848,8 @@ 
 		|| bitpos % GET_MODE_ALIGNMENT (mode))
 	       && SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (target)))
 	      || (bitpos % BITS_PER_UNIT != 0)))
+      || (bitsize >= 0 && mode != BLKmode
+	  && GET_MODE_BITSIZE (mode) > bitsize)
       /* If the RHS and field are a constant size and the size of the
 	 RHS isn't the same size as the bitfield, we must use bitfield
 	 operations.  */
Index: src/gcc-4.5-2010.09/gcc/stor-layout.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/stor-layout.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/stor-layout.c	(working copy)
@@ -595,12 +595,13 @@ 
 	  /* See if we can use an ordinary integer mode for a bit-field.
 	     Conditions are: a fixed size that is correct for another mode,
 	     occupying a complete byte or bytes on proper boundary,
-	     and not volatile or not -fstrict-volatile-bitfields.  */
+	     and not -fstrict-volatile-bitfields.  If the latter is set,
+	     we unfortunately can't check TREE_THIS_VOLATILE, as a cast
+	     may make a volatile object later.  */
 	  if (TYPE_SIZE (type) != 0
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
 	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT
-	      && !(TREE_THIS_VOLATILE (decl)
-		   && flag_strict_volatile_bitfields > 0))
+	      && flag_strict_volatile_bitfields <= 0)
 	    {
 	      enum machine_mode xmode
 		= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
Index: src/gcc-4.5-2010.09/gcc/config/m32c/m32c.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/config/m32c/m32c.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/config/m32c/m32c.c	(working copy)
@@ -430,7 +430,7 @@ 
     flag_ivopts = 0;
 
   /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0)
+  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
     flag_strict_volatile_bitfields = 1;
 }
 
Index: src/gcc-4.5-2010.09/gcc/config/rx/rx.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/config/rx/rx.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/config/rx/rx.c	(working copy)
@@ -2191,7 +2191,7 @@ 
 rx_option_override (void)
 {
   /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0)
+  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
     flag_strict_volatile_bitfields = 1;
 }
 
Index: src/gcc-4.5-2010.09/gcc/config/sh/sh.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/config/sh/sh.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/config/sh/sh.c	(working copy)
@@ -974,7 +974,7 @@ 
     sh_fix_range (sh_fixed_range_str);
 
   /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0)
+  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
     flag_strict_volatile_bitfields = 1;
 }
 
Index: src/gcc-4.5-2010.09/gcc/config/arm/arm.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/config/arm/arm.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/config/arm/arm.c	(working copy)
@@ -1952,7 +1952,8 @@ 
     set_param_value ("gcse-unrestricted-cost", 2);
 
   /* ARM EABI defaults to strict volatile bitfields.  */
-  if (TARGET_AAPCS_BASED && flag_strict_volatile_bitfields < 0)
+  if (TARGET_AAPCS_BASED && flag_strict_volatile_bitfields < 0
+      && abi_version_at_least(2))
     flag_strict_volatile_bitfields = 1;
 
   /* Register global variables with the garbage collector.  */
Index: src/gcc-4.5-2010.09/gcc/config/h8300/h8300.c
===================================================================
--- src/gcc-4.5-2010.09/gcc/config/h8300/h8300.c	(revision 307813)
+++ src/gcc-4.5-2010.09/gcc/config/h8300/h8300.c	(working copy)
@@ -405,7 +405,7 @@ 
     }
 
   /* This target defaults to strict volatile bitfields.  */
-  if (flag_strict_volatile_bitfields < 0)
+  if (flag_strict_volatile_bitfields < 0 && abi_version_at_least(2))
     flag_strict_volatile_bitfields = 1;
 }