Bug 53908 - [4.6 Regression] csa removes needed memory load
Summary: [4.6 Regression] csa removes needed memory load
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.6.4
: P1 normal
Target Milestone: 4.6.4
Assignee: Steven Bosscher
URL: http://gcc.gnu.org/ml/gcc-patches/201...
Keywords: patch, wrong-code
Depends on:
Blocks:
 
Reported: 2012-07-10 03:06 UTC by Hans-Peter Nilsson
Modified: 2012-11-08 22:23 UTC (History)
4 users (show)

See Also:
Host: x86_64-linux
Target: x86_64-linux-gnu, cris-axis-elf, crisv32-axis-linux-gnu, sparc64-linux
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-07-11 00:00:00


Attachments
Repeat with e.g. cc1 -O2 ba2.c (1.84 KB, text/plain)
2012-07-10 03:06 UTC, Hans-Peter Nilsson
Details
patch from Richard Sandiford (418 bytes, patch)
2012-07-13 06:53 UTC, Hans-Peter Nilsson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans-Peter Nilsson 2012-07-10 03:06:21 UTC
Created attachment 27768 [details]
Repeat with e.g. cc1 -O2 ba2.c

Observed with gcc-4_7-branch revision 189394.
The attached test-case calls
is_basic(user_name) < 0 && is_digest(user_name) < 0
with *user_name = NULL.
The call to is_basic "fails"; returns < 0 without writing to *user_name.
The call to is_digest is successful and furthermore writes *user_name (a char **).  Next, *user_item = find_user(*user_name) is called, but the load of *user_name after the is_digest call is lost; wrongly eliminated in favor of the load after the call to is_basic.

A dump shows wrong code first in the ".207r.csa" pass (for crisv32-axis-linux-gnu, likely for x86_64 as well).

The test-case does not expose the bug on trunk r189401.
Not observed with a gcc-4.3-based toolchain (cris-*), not observed with (host) "Debian 4.4.5-8". Unknown 4.5, 4.6.
Comment 1 Mikael Pettersson 2012-07-10 09:04:58 UTC
I can reproduce on sparc64-linux: gcc 4.4, 4.5, and 4.8 generate working code, but 4.6 and 4.7 generate code that SEGVs.  I wasn't able to reproduce on ARMv5 or m68k.
Comment 2 Mikael Pettersson 2012-07-10 10:11:15 UTC
I can't reproduce on x86_64-linux, nor with a cross to sparc64-linux (only natively on sparc64-linux so far).

HP: are you sure it fails on x86_64?

I'll try to bisect natively on sparc64, but that will take ages...
Comment 3 Hans-Peter Nilsson 2012-07-10 17:03:14 UTC
(In reply to comment #2)
> I can't reproduce on x86_64-linux, 

Odd.  What does valgrind say / did you check the generated code?

> HP: are you sure it fails on x86_64?

Yes, as written.  N.B.: I used --disable-bootstrap, but that shouldn't have any significance modulo bugs.  Host (native) compiler was the mentioned "Debian 4.4.5-8" x86_64 gcc, but not for all targets and only for pristine FSF code checks.  Are you sure you used the gcc 4.7 branch for x86_64? ;)

It can't be ruled out of course that there's some address hash thing clouding the issue, but the reproducibility seems stable (same gcc binary on different targets; different host gcc).
Comment 4 Mikael Pettersson 2012-07-10 19:55:25 UTC
My mistake, I can now reproduce the runtime SEGV on x86_64-linux with vanilla
gcc-4.7-20120707.
Comment 5 Mikael Pettersson 2012-07-10 22:50:24 UTC
On x86_64-linux the SEGV went away on trunk with r186159:
http://gcc.gnu.org/ml/gcc-cvs/2012-04/msg00108.html
http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00202.html

The patch description makes it sound more like a cleanup than fixing actual bugs, but when I diff the assembly code for the test case with r186158 and r186159 I see:

--- pr53908.s-r186158   2012-07-11 00:28:57.000000000 +0200
+++ pr53908.s-r186159   2012-07-11 00:34:32.000000000 +0200
...
        call    is_basic
        testl   %eax, %eax
-       movq    8(%rsp), %rbp
-       js      .L68
-.L29:
+       js      .L29
+.L33:
        movq    users(%rip), %rbx
+       movq    8(%rsp), %rbp
        testq   %rbx, %rbx
...

That is, a load is being moved across a control flow insn.  All other diffs seem to just be changed label numbers.

I'll give it some more testing tomorrow.
Comment 6 H.J. Lu 2012-07-11 06:27:29 UTC
It is fixed on trunk by revision 186159:

http://gcc.gnu.org/ml/gcc-cvs/2012-04/msg00108.html
Comment 7 H.J. Lu 2012-07-11 08:04:48 UTC
It is caused by revision 167779:

http://gcc.gnu.org/ml/gcc-cvs/2010-12/msg00461.html
Comment 8 Mikael Pettersson 2012-07-11 09:46:06 UTC
Backporting r186159 and its followup fix r186164 to 4.7.1 was easy and fixed the test case there too (untested beyond this test case).  Backporting those revisions to 4.6.3 required more elbow grease but didn't fix the test case there.

I'm going to test the 4.7 backport properly now, in case a smaller more direct fix doesn't emerge soon.
Comment 9 H.J. Lu 2012-07-11 12:38:16 UTC
The fix is posted at

http://gcc.gnu.org/ml/gcc-patches/2012-07/msg00410.html
Comment 10 Hans-Peter Nilsson 2012-07-13 06:53:30 UTC
Created attachment 27783 [details]
patch from Richard Sandiford

Updated patch, from http://gcc.gnu.org/ml/gcc-patches/2012-07/msg00463.html (which wasn't in patch form).
Comment 11 Hans-Peter Nilsson 2012-07-13 08:53:28 UTC
Author: hp
Date: Fri Jul 13 08:53:24 2012
New Revision: 189454

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189454
Log:
	PR rtl-optimization/53908
	* df-problems.c (can_move_insns_across): When doing
	memory-reference book-keeping, handle call insns.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/df-problems.c
Comment 12 Hans-Peter Nilsson 2012-07-13 08:58:52 UTC
Author: hp
Date: Fri Jul 13 08:58:46 2012
New Revision: 189455

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189455
Log:
	PR rtl-optimization/53908
	* gcc.dg/torture/pr53908.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr53908.c
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 13 Hans-Peter Nilsson 2012-07-13 17:21:47 UTC
Author: hp
Date: Fri Jul 13 17:21:41 2012
New Revision: 189468

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189468
Log:
	PR rtl-optimization/53908
	* df-problems.c (can_move_insns_across): When doing
	memory-reference book-keeping, handle call insns.

Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/df-problems.c
Comment 14 Hans-Peter Nilsson 2012-07-13 17:23:02 UTC
Author: hp
Date: Fri Jul 13 17:22:55 2012
New Revision: 189469

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189469
Log:
	PR rtl-optimization/53908
	* gcc.dg/torture/pr53908.c: New test.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/torture/pr53908.c
Modified:
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
Comment 15 Hans-Peter Nilsson 2012-07-13 17:27:11 UTC
Testing for 4.7 went well, as expected from the previous versions, so committed.
Closing, but feel free to post test-results for 4.6 and permission for back-port.
Comment 16 Steven Bosscher 2012-07-13 17:39:21 UTC
GCC 4.6 is still maintained, so this regression report should stay open.

In fact, being wrong code on a primary target, and relatively trivial to fix, I'd say this has to be a P1 bug for GCC 4.6.
Comment 17 Steven Bosscher 2012-07-13 17:41:12 UTC
Will take care of GCC 4.6.
Comment 18 Hans-Peter Nilsson 2012-07-13 19:10:07 UTC
(In reply to comment #17)
> Will take care of GCC 4.6.

Thanks!
Comment 19 Steven Bosscher 2012-07-16 09:36:11 UTC
Author: steven
Date: Mon Jul 16 09:36:04 2012
New Revision: 189512

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189512
Log:
Backport from trunk:

gcc/
	PR rtl-optimization/53908
	* df-problems.c (can_move_insns_across): When doing
	memory-reference book-keeping, handle call insns.

testsuite/
	PR rtl-optimization/53908
	* gcc.dg/torture/pr53908.c: New test.


Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.dg/torture/pr53908.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/df-problems.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
Comment 20 Steven Bosscher 2012-11-08 22:23:16 UTC
.