If compiled with -O2 or greater and without -fno-strict-aliasing, the following code volatile int *ptr; void problem(int flag) { *ptr = 1; if (flag) *ptr = 2; } produces an unintended read, as if the "if" statement had continued with else *ptr = *ptr; Code generated by gcc 4.7.2 on Ubuntu on x86-64 when invoked with gcc-4.7 -Wall -Wextra -O2 -S bug.c ... problem: .LFB0: .cfi_startproc movq ptr(%rip), %rax testl %edi, %edi movl $2, %edx movl $1, (%rax) jne .L2 movl (%rax), %edx <<< unexpected .L2: movl %edx, (%rax) ret .cfi_endproc ... This happens with - gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3 for x86-64, - gcc-4.7 (Ubuntu/Linaro 4.7.2-2ubuntu1) 4.7.2 for x86-64, - mipsel-openwrt-linux-gcc (Linaro GCC 4.6-2012.02) 4.6.3 20120201 (prerelease) for MIPS, and I've had someone confirm it for an ARM target (without further details) as well. -Os instead of -O2 has the same effect. The unintended read disappears if optimizing with -O1 or less, or with -fno-strict-aliasing. This bears a striking resemblance to Bug 15456 and maybe Bug 5395, except that these were for C++ while this is plain C.
gcc 3.3.6 to 4.2.4 generate: problem: .LFB2: movq ptr(%rip), %rax testl %edi, %edi movl $1, (%rax) je .L4 movl $2, (%rax) .L4: rep ; ret which looks Ok to me. From 4.3.6 up to 4.7.2 we get the broken code Werner showed. 3.2.3 generates different broken code: problem: .LFB1: xorl %eax, %eax movq ptr(%rip), %rdx testl %edi, %edi setne %al movl $1, (%rdx) incl %eax movl %eax, (%rdx) ret
This is cselim at work, preparing the way for if-conversion of the store ... It shouldn't do this for volatiles of course. Workaround: -fno-tree-cselim
Created attachment 29275 [details] gcc48-pr56098.patch Untested fix.
Author: jakub Date: Fri Jan 25 20:03:54 2013 New Revision: 195475 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195475 Log: PR tree-optimization/56098 * tree-ssa-phiopt.c (nt_init_block): Don't call add_or_mark_expr for stmts with volatile ops. (cond_store_replacement): Don't optimize if assign has volatile ops. (cond_if_else_store_replacement_1): Don't optimize if either then_assign or else_assign have volatile ops. (hoist_adjacent_loads): Don't optimize if either def1 or def2 have volatile ops. * gcc.dg/pr56098-1.c: New test. * gcc.dg/pr56098-2.c: New test. Added: trunk/gcc/testsuite/gcc.dg/pr56098-1.c trunk/gcc/testsuite/gcc.dg/pr56098-2.c Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/tree-ssa-phiopt.c
Fixed on the trunk so far.
Thanks for the analysis and the fixes ! I'll try them soonish. Regarding work-arounds, the ones I mentioned for my original code snippet (i.e., -O1 or -fno-strict-aliasing) aren't sufficient in the following more general case: volatile void *p; #define P (*(volatile int *) (p+8)) void foo(int x) { P = 1; if (x) P = 2; } This is for gcc (Ubuntu/Linaro 4.7.2-2ubuntu1) 4.7.2 on x86-64. The offset (8) may be architecture-specific. For values < 8, correct code is generated with -O1 (but not -O2 or higher). The good news is that -fno-tree-cselim does indeed avoid the bad read in all cases I've tried, with any optimization level.
I'm happy to confirm that trunk (for x86-64) now passes all variations of this theme I tried with flying colors. Thanks again !
Author: jakub Date: Fri Feb 1 14:16:20 2013 New Revision: 195663 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195663 Log: Backported from mainline 2013-01-25 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/56098 * tree-ssa-phiopt.c (nt_init_block): Don't call add_or_mark_expr for stmts with volatile ops. (cond_store_replacement): Don't optimize if assign has volatile ops. (cond_if_else_store_replacement_1): Don't optimize if either then_assign or else_assign have volatile ops. * gcc.dg/pr56098-1.c: New test. Added: branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/pr56098-1.c Modified: branches/gcc-4_7-branch/gcc/ChangeLog branches/gcc-4_7-branch/gcc/testsuite/ChangeLog branches/gcc-4_7-branch/gcc/tree-ssa-phiopt.c
Fixed for 4.7.3+ too.
*** Bug 56476 has been marked as a duplicate of this bug. ***
Author: jakub Date: Wed Apr 3 18:02:57 2013 New Revision: 197449 URL: http://gcc.gnu.org/viewcvs?rev=197449&root=gcc&view=rev Log: Backported from mainline 2013-01-25 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/56098 * tree-ssa-phiopt.c (nt_init_block): Don't call add_or_mark_expr for stmts with volatile ops. (cond_store_replacement): Don't optimize if assign has volatile ops. (cond_if_else_store_replacement_1): Don't optimize if either then_assign or else_assign have volatile ops. * gcc.dg/pr56098-1.c: New test. Added: branches/gcc-4_6-branch/gcc/testsuite/gcc.dg/pr56098-1.c Modified: branches/gcc-4_6-branch/gcc/ChangeLog branches/gcc-4_6-branch/gcc/testsuite/ChangeLog branches/gcc-4_6-branch/gcc/tree-ssa-phiopt.c