This is a bug with named address spaces trunk gcc 4.7 for the following source. struct rgb { char r, g, b; }; char read_rgb_ok (const __pgm struct rgb *s) { return s->r + s->g + s->b; } char read_rgb_bug (const __pgm struct rgb *s) { struct rgb t = *s; return t.r + t.g + t.b; } Reading through *s must happen by means of LPM instructions (reading from flash) and not by menas of LD instructions (reading from RAM). However, the second function reads from RAM. == Command Line == avr-gcc a-bug.c -S -Os -mmcu=avr4 -dp -fverbose-asm -v \ -fdump-tree-optimized -fdump-tree-original -fdump-rtl-expand -fdump-tree-gimple == configure == Generated from 4.7.0 sources (HEAD) from 2012-01-03 SVN 182900 Target: avr Configured with: ../../gcc.gnu.org/trunk/configure --target=avr --prefix=/local/gnu/install/gcc-4.7-mingw32 --host=i386-mingw32 --build=i686-linux-gnu --enable-languages=c,c++ --disable-nls --disable-shared --disable-lto --disable-checking --disable-libquadmath --with-dwarf2 Thread model: single gcc version 4.7.0 20120102 (experimental) (GCC)
Created attachment 26262 [details] a-bug.c C source file triggering the bug.
Created attachment 26263 [details] a-bug.s (assembler output) read_rgb_ok uses LPM instructions to read data (okay) read_rgb_bug uses LD/LDD instructions to read data (wrong)
Created attachment 26264 [details] a-bug.c.003t.original a-bug.c.003t.original tree-dump, FYI
Created attachment 26265 [details] a-bug.c.004t.gimple a-bug.c.004t.gimple tree-dump, FYI
Created attachment 26266 [details] a-bug.c.149t.optimized a-bug.c.149t.optimized tree dump, FYI
Created attachment 26267 [details] a-bug.c.150r.expand a-bug.c.150r.expand RTL dump, FYI As you can see in read_rgb_ok that move insns 8, 9, 13 are reading from AS1 which is __pgm. This code is correct In read_rgb_bug, insns 8, 9 and 13 are reading from generic address space brcause there is no address space information. This code is wrong.
Please figure out where the address-space information is lost.
It's scalar replacement of aggregates: With -O1 code is wrong. With -O1 -fno-tree-sra code is correct.
(In reply to comment #7) > Please figure out where the address-space information is lost. Is -f[no-]tree-sra enough information to find the bug for someone familiar with RSA? Here is an even simpler test case: struct rgb { char r; }; char read_rgb_bug (const __pgm struct rgb *s) { struct rgb t = *s; return t.r; }
Where is the address space information about a particular memory access stored in gimple/tree infrastructure? Do you happen to know if this bug started happening recently on the trunk? Thanks.
(In reply to comment #10) > Where is the address space information about a particular memory access stored > in gimple/tree infrastructure? You mean the ADDR_SPACE macros from tree.h?
Author: rguenth Date: Tue Jan 17 14:52:57 2012 New Revision: 183249 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183249 Log: 2012-01-17 Richard Guenther <rguenther@suse.de> PR middle-end/51782 * expr.c (expand_assignment): Take address-space information from the address operand of MEM_REF and TARGET_MEM_REF. (expand_expr_real_1): Likewise. Modified: trunk/gcc/ChangeLog trunk/gcc/expr.c
(In reply to comment #12) > Author: rguenth > Date: Tue Jan 17 14:52:57 2012 > New Revision: 183249 Just for feedback: In r183270 the bug is still present (and -fno-tree-sra is work around).
Created attachment 26467 [details] a-bug.c The draft address space names have been replaced by their final names, which is __flash now. The draft __pgm is no more supported. http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183529 Updated test case: ================== struct rgb { char r, g, b; }; char read_rgb_ok (const __flash struct rgb *s) { return s->r + s->g + s->b; } char read_rgb_bug (const __flash struct rgb *s) { struct rgb t = *s; return t.r + t.g + t.b; }
In addition to mistakes corrected by the patch in comment #12, build_ref_for_offset indeed also does not care about address spaces of MEM_REFs it is creating. Thus, mine. Perhaps we should even add a check to a verifier to catch situations when a MEM_REF's zero argument has a TREE_TYPE^2 with a different address space than the MEM_REF itself?
OK, I clearly misremembered where the address space information is supposed to be stored. Richi's patch in comment #12 clearly shows it's the address operand of MEM_REF should carry that information, so please disregard my previous comment. Nevertheless, during expansion of a COMPONENT_REF or any handled_component we do rely on its type's TYPE_ADDR_SPACE because expand_expr_real_1 calls set_mem_attributes with the whole reference tree to deduce the attributes from and it uses the address space of its type. If only the base object or pointer of a reference is supposed to hold the address space information, then this function needs to be changed to look at the base object. I'll attach an untested patch straight away. Alternatively, we could alter types o references built by SRA at each step (in build_ref_for_offset and build_ref_for_model) which also works (I also have a patch) but would be rather inconsistent with how we expand MEM_REFs (but front-end generated stuff looks like this).
Created attachment 26695 [details] Untested proposed fix This untested patch fixes the issue for me on a cross-compiler. It would be great if someone who is set up to run the testsuite on a platform with multiple address spaces or a simulator of one could test it a bit. My plan is to discuss this with maintainers next week and if they like the approach, give it a formal bootstrap and test run on x86_64 and submit shortly thereafter if everything goes fine.
(In reply to comment #17) > Created attachment 26695 [details] > Untested proposed fix > > This untested patch fixes the issue for me on a cross-compiler. It > would be great if someone who is set up to run the testsuite on a > platform with multiple address spaces or a simulator of one could test > it a bit. Thanks. It will take some days.
On Fri, 17 Feb 2012, jamborm at gcc dot gnu.org wrote: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51782 > > --- Comment #17 from Martin Jambor <jamborm at gcc dot gnu.org> 2012-02-17 17:59:43 UTC --- > Created attachment 26695 [details] > --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26695 > Untested proposed fix > > This untested patch fixes the issue for me on a cross-compiler. It > would be great if someone who is set up to run the testsuite on a > platform with multiple address spaces or a simulator of one could test > it a bit. > > My plan is to discuss this with maintainers next week and if they like > the approach, give it a formal bootstrap and test run on x86_64 and > submit shortly thereafter if everything goes fine. base returned from get_base_address should never be NULL, so it's safe to assume it isn't. Otherwise the patch looks ok to me. Richard.
(In reply to comment #19) > base returned from get_base_address should never be NULL, so it's > safe to assume it isn't. Otherwise the patch looks ok to me. > Unfortunately, when I was bootstrapping a modified patch without the NULL test on x86_64, it segfaulted. The reason is that expand_debug_expr calls set_mem_attributes_minus_bitpos and in t passes MEM[(struct basic_stringbuf *)&__s._M_stringbuf]._M_string which might be OK for debug statements, I don't really know. Even though I guess it might wreck havoc in address spaces in debug info, for now I'm reverting to the original patch from comment #17 for the purposes of this PR. Perhaps get_base_address misses a DECL_P in the condition looking into MEM_REFs?
On Mon, 20 Feb 2012, jamborm at gcc dot gnu.org wrote: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51782 > > --- Comment #20 from Martin Jambor <jamborm at gcc dot gnu.org> 2012-02-20 17:27:36 UTC --- > (In reply to comment #19) > > base returned from get_base_address should never be NULL, so it's > > safe to assume it isn't. Otherwise the patch looks ok to me. > > > > Unfortunately, when I was bootstrapping a modified patch without the > NULL test on x86_64, it segfaulted. The reason is that > expand_debug_expr calls set_mem_attributes_minus_bitpos and in t > passes > > MEM[(struct basic_stringbuf *)&__s._M_stringbuf]._M_string That's an invalid MEM_REF, and the result of set_mem_attributes_minus_bitpos is probably completely bogus. > which might be OK for debug statements, I don't really know. Even > though I guess it might wreck havoc in address spaces in debug info, > for now I'm reverting to the original patch from comment #17 for the > purposes of this PR. Ok, probably safer for 4.7 > Perhaps get_base_address misses a DECL_P in the condition looking into > MEM_REFs? No, sure not - in the above we have a component-ref inside the ADDR_EXPR. Richard.
btw, what's the right component for the PR? tree-optimization? middle-end?
(In reply to comment #21) > On Mon, 20 Feb 2012, jamborm at gcc dot gnu.org wrote: > > Perhaps get_base_address misses a DECL_P in the condition looking into > > MEM_REFs? > > No, sure not - in the above we have a component-ref inside the > ADDR_EXPR. I know it's invalid, I just thought it would be better to return the MEM_REF rather than NULL_TREE anyway. Most of checks whether the zero argument is ADDR_EXPR are accompanied with a DECL_P of its argument check too (e.g. the new mem_ref_refers_to_non_mem_p). However, now I see we don't do it e.g. in get_ref_base_and_extent so I agree we should not do it in get_base_address either. (In reply to comment #22) > btw, what's the right component for the PR? tree-optimization? middle-end? Yeah, I guess (and what it's worth, I'm changing the component to middle-end).
Unfortunately, with the patch I got following new LTO link failures on x86_64-linux: gcc.dg/lto/trans-mem-1 c_lto_trans-mem-1_0.o-c_lto_trans-mem-1_1.o link, -flto -fgnu-tm gcc.dg/lto/trans-mem-2 c_lto_trans-mem-2_0.o-c_lto_trans-mem-2_1.o link, -flto -fgnu-tm gcc.dg/lto/trans-mem-4 c_lto_trans-mem-4_0.o-c_lto_trans-mem-4_1.o link, -flto -fgnu-tm I'll have a look a that, even though they look rather scary.
> Unfortunately, with the patch I got following new LTO link failures on > x86_64-linux: > > gcc.dg/lto/trans-mem-1 c_lto_trans-mem-1_0.o-c_lto_trans-mem-1_1.o link, -flto > -fgnu-tm > gcc.dg/lto/trans-mem-2 c_lto_trans-mem-2_0.o-c_lto_trans-mem-2_1.o link, -flto > -fgnu-tm > gcc.dg/lto/trans-mem-4 c_lto_trans-mem-4_0.o-c_lto_trans-mem-4_1.o link, -flto > -fgnu-tm > > I'll have a look a that, even though they look rather scary. Don't you see the failures without the patch (see pr52297)?
Author: gjl Date: Tue Feb 21 11:54:27 2012 New Revision: 184434 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184434 Log: PR middle-end/51782 * gcc.target/avr/torture/pr51782-1.c: New test. Added: trunk/gcc/testsuite/gcc.target/avr/torture/pr51782-1.c Modified: trunk/gcc/testsuite/ChangeLog
(In reply to comment #25) > > Unfortunately, with the patch I got following new LTO link failures on > > x86_64-linux: > > > > gcc.dg/lto/trans-mem-1 c_lto_trans-mem-1_0.o-c_lto_trans-mem-1_1.o link, -flto > > -fgnu-tm > > gcc.dg/lto/trans-mem-2 c_lto_trans-mem-2_0.o-c_lto_trans-mem-2_1.o link, -flto > > -fgnu-tm > > gcc.dg/lto/trans-mem-4 c_lto_trans-mem-4_0.o-c_lto_trans-mem-4_1.o link, -flto > > -fgnu-tm > > > > I'll have a look a that, even though they look rather scary. > > Don't you see the failures without the patch (see pr52297)? No, I don't, for some reason. Nevertheless, it indeed seems to be an unrelated problem and so I have posted the patch to the mailing list and now only wait for test results on a platform with address spaces: http://gcc.gnu.org/ml/gcc-patches/2012-02/msg01080.html
Author: jamborm Date: Wed Feb 22 10:37:03 2012 New Revision: 184463 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184463 Log: 2012-02-22 Martin Jambor <mjambor@suse.cz> PR middle-end/51782 * emit-rtl.c (set_mem_attributes_minus_bitpos): Set address space according to the base object. Modified: trunk/gcc/ChangeLog trunk/gcc/emit-rtl.c
Patch is in, this should be finally fixed.