Bug 43879 - -fipa-pta causes various miscompilations
Summary: -fipa-pta causes various miscompilations
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: 4.6.0
Assignee: Richard Biener
URL:
Keywords: wrong-code
Depends on: 40011
Blocks: 68331 29212
  Show dependency treegraph
 
Reported: 2010-04-24 15:54 UTC by Zdenek Sojka
Modified: 2023-04-01 00:48 UTC (History)
2 users (show)

See Also:
Host: x86_64-pc-linux-gnu
Target: x86_64-pc-linux-gnu
Build:
Known to work: 4.5.0
Known to fail: 4.6.0
Last reconfirmed: 2010-04-24 18:55:15


Attachments
first part of reduced testcase (213 bytes, text/plain)
2010-04-28 15:09 UTC, Zdenek Sojka
Details
second part of reduced testcase (122 bytes, text/plain)
2010-04-28 15:12 UTC, Zdenek Sojka
Details
reduced testcase, from lto-section-out.c (341 bytes, text/plain)
2010-05-02 13:45 UTC, Zdenek Sojka
Details
next testcase, second part will follow (77 bytes, text/plain)
2010-05-03 14:56 UTC, Zdenek Sojka
Details
next testcase (242 bytes, text/plain)
2010-05-03 15:02 UTC, Zdenek Sojka
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zdenek Sojka 2010-04-24 15:54:18 UTC
Tested revisions:
r158683 - fail
r158486 - OK

Compiler output:
$ binary-158683-lto-fortran/bin/gcc -O2 -fipa-pta gcc/testsuite/gcc.c-torture/execute/20000822-1.c && ./a.out
gcc/testsuite/gcc.c-torture/execute/20000822-1.c: In function 'main':
gcc/testsuite/gcc.c-torture/execute/20000822-1.c:25:5: warning: incompatible implicit declaration of built-in function 'abort' [enabled by default]
Aborted

Is -fipa-pta still no-op?
Comment 1 Zdenek Sojka 2010-04-24 16:17:45 UTC
It also causes:

$ binary-158683-lto-fortran/bin/gcc -O3 -fipa-pta gcc/testsuite/gcc.c-torture/execute/nestfunc-3.c && ./a.out
gcc/testsuite/gcc.c-torture/execute/nestfunc-3.c: In function 'main':
gcc/testsuite/gcc.c-torture/execute/nestfunc-3.c:44:5: warning: incompatible implicit declaration of built-in function 'abort' [enabled by default]
gcc/testsuite/gcc.c-torture/execute/nestfunc-3.c:47:3: warning: incompatible implicit declaration of built-in function 'exit' [enabled by default]
Aborted

r158486 was fine
Comment 2 Zdenek Sojka 2010-04-24 18:49:34 UTC
Bootstrap fails too with -fipa-pta, (at least) cfgrtl.o is miscompiled.
Comment 3 Richard Biener 2010-04-24 18:55:15 UTC
Well, -fipa-pta is no longer a no-op and I wouldn't call the existing (known)
bugs a regression.  I have some pending patches to fix issues.
Comment 4 Richard Biener 2010-04-24 20:04:03 UTC
(In reply to comment #2)
> Bootstrap fails too with -fipa-pta, (at least) cfgrtl.o is miscompiled.

Yep, I know.  If you can isolate the miscompile that would be great
(I am concentrating on completing IPA PTA as time permits).
Comment 5 Richard Biener 2010-04-28 11:51:50 UTC
Subject: Bug 43879

Author: rguenth
Date: Wed Apr 28 11:51:31 2010
New Revision: 158825

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=158825
Log:
2010-04-28  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/43879
	PR tree-optimization/43909
	* tree-ssa-structalias.c (struct variable_info): Add
	only_restrict_pointers flag.
	(new_var_info): Initialize it.  Increment stats.total_vars here.
	(create_function_info_for): Do not increment stats.total_vars
	here.
	(get_function_part_constraint): Fix build with C++.
	(insert_into_field_list): Remove.
	(push_fields_onto_fieldstack): Properly merge fields.
	(create_variable_info_for): Split and simplify.
	(create_variable_info_for_1): New piece.
	(intra_create_variable_infos): Properly make restrict constraints
	from parameters.

	* gcc.dg/ipa/ipa-pta-14.c: Adjust.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/ipa/ipa-pta-14.c
    trunk/gcc/tree-ssa-structalias.c

Comment 6 Richard Biener 2010-04-28 12:53:10 UTC
Real miscompiles prevailing:

                === libstdc++ tests ===


Running target unix/-fipa-pta/
FAIL: 20_util/shared_ptr/thread/default_weaktoshared.cc execution test
FAIL: 23_containers/bitset/cons/1.cc execution test
FAIL: 23_containers/bitset/operations/1.cc execution test
FAIL: 23_containers/bitset/to_ullong/1.cc execution test
WARNING: program timed out.
WARNING: program timed out.
FAIL: tr1/2_general_utilities/shared_ptr/thread/default_weaktoshared.cc executio
n test
Comment 7 Zdenek Sojka 2010-04-28 15:09:18 UTC
Created attachment 20507 [details]
first part of reduced testcase

second part to follow
Comment 8 Zdenek Sojka 2010-04-28 15:12:52 UTC
Created attachment 20508 [details]
second part of reduced testcase

These testcases demonstrate what happens in cfgrtl.c, why is cfg_layout_merge_blocks miscompiled:
((a)->il.rtl->end_)->code is expected not to change during call to try_redirect_by_replacing_jump()

Command line to demonstrate:
$ gcc -O[123s] -fipa-pta pr43879.c pr43879-2.c && ./a.out
Aborted
Comment 9 Richard Biener 2010-04-29 15:29:48 UTC
Wow, thanks for the testcase!
Comment 10 Richard Biener 2010-04-29 15:37:13 UTC
And the bug is:

struct TBL tbl = { foo };

but:

Generating constraints for global initializers

...
tbl = NONLOCAL
tbl = &NULL

ouch.  I have a patch.
Comment 11 Richard Biener 2010-04-30 08:23:00 UTC
Subject: Bug 43879

Author: rguenth
Date: Fri Apr 30 08:22:15 2010
New Revision: 158924

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=158924
Log:
2010-04-30  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/43879
	* tree-ssa-structalias.c (get_constraint_for_1): Properly
	handle non-zero initializers.

	* gcc.dg/torture/pr43879_1.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr43879_1.c
    trunk/gcc/testsuite/gcc.dg/torture/pr43879_2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-structalias.c

Comment 12 Richard Biener 2010-04-30 09:00:04 UTC
With the last patch we are down to

                === libstdc++ tests ===


Running target unix/-fipa-pta/
FAIL: 23_containers/bitset/operations/1.cc execution test
FAIL: 23_containers/bitset/to_ullong/1.cc execution test

plus some gfortran procedure pointer tests (might be invalid or require
-fwhole-file to work) and 

FAIL: gcc.c-torture/execute/frame-address.c execution,  -O2

which is an invalid testcase with -fipa-pta (it gets optimized too far,
the stack frame is elided).
Comment 13 Richard Biener 2010-04-30 10:06:01 UTC
I can now successfully bootstrap with -O2 -fipa-pta but lto1 seems to be
miscompiled (all LTO tests ICE).
Comment 14 Richard Biener 2010-04-30 13:57:27 UTC
Indeed all proc_ptr_* execution FAILs are fixed with -fwhole-program.  The
frontend does not properly unify decls for example in proc_ptr_comp_12.f90
for

  testObj%test => returnMat

vs.

  function returnMat( a, b ) result( mat )

without -fwhole-file.  But all IPA optimizations rely on that, so it's
critical that -fwhole-file is enabled.
Comment 15 Dominique d'Humieres 2010-04-30 14:51:25 UTC
> ... without -fwhole-file.  But all IPA optimizations rely on that, so it's
> critical that -fwhole-file is enabled.

-fwhole-file works only at the file level, i.e., if proc_ptr_comp_12.f90 is split as:

[macbook] f90/bug% cat aa.f90
! { dg-do run }
!
! PR 40646: [F03] array-valued procedure pointer components
!
! Original test case by Charlie Sharpsteen <chuck@sharpsteen.net>
! Modified by Janus Weil <janus@gcc.gnu.org>

module bugTestMod
  implicit none
  type:: boundTest
    procedure(returnMat), pointer, nopass:: test
  end type boundTest
contains
  function returnMat( a, b ) result( mat )
    integer:: a, b
    double precision, dimension(a,b):: mat 
    mat = 1d0
  end function returnMat
end module bugTestMod

[macbook] f90/bug% cat bb.f90
program bugTest
  use bugTestMod
  implicit none
  type( boundTest ):: testObj
  double precision, dimension(2,2):: testCatch
  testObj%test => returnMat
  testCatch = testObj%test(2,2)
  print *,testCatch
  if (sum(testCatch)/=4) call abort()
  print *,testObj%test(3,3)
  if (sum(testObj%test(3,3))/=9) call abort()
end program bugTest

! { dg-final { cleanup-modules "bugTestMod" } }

one gets

[macbook] f90/bug% gfc -O2 -fipa-pta bb.f90 aa.f90 -fwhole-file -flto
[macbook] f90/bug% a.out
   1.0000000000000000        1.0000000000000000        1.0000000000000000        1.0000000000000000     
   1.0000000000000000        1.0000000000000000        1.0000000000000000        1.0000000000000000        1.0000000000000000        1.0000000000000000        1.0000000000000000        1.0000000000000000        1.0000000000000000     
Segmentation fault
Comment 16 Richard Biener 2010-04-30 15:17:30 UTC
(In reply to comment #15)
> > ... without -fwhole-file.  But all IPA optimizations rely on that, so it's
> > critical that -fwhole-file is enabled.
> 
> -fwhole-file works only at the file level, i.e., if proc_ptr_comp_12.f90 is
> split as:
> 
> [macbook] f90/bug% cat aa.f90
> ! { dg-do run }
> !
> ! PR 40646: [F03] array-valued procedure pointer components
> !
> ! Original test case by Charlie Sharpsteen <chuck@sharpsteen.net>
> ! Modified by Janus Weil <janus@gcc.gnu.org>
> 
> module bugTestMod
>   implicit none
>   type:: boundTest
>     procedure(returnMat), pointer, nopass:: test
>   end type boundTest
> contains
>   function returnMat( a, b ) result( mat )
>     integer:: a, b
>     double precision, dimension(a,b):: mat 
>     mat = 1d0
>   end function returnMat
> end module bugTestMod
> 
> [macbook] f90/bug% cat bb.f90
> program bugTest
>   use bugTestMod

As far as I understand this use statement causes GFortran to read in the
bytecode from the .mod file and combine the files again to a single
translation unit.  So this split is in fact not a split.

Comment 17 Joost VandeVondele 2010-04-30 15:26:36 UTC
in this case the Fortran bits depend on PR40011
Comment 18 Dominique d'Humieres 2010-04-30 15:53:28 UTC
> As far as I understand this use statement causes GFortran to read in the
> bytecode from the .mod file and combine the files again to a single
> translation unit.  So this split is in fact not a split.

From my very limited understanding of the content of *.mod files, there is nothing such as a "bytecode" in them, but only the information needed for the USE statements:

[macbook] f90/bug% cat aa.mod
GFORTRAN module version '5' created from pr36761.f90 on Tue Apr 20 08:55:48 2010
MD5:8452a2eab439f2a00c76ddd985f3af67 -- If you edit this, you'll get what you deserve.

(() () () () () () () () () () () () () () () () () () () () () () () ()
() () ())

()

()

()

()

()

(2 'aa' 'aa' 'aa' 1 ((MODULE UNKNOWN-INTENT UNKNOWN-PROC UNKNOWN UNKNOWN
0 0) (UNKNOWN 0 0 0 UNKNOWN ()) 0 0 () () 0 () () () 0 0)
3 'md' 'aa' 'md' 1 ((VARIABLE UNKNOWN-INTENT UNKNOWN-PROC UNKNOWN
UNKNOWN 0 0 ALLOCATABLE DIMENSION) (REAL 8 0 0 REAL ()) 0 0 () (2 0
DEFERRED () () () ()) 0 () () () 0 0)
)

('aa' 0 2 'md' 0 3)

I don't know what information you need for -fipa-pta, but it is likely not there.
Comment 19 Richard Biener 2010-04-30 17:12:22 UTC
(In reply to comment #18)
> > As far as I understand this use statement causes GFortran to read in the
> > bytecode from the .mod file and combine the files again to a single
> > translation unit.  So this split is in fact not a split.
> 
> From my very limited understanding of the content of *.mod files, there is
> nothing such as a "bytecode" in them, but only the information needed for the
> USE statements:

You are right.  The .mod files only seem to contain declaration information.
So splitting the file should make the testcase work ...

> I don't know what information you need for -fipa-pta, but it is likely not
> there.

... I need callgraph information not misrepresented.  Indeed it's a bug
in IPA-PTA that makes the separate case fail.  I have a patch.
Comment 20 Richard Biener 2010-04-30 18:53:28 UTC
Subject: Bug 43879

Author: rguenth
Date: Fri Apr 30 18:52:44 2010
New Revision: 158945

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=158945
Log:
2010-04-30  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/43879
	* tree-ssa-structalias.c (type_could_have_pointers): Functions
	can have pointers.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa-structalias.c

Comment 21 Zdenek Sojka 2010-05-02 13:45:08 UTC
Created attachment 20534 [details]
reduced testcase, from lto-section-out.c

Miscompiled function is lto-section-out.c:lto_output_fn_decl_index()
lto_output_fn_decl_index:
	mov	rax, rdi	# decl_state, decl_state
	lea	rcx, [rsp-12]	# tmp62,
	mov	rdi, rsi	# obs, obs
	lea	rsi, [rax+48]	# tmp63,
	jmp	lto_output_decl_index	#

tmp62 is &index, but it is allocated under the stack pointer. Later, when lto_output_decl_index() executes "*this_index = index;", it overwrites registers stored on stack:
lto_output_decl_index:
... # r15 is saved:
	mov	QWORD PTR [rsp-8], r15	#,
... # saved r15 is overwritten:
	mov	DWORD PTR [r15], ebx	# *this_index_24(D), index
... # wrong r15 is reloaded:
	mov	r15, QWORD PTR [rsp+80]	#,

The testcase works only on x86_64, with command line:
$ gcc -O[2s] -fipa-pta pr43879-3.c
$ gcc -O1 -fipa-pta -foptimize-sibling-calls pr43879-3.c
$ ./a.out
Aborted
Comment 22 Zdenek Sojka 2010-05-02 13:47:44 UTC
Of course the overwritten register is r14:
	mov	QWORD PTR [rsp-16], r14	#,
	mov	DWORD PTR [r15], ebx	# *this_index_24(D), index
	mov	r14, QWORD PTR [rsp+72]	#,
Comment 23 Richard Biener 2010-05-02 18:11:08 UTC
Subject: Bug 43879

Author: rguenth
Date: Sun May  2 18:10:53 2010
New Revision: 158977

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=158977
Log:
2010-05-02  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/43879
	* tree-tailcall.c (find_tail_calls): Clobbers also prevent
	tail calls.

	* gcc.dg/torture/pr43879-3.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr43879-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-tailcall.c

Comment 24 Richard Biener 2010-05-02 18:12:21 UTC
Thanks for the testcase!  The issue is now fixed and bootstrap with IPA-PTA
enabled succeeds and the testresults are the same as without (with -fipa-pta
enabled).

There are still the same libstdc++ miscompiles and the known missed optimizations.
Comment 25 Zdenek Sojka 2010-05-03 14:56:22 UTC
Created attachment 20546 [details]
next testcase, second part will follow
Comment 26 Zdenek Sojka 2010-05-03 15:02:07 UTC
Created attachment 20547 [details]
next testcase

Reduced from libstdc++-v3/testsuite/23_containers/bitset/operations/1.cc

I am happy those testcases helped with gcc development, and I hope this one will too.

Command line:
$ g++ -O[123] -fipa-pta pr43879-4_1.C pr43879-4_2.C && ./a.out
Aborted

For some reason, b1.i is expected to be zero:
(diff from asm output for a bit different testcase)
101c98
<       cmp     QWORD PTR [rsp+48], rbx # b2.i, b1$i
---
>       cmp     QWORD PTR [rsp+48], 0   # b2.i,
Comment 27 Steven Bosscher 2010-05-03 16:56:13 UTC
Zdenek, has anyone told you how amazing your contribution is here? Wow!
Comment 28 Zdenek Sojka 2010-05-03 23:45:56 UTC
Thank you, Steven and Richard! It's nice to see the testcases are useful and appreciated.
Comment 29 Richard Biener 2010-05-04 11:16:25 UTC
Indeed.  Many thanks for these testcases!  I have a fix in testing.
Comment 30 Richard Biener 2010-05-04 13:12:41 UTC
Subject: Bug 43879

Author: rguenth
Date: Tue May  4 13:12:02 2010
New Revision: 159026

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159026
Log:
2010-05-04  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/43879
	* tree-ssa-structalias.c (alias_get_name): Use
	DECL_ASSEMBLER_NAME if available.
	(create_function_info_for): Return the varinfo node.
	(ipa_pta_execute): Associate same-body aliases and extra names
	with their origin nodes varinfo.  Dump DECL_ASSEMBLER_NAME.

	* g++.dg/torture/pr43879-1_0.C: New testcase.
	* g++.dg/torture/pr43879-1_1.C: Likewise.

Added:
    trunk/gcc/testsuite/g++.dg/torture/pr43879-1_0.C
    trunk/gcc/testsuite/g++.dg/torture/pr43879-1_1.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-structalias.c

Comment 31 Richard Biener 2010-05-04 14:02:36 UTC
Fixed!