| Message ID | alpine.LSU.2.20.1704271129590.17885@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
On Thu, 27 Apr 2017, Richard Biener wrote: > struct q { int n; long o[100]; }; > struct r { int n; long o[0]; }; > > union { > struct r r; > struct q q; > } u; > > int foo (int i, int j) > { > long *q = u.r.o; > u.r.o[i/j] = 1; > return q[2]; > } > > but nothing convinced scheduling to move the load before the store ;) > The two memory references are seen as not aliasing though. Stupid > scheduler. On x86 there's an anti-dependence on %eax that prevents the second scheduler from performing the breaking motion; with -fschedule-insns, pre-RA scheduler is actually able to move the load too early, but then IRA immediately undoes that. Also, -fselective-scheduling2 is able to move the load early via renaming (and uncovers the miscompile, as nothing undoes it later). Applying division to the result of the load, rather than the address of the store, also produces code demonstrating the miscompile: struct q { int n; long o[100]; }; struct r { int n; long o[0]; }; union { struct r r; struct q q; } u; int foo (int i, int j) { long *q = u.r.o; u.r.o[i] = 1; return q[2]/j; } foo: .LFB0: .cfi_startproc movq u+24(%rip), %rax movslq %edi, %rdi movslq %esi, %rsi movq $1, u+8(,%rdi,8) cqto idivq %rsi ret Alexander
Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c (revision 247277) +++ gcc/emit-rtl.c (working copy) @@ -1954,7 +1954,10 @@ set_mem_attributes_minus_bitpos (rtx ref while (TREE_CODE (t2) == ARRAY_REF); if (DECL_P (t2) - || TREE_CODE (t2) == COMPONENT_REF) + || (TREE_CODE (t2) == COMPONENT_REF + /* For trailing arrays t2 doesn't have a size that + covers all valid accesses. */ + && ! array_at_struct_end_p (t, false))) { attrs.expr = t2; attrs.offset_known_p = false;
When set_mem_attributes_minus_bitpos computes the MEM_EXPR for MEM RTXen it strips outermost ARRAY_REFs (for historic efficiency reasons). Nowadays a MEM_EXPR has to be conservative with respect to alias analysis which means it has to cover the original access. This means we may _not_ end up with a MEM_EXPR accessing a trailing array as its size is not conservative when used in overlap analysis. The following patch fixes that (by dropping the MEM_EXPR). This fix should be suitable for release branches and hopefully not pessimize things too much. In the end my idea always was to use the full memory reference tree and not stripping anything but I never came to implementing this. Bootstrap and regtest running on x86_64-unknown-linux-gnu. No testcase, I failed to create one on x86_64 and the ppc one in the PR is not an execute testcase so quite useless. I tried struct q { int n; long o[100]; }; struct r { int n; long o[0]; }; union { struct r r; struct q q; } u; int foo (int i, int j) { long *q = u.r.o; u.r.o[i/j] = 1; return q[2]; } but nothing convinced scheduling to move the load before the store ;) The two memory references are seen as not aliasing though. Stupid scheduler. Richard. 2017-04-27 Richard Biener <rguenther@suse.de> PR middle-end/80533 * emit-rtl.c (set_mem_attributes_minus_bitpos): When stripping ARRAY_REFs from MEM_EXPR make sure we're not keeping a reference to a trailing array.