[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #18 from Bernd Edlinger --- (In reply to Bernd Edlinger from comment #17) > (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 ? BTW. SLOW_UNALIGNED_ACCESS is no longer used in the strict volatile bitfields code path after r221222.
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #17 from Bernd Edlinger --- (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 ?
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #16 from Andrew Pinski --- This testcase fails on aarch64 when SLOW_UNALIGNED_ACCESS is true.
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #15 from jye2 at gcc dot gnu.org --- 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 Backport mainline strict-volatile-bitfields fixes 2013-09-28 Sandra Loosemore 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 * 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 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 Sandra Loosemore 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 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-br
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 Joey Ye changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #14 from Joey Ye --- Resolved in trunk
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #13 from edlinger at gcc dot gnu.org --- 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 Sandra Loosemore 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
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #12 from edlinger at gcc dot gnu.org --- 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 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
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #11 from Sandra Loosemore --- 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. :-(
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 Ramana Radhakrishnan changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2013-08-28 CC||ramana at gcc dot gnu.org Ever confirmed|0 |1 Known to fail||4.6.0, 4.7.0, 4.8.0
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #10 from Bernd Edlinger --- 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.
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #9 from Bernd Edlinger --- 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)
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #8 from Sandra Loosemore --- 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.
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997
--- Comment #7 from Bernd Edlinger ---
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
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997
Bernd Edlinger changed:
What|Removed |Added
CC||bernd.edlinger at hotmail dot
de
--- Comment #6 from Bernd Edlinger ---
(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 = 0xAAA;
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.
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #5 from Sandra Loosemore --- Patch posted here: http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00750.html
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 Sandra Loosemore changed: What|Removed |Added CC||sandra at codesourcery dot com --- Comment #4 from Sandra Loosemore --- I'm working on a fix for this.
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #3 from 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.
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 Richard Biener changed: What|Removed |Added Keywords||wrong-code --- Comment #2 from 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)?
[Bug target/56997] Incorrect write to packed field when strict-volatile-bitfields enabled on aarch32
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56997 --- Comment #1 from 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.
