Patchwork Default to -fstrict-volatile-bitfields for ARM EABI

login
register
mail settings
Submitter Jie Zhang
Date Sept. 2, 2010, 3:35 p.m.
Message ID <4C7FC42E.10208@codesourcery.com>
Download mbox | patch
Permalink /patch/63497/
State New
Headers show

Comments

Jie Zhang - Sept. 2, 2010, 3:35 p.m.
-fstrict-volatile-bitfields should also be default for ARM EABI. This 
patch does it. I also copied volatile-bitfields-1.c and 
volatile-bitfields-2.c from i386 target subdirectory and tweaked for 
ARM. For gcc.target/arm/volatile-bitfields-3.c, we need the change of 
stor-layout.c in this patch.

Tested for arm-none-eabi. There are some regressions for 
gcc.c-torture/execute/20010518-2.c. When compile the test, gcc reports 
the warning and note:

20010518-2.c: In function 'main':
20010518-2.c:34:21: warning: mis-aligned access used for structure 
member [-fstrict-volatile-bitfields]
20010518-2.c:34:21: note: When a volatile object spans multiple 
type-sized locations, the compiler must choose between using a single 
mis-aligned access to preserve the volatility, or using multiple aligned 
accesses to avoid runtime faults.  This code may fail at runtime if the 
hardware does not allow this access.

So I think this test should be disabled for all targets which default to 
-fstrict-volatile-bitfields. If this is what we should do, I will update 
my patch to disable it for all such targets.

Any comments?


Thanks,
Joseph S. Myers - Sept. 2, 2010, 3:55 p.m.
On Thu, 2 Sep 2010, Jie Zhang wrote:

> 20010518-2.c: In function 'main':
> 20010518-2.c:34:21: warning: mis-aligned access used for structure member
> [-fstrict-volatile-bitfields]
> 20010518-2.c:34:21: note: When a volatile object spans multiple type-sized
> locations, the compiler must choose between using a single mis-aligned access
> to preserve the volatility, or using multiple aligned accesses to avoid
> runtime faults.  This code may fail at runtime if the hardware does not allow
> this access.
> 
> So I think this test should be disabled for all targets which default to
> -fstrict-volatile-bitfields. If this is what we should do, I will update my
> patch to disable it for all such targets.

Why should it be disabled?  It should still execute OK; that's just a 
warning, and warnings aren't meant to cause c-torture tests to fail.
Jie Zhang - Sept. 2, 2010, 4:03 p.m.
On 09/02/2010 11:55 PM, Joseph S. Myers wrote:
> On Thu, 2 Sep 2010, Jie Zhang wrote:
>
>> 20010518-2.c: In function 'main':
>> 20010518-2.c:34:21: warning: mis-aligned access used for structure member
>> [-fstrict-volatile-bitfields]
>> 20010518-2.c:34:21: note: When a volatile object spans multiple type-sized
>> locations, the compiler must choose between using a single mis-aligned access
>> to preserve the volatility, or using multiple aligned accesses to avoid
>> runtime faults.  This code may fail at runtime if the hardware does not allow
>> this access.
>>
>> So I think this test should be disabled for all targets which default to
>> -fstrict-volatile-bitfields. If this is what we should do, I will update my
>> patch to disable it for all such targets.
>
> Why should it be disabled?  It should still execute OK; that's just a
> warning, and warnings aren't meant to cause c-torture tests to fail.
>
But GCC chooses to preserve the volatility in this case. It tries to use 
"ldr" to access unaligned address. This test fails at runtime on 
arm-none-eabi target at any optimization levels.
Joseph S. Myers - Sept. 2, 2010, 4:48 p.m.
On Fri, 3 Sep 2010, Jie Zhang wrote:

> On 09/02/2010 11:55 PM, Joseph S. Myers wrote:
> > On Thu, 2 Sep 2010, Jie Zhang wrote:
> > 
> > > 20010518-2.c: In function 'main':
> > > 20010518-2.c:34:21: warning: mis-aligned access used for structure member
> > > [-fstrict-volatile-bitfields]
> > > 20010518-2.c:34:21: note: When a volatile object spans multiple type-sized
> > > locations, the compiler must choose between using a single mis-aligned
> > > access
> > > to preserve the volatility, or using multiple aligned accesses to avoid
> > > runtime faults.  This code may fail at runtime if the hardware does not
> > > allow
> > > this access.
> > > 
> > > So I think this test should be disabled for all targets which default to
> > > -fstrict-volatile-bitfields. If this is what we should do, I will update
> > > my
> > > patch to disable it for all such targets.
> > 
> > Why should it be disabled?  It should still execute OK; that's just a
> > warning, and warnings aren't meant to cause c-torture tests to fail.
> > 
> But GCC chooses to preserve the volatility in this case. It tries to use "ldr"
> to access unaligned address. This test fails at runtime on arm-none-eabi
> target at any optimization levels.

That sounds like a bug in this option.  The test looks like perfectly 
valid GNU C to me and should not fail depending on the target.
Jie Zhang - Sept. 2, 2010, 4:59 p.m.
On 09/03/2010 12:48 AM, Joseph S. Myers wrote:
> On Fri, 3 Sep 2010, Jie Zhang wrote:
>
>> On 09/02/2010 11:55 PM, Joseph S. Myers wrote:
>>> On Thu, 2 Sep 2010, Jie Zhang wrote:
>>>
>>>> 20010518-2.c: In function 'main':
>>>> 20010518-2.c:34:21: warning: mis-aligned access used for structure member
>>>> [-fstrict-volatile-bitfields]
>>>> 20010518-2.c:34:21: note: When a volatile object spans multiple type-sized
>>>> locations, the compiler must choose between using a single mis-aligned
>>>> access
>>>> to preserve the volatility, or using multiple aligned accesses to avoid
>>>> runtime faults.  This code may fail at runtime if the hardware does not
>>>> allow
>>>> this access.
>>>>
>>>> So I think this test should be disabled for all targets which default to
>>>> -fstrict-volatile-bitfields. If this is what we should do, I will update
>>>> my
>>>> patch to disable it for all such targets.
>>>
>>> Why should it be disabled?  It should still execute OK; that's just a
>>> warning, and warnings aren't meant to cause c-torture tests to fail.
>>>
>> But GCC chooses to preserve the volatility in this case. It tries to use "ldr"
>> to access unaligned address. This test fails at runtime on arm-none-eabi
>> target at any optimization levels.
>
> That sounds like a bug in this option.  The test looks like perfectly
> valid GNU C to me and should not fail depending on the target.
>
Hmmm, there are no bitfields in the test case, but 
-fstrict-volatile-bitfields affected it. I will take a look tomorrow.
Jie Zhang - Sept. 3, 2010, 7:44 a.m.
On 09/03/2010 12:59 AM, Jie Zhang wrote:
> On 09/03/2010 12:48 AM, Joseph S. Myers wrote:
>> On Fri, 3 Sep 2010, Jie Zhang wrote:
>>
>>> On 09/02/2010 11:55 PM, Joseph S. Myers wrote:
>>>> On Thu, 2 Sep 2010, Jie Zhang wrote:
>>>>
>>>>> 20010518-2.c: In function 'main':
>>>>> 20010518-2.c:34:21: warning: mis-aligned access used for structure
>>>>> member
>>>>> [-fstrict-volatile-bitfields]
>>>>> 20010518-2.c:34:21: note: When a volatile object spans multiple
>>>>> type-sized
>>>>> locations, the compiler must choose between using a single mis-aligned
>>>>> access
>>>>> to preserve the volatility, or using multiple aligned accesses to
>>>>> avoid
>>>>> runtime faults. This code may fail at runtime if the hardware does not
>>>>> allow
>>>>> this access.
>>>>>
>>>>> So I think this test should be disabled for all targets which
>>>>> default to
>>>>> -fstrict-volatile-bitfields. If this is what we should do, I will
>>>>> update
>>>>> my
>>>>> patch to disable it for all such targets.
>>>>
>>>> Why should it be disabled? It should still execute OK; that's just a
>>>> warning, and warnings aren't meant to cause c-torture tests to fail.
>>>>
>>> But GCC chooses to preserve the volatility in this case. It tries to
>>> use "ldr"
>>> to access unaligned address. This test fails at runtime on arm-none-eabi
>>> target at any optimization levels.
>>
>> That sounds like a bug in this option. The test looks like perfectly
>> valid GNU C to me and should not fail depending on the target.
>>
> Hmmm, there are no bitfields in the test case, but
> -fstrict-volatile-bitfields affected it. I will take a look tomorrow.
>
This warning is somewhat misleading. There are no bitfields in the test 
case, but -fstrict-volatile-bitfields causes a warning.

However, this warning is correct at least for ARM EABI in someway. The 
"Procedure Call Standard for the ARM Architecture" says (7.1.5:

[quote]
A volatile qualification on a structure or union shall be interpreted as 
applying the qualification recursively to each of the fundamental data 
types of which it is composed. Access to a volatile-qualified 
fundamental data type must always be made by accessing the whole type.
[/quote]

So in 20010815-2.c, a->b should be accessed using 32-bit load/store 
instruction. But a->b is not aligned on 32-bit because the struct is 
packed. For ARMv7, which supports the unaligned memory access, it should 
be OK. But for ARMv6 or older ARM architectures, it might cause run time 
errors. From this perspective, this warning is useful to alert user for 
possible error, although there are still something to improve.

I have no knowledge about other targets which default to 
-fstrict-volatile-bitfields. How about disable this test only for 
arm*-*-eabi* for now?
Joseph S. Myers - Sept. 3, 2010, 11:25 a.m.
On Fri, 3 Sep 2010, Jie Zhang wrote:

> However, this warning is correct at least for ARM EABI in someway. The
> "Procedure Call Standard for the ARM Architecture" says (7.1.5:
> 
> [quote]
> A volatile qualification on a structure or union shall be interpreted as
> applying the qualification recursively to each of the fundamental data types
> of which it is composed. Access to a volatile-qualified fundamental data type
> must always be made by accessing the whole type.
> [/quote]
> 
> So in 20010815-2.c, a->b should be accessed using 32-bit load/store
> instruction. But a->b is not aligned on 32-bit because the struct is packed.

But the above is a description of an ABI for ISO C, where that issue does 
not arise.  For GNU C, where packed structures are permitted, clearly 
being packed should take precedence over being volatile here so that GNU C 
programs are properly portable across different architectures.
Jie Zhang - Sept. 3, 2010, 12:11 p.m.
On 09/03/2010 07:25 PM, Joseph S. Myers wrote:
> On Fri, 3 Sep 2010, Jie Zhang wrote:
>
>> However, this warning is correct at least for ARM EABI in someway. The
>> "Procedure Call Standard for the ARM Architecture" says (7.1.5:
>>
>> [quote]
>> A volatile qualification on a structure or union shall be interpreted as
>> applying the qualification recursively to each of the fundamental data types
>> of which it is composed. Access to a volatile-qualified fundamental data type
>> must always be made by accessing the whole type.
>> [/quote]
>>
>> So in 20010815-2.c, a->b should be accessed using 32-bit load/store
>> instruction. But a->b is not aligned on 32-bit because the struct is packed.
>
> But the above is a description of an ABI for ISO C, where that issue does
> not arise.  For GNU C, where packed structures are permitted, clearly
> being packed should take precedence over being volatile here so that GNU C
> programs are properly portable across different architectures.
>
Why being packed should take precedence over being volatile here? I 
would say it is more possible to be a program error on architectures 
without unaligned memory access instructions. Such program is certainly 
not portable. This warning would be a good reminder for user. One 
possible improvement is to make GCC only issue this warning when there 
are no unaligned memory access instructions on the target.
Joseph S. Myers - Sept. 3, 2010, 12:54 p.m.
On Fri, 3 Sep 2010, Jie Zhang wrote:

> > But the above is a description of an ABI for ISO C, where that issue does
> > not arise.  For GNU C, where packed structures are permitted, clearly
> > being packed should take precedence over being volatile here so that GNU C
> > programs are properly portable across different architectures.
> > 
> Why being packed should take precedence over being volatile here? I would say

Because it is not the job of an ABI to specify things conflicting with the 
underlying language, only to specify things not specified by that 
language.  It is unambiguous that GNU C programs using packed structures 
have particular semantics independent of the architecture, and that if the 
semantics of a program are defined without volatile qualifiers, adding 
such qualifiers should not cause the program to stop working.  If parts of 
the ABI only make sense for ISO C, not for GNU C, maybe GCC should 
document a GNU C-compatible adaptation of those parts for the GNU C 
extensions - but it should not implement a GNU C-incompatible variant.
Daniel Jacobowitz - Sept. 3, 2010, 3:22 p.m.
On Fri, Sep 03, 2010 at 12:54:25PM +0000, Joseph S. Myers wrote:
> On Fri, 3 Sep 2010, Jie Zhang wrote:
> 
> > > But the above is a description of an ABI for ISO C, where that issue does
> > > not arise.  For GNU C, where packed structures are permitted, clearly
> > > being packed should take precedence over being volatile here so that GNU C
> > > programs are properly portable across different architectures.
> > > 
> > Why being packed should take precedence over being volatile here? I would say
> 
> Because it is not the job of an ABI to specify things conflicting with the 
> underlying language, only to specify things not specified by that 
> language.  It is unambiguous that GNU C programs using packed structures 
> have particular semantics independent of the architecture, and that if the 
> semantics of a program are defined without volatile qualifiers, adding 
> such qualifiers should not cause the program to stop working.  If parts of 
> the ABI only make sense for ISO C, not for GNU C, maybe GCC should 
> document a GNU C-compatible adaptation of those parts for the GNU C 
> extensions - but it should not implement a GNU C-incompatible variant.

How would you suggest we solve this issue?  The ARM ABI describes how
you have to access a volatile "long x : 31", and having that use a
single unaligned ldr but "volatile long x" use byte accesses is IMO
not a service to users.  I think the current state, where we get a
warning, is fine.  A volatile packed structure with unaligned word
members is not going to be a common occurance.  And we do want
-fstrict-volatile-bitfields to apply, IMO; if the packed item is a
short, we should use a short to access it.
Joseph S. Myers - Sept. 3, 2010, 3:33 p.m.
On Fri, 3 Sep 2010, Daniel Jacobowitz wrote:

> How would you suggest we solve this issue?  The ARM ABI describes how
> you have to access a volatile "long x : 31", and having that use a

How you have to access it *in the ISO C context where it is within an 
aligned container*.  My claim is that this is obviously inapplicable to a 
GNU C packed structure case where the compiler cannot be sure a single 
load instruction would work.
Mark Mitchell - Sept. 3, 2010, 5:40 p.m.
On 9/3/2010 8:33 AM, Joseph S. Myers wrote:

>> How would you suggest we solve this issue?  The ARM ABI describes how
>> you have to access a volatile "long x : 31", and having that use a
> 
> How you have to access it *in the ISO C context where it is within an 
> aligned container*.  My claim is that this is obviously inapplicable to a 
> GNU C packed structure case where the compiler cannot be sure a single 
> load instruction would work.

I think I agree with Joseph here.  As soon as you put "packed" in play,
you've left ISO C behind, and I think you've also left ARM's ABI behind.
 It just doesn't say anything about this situation, so we get to pick.
I don't think a warning is necessarily in appropriate, but the test
should still execute correctly.

I think that means that DJ's patch should be modified to disable what
it's doing, at least when the structure is packed, on machines where
what it is doing is likely to result in a fault.

Patch


	* stor-layout.c (layout_decl): Use the field's type to
	determine the mode and keep DECL_BIT_FIELD for a volatile
	bit-field.
	* config/arm/arm.c (arm_override_options): Default to
	-fstrict-volatile-bitfields.

	testsuite/
	* gcc.target/arm/volatile-bitfields-1.c: New test.
	* gcc.target/arm/volatile-bitfields-2.c: New test.
	* gcc.target/arm/volatile-bitfields-3.c: New test.

Index: testsuite/gcc.target/arm/volatile-bitfields-1.c
===================================================================
--- testsuite/gcc.target/arm/volatile-bitfields-1.c	(revision 0)
+++ testsuite/gcc.target/arm/volatile-bitfields-1.c	(revision 0)
@@ -0,0 +1,17 @@ 
+/* { dg-do compile { target arm*-*-eabi* } } */
+/* { dg-options "-O2" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "ldrb\[\\t \]+.*,\[\\t \]*\\\[.*\\\]" } } */
Index: testsuite/gcc.target/arm/volatile-bitfields-2.c
===================================================================
--- testsuite/gcc.target/arm/volatile-bitfields-2.c	(revision 0)
+++ testsuite/gcc.target/arm/volatile-bitfields-2.c	(revision 0)
@@ -0,0 +1,17 @@ 
+/* { dg-do compile { target arm*-*-eabi* } } */
+/* { dg-options "-O2" } */
+
+typedef struct {
+  volatile unsigned long a:8;
+  volatile unsigned long b:8;
+  volatile unsigned long c:16;
+} BitStruct;
+
+BitStruct bits;
+
+unsigned long foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "ldr\[\\t \]+.*,\[\\t \]*\\\[.*\\\]" } } */
Index: testsuite/gcc.target/arm/volatile-bitfields-3.c
===================================================================
--- testsuite/gcc.target/arm/volatile-bitfields-3.c	(revision 0)
+++ testsuite/gcc.target/arm/volatile-bitfields-3.c	(revision 0)
@@ -0,0 +1,17 @@ 
+/* { dg-do compile { target arm*-*-eabi* } } */
+/* { dg-options "-O2" } */
+
+typedef struct {
+  volatile unsigned long a:8;
+  volatile unsigned long b:8;
+  volatile unsigned long c:16;
+} BitStruct;
+
+BitStruct bits;
+
+unsigned long foo ()
+{
+  return bits.c;
+}
+
+/* { dg-final { scan-assembler "ldr\[\\t \]+.*,\[\\t \]*\\\[.*\\\]" } } */
Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 163756)
+++ stor-layout.c	(working copy)
@@ -591,11 +591,14 @@  layout_decl (tree decl, unsigned int kno
 	    }
 
 	  /* See if we can use an ordinary integer mode for a bit-field.
-	     Conditions are: a fixed size that is correct for another mode
-	     and occupying a complete byte or bytes on proper boundary.  */
+	     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.  */
 	  if (TYPE_SIZE (type) != 0
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
-	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
+	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT
+	      && !(TREE_THIS_VOLATILE (decl)
+		   && flag_strict_volatile_bitfields > 0))
 	    {
 	      enum machine_mode xmode
 		= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 163756)
+++ config/arm/arm.c	(working copy)
@@ -1916,6 +1916,10 @@  arm_override_options (void)
        calculation, which is 2 instructions.  */
     set_param_value ("gcse-unrestricted-cost", 2);
 
+  /* ARM EABI defaults to strict volatile bitfields.  */
+  if (TARGET_AAPCS_BASED && flag_strict_volatile_bitfields < 0)
+    flag_strict_volatile_bitfields = 1;
+
   /* Register global variables with the garbage collector.  */
   arm_add_gc_roots ();
 }