| Summary: | [4.6 Regression] conditional write through volatile pointer produces unintended read | ||
|---|---|---|---|
| Product: | gcc | Reporter: | werner |
| Component: | middle-end | Assignee: | Jakub Jelinek <jakub> |
| Status: | RESOLVED FIXED | ||
| Severity: | major | CC: | matz, mikpelinux |
| Priority: | P3 | Keywords: | wrong-code |
| Version: | 4.7.2 | ||
| Target Milestone: | 4.6.4 | ||
| Host: | Target: | ||
| Build: | Known to work: | ||
| Known to fail: | Last reconfirmed: | 2013-01-25 00:00:00 | |
| Attachments: | gcc48-pr56098.patch | ||
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 |
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.