Bug 56997 - Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
Incorrect write to packed field when strict-volatile-bitfields enabled on aar...
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: target
4.9.0
: P3 normal
: ---
Assigned To: Not yet assigned to anyone
: wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-18 08:05 UTC by Joey Ye
Modified: 2014-08-15 16:36 UTC (History)
6 users (show)

See Also:
Host:
Target: arm/aarch32
Build:
Known to work:
Known to fail: 4.6.0, 4.7.0, 4.8.0
Last reconfirmed: 2013-08-28 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joey Ye 2013-04-18 08:05:02 UTC
Attached case fails when
Target: all arm aarch32
Optimization level:all optimization levels.
Version: Trunk 197955, 4.7, 4.6

But it passes:
- if -fno-strict-volatile-bitfields is specified, or
- on x86 even if -fstrict-volatile-bitfields is specified

For example:

arm-none-eabi-gcc -mthumb -mcpu=cortex-m0 -Os  a.c -o a.s -S

foo:
        ldr     r3, .L2
        lsl     r2, r0, #8  ; First byte of input r0 is truncated
        ldr     r0, [r3]
        uxtb    r0, r0
        orr     r0, r2
        str     r0, [r3]
        bx      lr

Runtime output:
Write FAIL 0x20304

---------------------
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#ifdef SHORT
#define test_type unsigned short
#define MAGIC 0x102u
#else
#define test_type unsigned int
#define MAGIC 0x1020304u
#endif

typedef struct s{
 unsigned char Prefix;
 test_type Type;
}__attribute((__packed__)) ss;

volatile ss v;
ss g;

void __attribute__((noinline))
foo (test_type u)
{
  v.Type = u;
}

test_type __attribute__((noinline))
bar (void)
{
  return v.Type;
}

int main()
{
  test_type temp;
  int err = 0;
  foo(MAGIC);
  memcpy(&g, (void *)&v, sizeof(g));
  if (g.Type != MAGIC)
    {
      printf("Write FAIL 0x%x\n", g.Type);
      err ++;
    }

  g.Type = MAGIC;
  memcpy((void *)&v, &g, sizeof(v));
  temp = bar();
  if (temp != MAGIC)
    {
      printf("Read FAIL 0x%x\n", temp);
      err ++;
    }
  return err;
}
Comment 1 Joey Ye 2013-04-18 08:12:50 UTC
Quoted from http://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/Code-Gen-Options.html#Code-Gen-Options:

-fstrict-volatile-bitfields
    If the target requires strict alignment, and honoring the field type would require violating this alignment, a warning is issued. If the field has packed attribute, the access is done without honoring the field type. If the field doesn't have packed attribute, the access is done honoring the field type. In both cases, GCC assumes that the user knows something about the target hardware that it is unaware of.

There are two issues in current behavior:
1. There is no warning when writing to packed fields requiring strict alignment. Although there is a warning when reading it.
2. The write access to packed field still honors the field type.
Comment 2 Richard Biener 2013-04-18 08:26:31 UTC
-fstrict-volatile-bitfields implementation is bogus, as I repeatedly said
it should now piggy-back on DECL_BIT_FIELD_REPRESENTATIVE.  Note that
in your testcase no bitfields are involved so how does it relate to
-f[no-]strict-volatile-bitfields?  Isn't this simply a wrong-code bug
(eventually caused by the bogus implementation of -fstrict-volatile-bitfields)?
Comment 3 Joey Ye 2013-04-18 08:46:36 UTC
(In reply to comment #2)
> -fstrict-volatile-bitfields implementation is bogus, as I repeatedly said
> it should now piggy-back on DECL_BIT_FIELD_REPRESENTATIVE.  Note that
> in your testcase no bitfields are involved so how does it relate to
> -f[no-]strict-volatile-bitfields?  Isn't this simply a wrong-code bug
> (eventually caused by the bogus implementation of -fstrict-volatile-bitfields)?

It also looks to me like a wrong-code bug caused by -fstrict-volatile-bitfields.
Comment 4 Sandra Loosemore 2013-06-08 19:58:59 UTC
I'm working on a fix for this.
Comment 5 Sandra Loosemore 2013-06-14 03:01:36 UTC
Patch posted here:

http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00750.html
Comment 6 Bernd Edlinger 2013-06-23 18:01:52 UTC
(In reply to Sandra Loosemore from comment #5)
> Patch posted here:

http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00750.html

Hi Sandra,

I tried your patch, but I dont like the code that it generates:

    printf("%x\n", (unsigned int)g.b);
    g.b = 0xAAAAAAA;

is compiled to invalid code (in ARMv5)

        ldr     r4, .L2
        ldr     r1, [r4]
        ldr     r3, [r4, #4]
        and     r3, r3, #7
        mov     r3, r3, asl #25
        orr     r1, r3, r1, lsr #7
        ldr     r0, .L2+4
        bl      printf
        ldr     r2, [r4]
        ldr     r3, .L2+8
        and     r2, r2, #127
        orr     r3, r2, r3
        str     r3, [r4]
        ldr     r3, [r4, #4]
        bic     r3, r3, #7
        orr     r3, r3, #5
        str     r3, [r4, #4]

code is invalid because: the object "g" is only 5 bytes large,
but the first statement reads 2x4 bytes, and ignores the 3 extra bytes.
this can fault if g is close to a segment boundary.
The second statement reads the 3 bytes beyond g and writes them
unmodified back. That is problematic if a task switch occurs between the
read and store sequence, and the other task modifies something in the 3 bytes.

Previous versions of gcc produced single 5x1 byte read/write sequences for
that structure, as does apparently the x86 version.

Regards
Bernd.
Comment 7 Bernd Edlinger 2013-06-23 22:03:37 UTC
aehmm sorry, the object "g" from above code is actually from PR#48784

#pragma pack(1)
volatile struct S0 {
   signed a : 7;
   unsigned b : 28;
} g = {0,-1};

=> sizeof(g) = 5

but the code from this example has pretty much the same problems:

typedef struct s{
 unsigned char Prefix;
 test_type Type;
}__attribute((__packed__)) ss;

=> sizeof(ss) = 5

foo:
        @ Function supports interworking.
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        @ link register save eliminated.
        ldr     r3, .L2
        ldr     r2, [r3]
        and     r2, r2, #255
        orr     r2, r2, r0, asl #8
        str     r2, [r3]
        ldr     r2, [r3, #4]
        bic     r2, r2, #255
        orr     r0, r2, r0, lsr #24
        str     r0, [r3, #4]
        bx      lr

accesses 8 bytes
Comment 8 Sandra Loosemore 2013-06-24 00:03:33 UTC
Thanks for giving it a try.  Do you think that in a case such as this where a single access of the appropriate size cannot be generated due to the struct having unaligned fields we should generate the same code as with -fno-strict-volatile-bitfields, or something else?  I agree the behavior of my current patch is problematical here, but we need to decide what this case is supposed to do before I can figure out how to fix the code.
Comment 9 Bernd Edlinger 2013-06-24 20:27:27 UTC
1. you should never touch memory that lies outside the struct.

2. if you have to generate multiple accesses you should generate
code as if "volatile" was not used at all.

3. if -mno-unaligned-access is given you should not use accesses
that are larger than the struct's __attribute__((alignment(x)))

4. otherwise if unaligned accesses are allowed, you may generate an
unaligned ldr/str instruction.

Note: please do not use ldmia/stmia with unaligned addresses,
because that does still segfault even in ARMv7.
(that may be handled by a Linux IRQ but not for other O/S like eCos)
Comment 10 Bernd Edlinger 2013-06-25 11:35:08 UTC
incredibly...

gcc 4.3.7 was the last version that did only write 5 bytes in foo().

starting with gcc 4.4 all variants read/write 8 bytes in foo().

that applies only to the arm code.

the x86 code does not use more than 5 bytes.
Comment 11 Sandra Loosemore 2013-10-07 15:42:21 UTC
Updated patch series:

http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02057.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02058.html
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg02059.html

Unfortunately, it seems that fixing bugs with -fstrict-volatile-bitfields has been blocked by disagreement between global reviewers and target maintainers who can't agree on whether the C/C++11 memory model should take precedence over target-specific ABIs by default.  :-(
Comment 12 Bernd Edlinger 2013-12-11 16:50:08 UTC
Author: edlinger
Date: Wed Dec 11 16:50:05 2013
New Revision: 205896

URL: http://gcc.gnu.org/viewcvs?rev=205896&root=gcc&view=rev
Log:
2013-12-11  Sandra Loosemore  <sandra@codesourcery.com>

        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997

        gcc/
        * expmed.c (strict_volatile_bitfield_p): New function.
        (store_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (store_bit_field): Handle strict volatile bitfields here instead.
        (store_fixed_bit_field): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field): Handle strict volatile bitfields here instead.
        (extract_fixed_bit_field): Don't special-case strict volatile
        bitfields here.  Simplify surrounding code to resemble that in
        store_fixed_bit_field.
        * doc/invoke.texi (Code Gen Options): Update
        -fstrict-volatile-bitfields description.

        gcc/testsuite/
        * gcc.dg/pr23623.c: New test.
        * gcc.dg/pr48784-1.c: New test.
        * gcc.dg/pr48784-2.c: New test.
        * gcc.dg/pr56341-1.c: New test.
        * gcc.dg/pr56341-2.c: New test.
        * gcc.dg/pr56997-1.c: New test.
        * gcc.dg/pr56997-2.c: New test.
        * gcc.dg/pr56997-3.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr23623.c
    trunk/gcc/testsuite/gcc.dg/pr48784-1.c
    trunk/gcc/testsuite/gcc.dg/pr48784-2.c
    trunk/gcc/testsuite/gcc.dg/pr56341-1.c
    trunk/gcc/testsuite/gcc.dg/pr56341-2.c
    trunk/gcc/testsuite/gcc.dg/pr56997-1.c
    trunk/gcc/testsuite/gcc.dg/pr56997-2.c
    trunk/gcc/testsuite/gcc.dg/pr56997-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/doc/invoke.texi
    trunk/gcc/expmed.c
    trunk/gcc/testsuite/ChangeLog
Comment 13 Bernd Edlinger 2013-12-11 16:59:27 UTC
Author: edlinger
Date: Wed Dec 11 16:59:24 2013
New Revision: 205897

URL: http://gcc.gnu.org/viewcvs?rev=205897&root=gcc&view=rev
Log:
2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>
            Sandra Loosemore  <sandra@codesourcery.com>

        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997
        * expmed.c (strict_volatile_bitfield_p): Add bitregion_start
        and bitregion_end parameters.  Test for compliance with C++
        memory model.
        (store_bit_field): Adjust call to strict_volatile_bitfield_p.
        Add fallback logic for cases where -fstrict-volatile-bitfields
        is supposed to apply, but cannot.
        (extract_bit_field): Likewise. Use narrow_bit_field_mem and
        extract_fixed_bit_field_1 to do the extraction.
        (extract_fixed_bit_field): Revert to previous mode selection algorithm.
        Call extract_fixed_bit_field_1 to do the real work.
        (extract_fixed_bit_field_1): New function.

testsuite:        
        * gcc.dg/pr23623.c: Update to test interaction with C++
        memory model.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expmed.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/pr23623.c
Comment 14 Joey Ye 2014-01-07 10:12:12 UTC
Resolved in trunk
Comment 15 jye2 2014-02-27 07:28:39 UTC
Author: jye2
Date: Thu Feb 27 07:28:06 2014
New Revision: 208195

URL: http://gcc.gnu.org/viewcvs?rev=208195&root=gcc&view=rev
Log:
2014-02-27  Joey Ye  <joey.ye@arm.com>

        Backport mainline strict-volatile-bitfields fixes
	2013-09-28  Sandra Loosemore  <sandra@codesourcery.com>

        gcc/
        * expr.h (extract_bit_field): Remove packedp parameter.
        * expmed.c (extract_fixed_bit_field): Remove packedp parameter
        from forward declaration.
        (store_split_bit_field): Remove packedp arg from calls to
        extract_fixed_bit_field.
        (extract_bit_field_1): Remove packedp parameter and packedp
        argument from recursive calls and calls to extract_fixed_bit_field.
        (extract_bit_field): Remove packedp parameter and corresponding
        arg to extract_bit_field_1.
        (extract_fixed_bit_field): Remove packedp parameter.  Remove code
        to issue warnings.
        (extract_split_bit_field): Remove packedp arg from call to
        extract_fixed_bit_field.
        * expr.c (emit_group_load_1): Adjust calls to extract_bit_field.
        (copy_blkmode_from_reg): Likewise.
        (copy_blkmode_to_reg): Likewise.
        (read_complex_part): Likewise.
        (store_field): Likewise.
        (expand_expr_real_1): Likewise.
        * calls.c (store_unaligned_arguments_into_pseudos): Adjust call
        to extract_bit_field.
        * config/tilegx/tilegx.c (tilegx_expand_unaligned_load): Adjust
        call to extract_bit_field.
        * config/tilepro/tilepro.c (tilepro_expand_unaligned_load): Adjust
        call to extract_bit_field.
        * doc/invoke.texi (Code Gen Options): Remove mention of warnings
        and special packedp behavior from -fstrict-volatile-bitfields
        documentation.

2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        * expr.c (expand_assignment): Remove dependency on 
        flag_strict_volatile_bitfields. Always set the memory
        access mode.
        (expand_expr_real_1): Likewise.

2013-12-11  Sandra Loosemore  <sandra@codesourcery.com>

        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997

        gcc/
        * expmed.c (strict_volatile_bitfield_p): New function.
        (store_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (store_bit_field): Handle strict volatile bitfields here instead.
        (store_fixed_bit_field): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field_1): Don't special-case strict volatile
        bitfields here.
        (extract_bit_field): Handle strict volatile bitfields here instead.
        (extract_fixed_bit_field): Don't special-case strict volatile
        bitfields here.  Simplify surrounding code to resemble that in
        store_fixed_bit_field.
        * doc/invoke.texi (Code Gen Options): Update
        -fstrict-volatile-bitfields description.

        gcc/testsuite/
        * gcc.dg/pr23623.c: New test.
        * gcc.dg/pr48784-1.c: New test.
        * gcc.dg/pr48784-2.c: New test.
        * gcc.dg/pr56341-1.c: New test.
        * gcc.dg/pr56341-2.c: New test.
        * gcc.dg/pr56997-1.c: New test.
        * gcc.dg/pr56997-2.c: New test.
        * gcc.dg/pr56997-3.c: New test.

2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>
             Sandra Loosemore  <sandra@codesourcery.com>
        
        PR middle-end/23623
        PR middle-end/48784
        PR middle-end/56341
        PR middle-end/56997
        * expmed.c (strict_volatile_bitfield_p): Add bitregion_start
        and bitregion_end parameters.  Test for compliance with C++
        memory model.
        (store_bit_field): Adjust call to strict_volatile_bitfield_p.
        Add fallback logic for cases where -fstrict-volatile-bitfields
        is supposed to apply, but cannot.
        (extract_bit_field): Likewise. Use narrow_bit_field_mem and
        extract_fixed_bit_field_1 to do the extraction.
        (extract_fixed_bit_field): Revert to previous mode selection
algorithm.
        Call extract_fixed_bit_field_1 to do the real work.
        (extract_fixed_bit_field_1): New function.
        
testsuite:
        * gcc.dg/pr23623.c: Update to test interaction with C++
        memory model.

2013-12-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>
             
        PR middle-end/59134
        * expmed.c (store_bit_field): Use narrow_bit_field_mem and
        store_fixed_bit_field_1 for -fstrict-volatile-bitfields.
        (store_fixed_bit_field): Split up.  Call store_fixed_bit_field_1
        to do the real work.
        (store_fixed_bit_field_1): New function. 
        (store_split_bit_field): Limit the unit size to the memory mode
size,
        to prevent recursion.
        
testsuite:
        * gcc.c-torture/compile/pr59134.c: New test.
        * gnat.dg/misaligned_volatile.adb: New test.

Added:
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.c-torture/compile/pr59134.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr23623.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr48784-1.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr48784-2.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56341-1.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56341-2.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-1.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-2.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gcc.dg/pr56997-3.c
    branches/ARM/embedded-4_8-branch/gcc/testsuite/gnat.dg/misaligned_volatile.adb
Modified:
    branches/ARM/embedded-4_8-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-4_8-branch/gcc/calls.c
    branches/ARM/embedded-4_8-branch/gcc/config/tilegx/tilegx.c
    branches/ARM/embedded-4_8-branch/gcc/config/tilepro/tilepro.c
    branches/ARM/embedded-4_8-branch/gcc/doc/invoke.texi
    branches/ARM/embedded-4_8-branch/gcc/expmed.c
    branches/ARM/embedded-4_8-branch/gcc/expr.c
    branches/ARM/embedded-4_8-branch/gcc/expr.h
    branches/ARM/embedded-4_8-branch/gcc/testsuite/ChangeLog.arm
Comment 16 Andrew Pinski 2014-08-15 04:46:29 UTC
This testcase fails on aarch64 when SLOW_UNALIGNED_ACCESS is true.
Comment 17 Bernd Edlinger 2014-08-15 16:36:39 UTC
(In reply to Andrew Pinski from comment #16)
> This testcase fails on aarch64 when SLOW_UNALIGNED_ACCESS is true.

hmm, yes.

there are targets that define SLOW_UNALIGNED_ACCESS=1, but
they also define STRICT_ALIGNMENT=1 at the same time.
probably this combination is not tested at all.

does it pass if you use -mstrict-align ?