| Message ID | 4C7FC42E.10208@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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.
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?
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.
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.
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.
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.
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.
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.
* 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 (); }