Bug 38151 - structures with _Complex arguments are not passed correctly
Summary: structures with _Complex arguments are not passed correctly
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: 4.4.0
Assignee: Richard Biener
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: ABI, alias, wrong-code
Depends on: 38236
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-16 02:55 UTC by Jack Howarth
Modified: 2008-11-25 10:34 UTC (History)
6 users (show)

See Also:
Host:
Target: x86_64
Build: i686-apple-darwin9
Known to work:
Known to fail: 4.3.0 4.4.0
Last reconfirmed: 2008-11-22 20:41:24


Attachments
preprocessed source for t028_main.i (9.37 KB, text/plain)
2008-11-16 02:57 UTC, Jack Howarth
Details
assembly file for t028_main.s at -m64 on i686-apple-darwin9 (451 bytes, text/plain)
2008-11-16 02:58 UTC, Jack Howarth
Details
preprocessed source for t028_x.i (9.87 KB, text/plain)
2008-11-16 02:59 UTC, Jack Howarth
Details
assembly file for t028_x.s at -m64 on i686-apple-darwin9 (2.06 KB, text/plain)
2008-11-16 03:00 UTC, Jack Howarth
Details
preprocessed source for t028_y.i (10.02 KB, text/plain)
2008-11-16 03:00 UTC, Jack Howarth
Details
assembly file for t028_y.s at -m64 on i686-apple-darwin9 (2.05 KB, text/plain)
2008-11-16 03:01 UTC, Jack Howarth
Details
Da patch. (328 bytes, patch)
2008-11-19 19:32 UTC, Uroš Bizjak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jack Howarth 2008-11-16 02:55:55 UTC
The gcc.dg-struct-layout-1/t028 test fails at -m64 on i686-apple-darwin9. The 
failure is due to line...

TCI(2848,unsigned int a;_Complex int atal1 b;struct{}atpaal c[1];,F(2848,a,4027477739U,855699588U)F(2848,b,CINT(723419448,-218144346),CINT(-1673573213,131753020)))

in t028_test.h. Deleting this line allows the remaining t028 testcases to pass the execution test.
Comment 1 Jack Howarth 2008-11-16 02:57:34 UTC
Created attachment 16694 [details]
preprocessed source for t028_main.i
Comment 2 Jack Howarth 2008-11-16 02:58:44 UTC
Created attachment 16695 [details]
assembly file for t028_main.s at -m64 on i686-apple-darwin9
Comment 3 Jack Howarth 2008-11-16 02:59:27 UTC
Created attachment 16696 [details]
preprocessed source for t028_x.i
Comment 4 Jack Howarth 2008-11-16 03:00:11 UTC
Created attachment 16697 [details]
assembly file for t028_x.s at -m64 on i686-apple-darwin9
Comment 5 Jack Howarth 2008-11-16 03:00:58 UTC
Created attachment 16698 [details]
preprocessed source for t028_y.i
Comment 6 Jack Howarth 2008-11-16 03:01:33 UTC
Created attachment 16699 [details]
assembly file for t028_y.s at -m64 on i686-apple-darwin9
Comment 7 Jack Howarth 2008-11-16 03:03:39 UTC
Attached preprocessed source files and assembly files created with...

/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/xgcc -B/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/  -I/sw/src/fink.build/gcc44-4.3.999-20081115/gcc-4.4-20081115/gcc/testsuite/gcc.dg/compat -mno-mmx -fno-common -DSKIP_DECIMAL_FLOAT -DSKIP_DECIMAL_FLOAT -c  -m64 --save-temps -o c_compat_main_tst.o /sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/testsuite/gcc/gcc.dg-struct-layout-1/t028_main.c
/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/xgcc -B/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/  -w -I/sw/src/fink.build/gcc44-4.3.999-20081115/gcc-4.4-20081115/gcc/testsuite/gcc.dg/compat -mno-mmx -fno-common -DSKIP_DECIMAL_FLOAT -DSKIP_DECIMAL_FLOAT -c  -m64 --save-temps -o c_compat_x_tst.o /sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/testsu
ite/gcc/gcc.dg-struct-layout-1//t028_x.c
/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/xgcc -B/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/  -w -I/sw/src/fink.build/gcc44-4.3.999-20081115/gcc-4.4-20081115/gcc/testsuite/gcc.dg/compat -mno-mmx -fno-common -DSKIP_DECIMAL_FLOAT -DSKIP_DECIMAL_FLOAT -c  -m64 --save-temps -o c_compat_y_tst.o /sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/testsuite/gcc/gcc.dg-struct-layout-1//t028_y.c
/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/xgcc -B/sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/ c_compat_main_tst.o c_compat_x_tst.o c_compat_y_tst.o  -I/sw/src/fink.build/gcc44-4.3.999-20081115/gcc-4.4-20081115/gcc/testsuite/gcc.dg/compat -mno-mmx -fno-common   -lm   -m64 -o tmpdir-gcc-dg-struct-layout-1-t028-01.exe

Comment 8 Jack Howarth 2008-11-16 15:55:52 UTC
This failing testcase produces the following in gdb...

 gdb ./tmpdir-gcc-dg-struct-layout-1-t028-01.exe
GNU gdb 6.3.50-20050815 (Apple version gdb-962) (Sat Jul 26 08:14:40 UTC 2008)
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i386-apple-darwin"...Reading symbols for shared libraries ... done

(gdb) r
Starting program: /sw/src/fink.build/gcc44-4.3.999-20081115/darwin_objdir/gcc/testsuite/gcc/tmpdir-gcc-dg-struct-layout-1-t028-01.exe 
warning: posix_spawn failed, trying execvp, error: 86
Reading symbols for shared libraries ++. done

Program received signal SIGABRT, Aborted.
0x00007fff83829ee6 in __kill ()
(gdb) bt
#0  0x00007fff83829ee6 in __kill ()
#1  0x00007fff8389af4d in abort ()
#2  0x0000000100000f6f in main ()
(gdb) 

Is there anything else I can provide to debug this?
Comment 9 Uroš Bizjak 2008-11-16 20:28:22 UTC
(In reply to comment #8)
> This failing testcase produces the following in gdb...

> #0  0x00007fff83829ee6 in __kill ()
> #1  0x00007fff8389af4d in abort ()
> #2  0x0000000100000f6f in main ()
> (gdb) 
> 
> Is there anything else I can provide to debug this?

Yes: compile all three testfiles with -g flag added, put a watchpoint on "fails" variable (watch fails) and see where it triggers.

It looks to me that something is wrong with s2848 argument that is passed to check2848 function. At calling site (in test2848 function), we have:

(gdb) print s2848
$10 = {a = 4027477739, b = -936922831153888968, c = 0x601720}

and in check2848 we receive:

(gdb) print arg0
$11 = {a = 4195077, b = 3221252723567493120, c = 0x7fffde0e11a0}

The structure is defined as:

     struct S2848
     {
       unsigned int a;
       _Complex int __attribute__ ((aligned (1))) b;
       struct
       {
       } __attribute__ ((packed, aligned)) c[1];
     };

Hm, looks like a real bug to me.

This also fails on linux, I'll try to create a testcase.


Comment 10 Uroš Bizjak 2008-11-16 20:58:09 UTC
Real bug with argument passing. Confirmed on x86_64-linux-gnu with the testcase:

--cut here--
struct S2848
{
  unsigned int a;
  _Complex int b;
};

struct S2848 s2848;

void __attribute__((noinline))
check2848 (struct S2848 arg0)
{
  if (arg0.b != s2848.b)
    abort ();
}

int main()
{
  s2848.a = 4027477739U;
  s2848.b = (723419448 + -218144346 * __extension__ 1i);

  check2848 (s2848);

  return 0;
}
--cut here--

> gcc -O2 t.c
> ./a.out
Aborted
Comment 11 Uroš Bizjak 2008-11-16 21:09:14 UTC
Also fails in return from function:

--cut here--
struct S2848
{
  unsigned int a;
  _Complex int b;
};

struct S2848 s2848;

struct S2848 __attribute__((noinline))
check2848 (struct S2848 arg0)
{
  s2848.a = 4027477739U;
  s2848.b = (723419448 + -218144346 * __extension__ 1i);

  return s2848;
}

int main()
{
  struct S2848 ret;

  ret = check2848 (s2848);

  if (ret.b != s2848.b)
    abort ();
  return 0;
}
--cut here--
Comment 12 Uroš Bizjak 2008-11-19 19:32:28 UTC
Created attachment 16723 [details]
Da patch.

Jack, can you try attached patch?
Comment 13 Uroš Bizjak 2008-11-19 21:17:02 UTC
(In reply to comment #12)
> Created an attachment (id=16723) [edit]
> Da patch.
> 
> Jack, can you try attached patch?

Patch at http://gcc.gnu.org/ml/gcc-patches/2008-11/msg01010.html

It looks we still fail va_arg cases. But number of failures with the original testcase drops from 10 to 4.

Comment 14 Jack Howarth 2008-11-20 01:10:28 UTC
The patch from Comment 13 when applied to gcc trunk (without a complete bootstrap) eliminates the failures in gcc.dg-struct-layout-1 at -m64 while not introducing any at -m32 on i686-apple-darwin9. I am doing a complete bootstrap and make check against current gcc trunk.
Comment 15 Jack Howarth 2008-11-20 12:55:39 UTC
The patch in comment 13 appears to be sufficient to completely fix the problem on i686-apple-darwin9.
The results for current gcc trunk with the patch...

http://gcc.gnu.org/ml/gcc-testresults/2008-11/msg01712.html

...compared to those without the patch...

http://gcc.gnu.org/ml/gcc-testresults/2008-11/msg01535.html

shows that...

FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o-c_compat_y_tst.o execute 

is eliminated at -m64 without any regressions in other tests.
Comment 16 Uroš Bizjak 2008-11-20 19:50:10 UTC
Problems from Comment #10 and Comment #11 are fixed by the patch from Comment #13, but following test still fails, even with a patched compiler:

--cut here--
void abort (void);

struct S2848
{
  unsigned int a;
  _Complex int b;
  struct
  {
  } __attribute__ ((aligned)) c;
};

struct S2848 s2848;

int fails;

void  __attribute__((noinline))
check2848va (int z, ...)
{
  struct S2848 arg;
  __builtin_va_list ap;

  __builtin_va_start (ap, z);

  arg = __builtin_va_arg (ap, struct S2848);

  if (s2848.a != arg.a)
    ++fails;
  if (s2848.b != arg.b)
    ++fails;

  __builtin_va_end (ap);
}

int main (void)
{
  s2848.a = 4027477739U;
  s2848.b = (723419448 + -218144346 * __extension__ 1i);

  check2848va (1, s2848);

  if (fails)
    abort ();

  return 0;
}
--cut here--

> gcc -O2 t1.c

> ./a.out
Aborted

> gdb ./a.out
[...]
(gdb) watch fails
Hardware watchpoint 1: fails
(gdb) run
Starting program: /home/uros/test/compat/a.out 
Hardware watchpoint 1: fails

Old value = 0
New value = 1
0x000000000040051d in check2848va (z=1) at t1.c:29
29	    ++fails;
Missing separate debuginfos, use: debuginfo-install glibc.x86_64
(gdb) p /x arg.b
$2 = 0x004005802b1e8138
(gdb) p /x s2848.b
$3 = 0xf2ff61a62b1e8138
Comment 17 uros 2008-11-20 21:12:46 UTC
Subject: Bug 38151

Author: uros
Date: Thu Nov 20 21:11:22 2008
New Revision: 142059

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142059
Log:
	PR target/38151
	* config/i386/i386.c (classify_argument) [integer mode size <= 64bit]:
	Handle cases when integer argument crosses argument register boundary.

testsuite/ChangeLog:

	PR target/38151
	* gcc.target/i386/pr38151-1.c: New test.


Added:
    trunk/gcc/testsuite/gcc.target/i386/pr38151-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog

Comment 18 Uroš Bizjak 2008-11-20 21:13:59 UTC
va_arg problem from Comment #16 remains unfixed.
Comment 19 Uroš Bizjak 2008-11-20 21:37:02 UTC
Hm, rdx gets corrupted:

check2848va:
.LFB0:
	.cfi_startproc
	movq	%rsi, %rcx	# tmp73,
	leaq	8(%rsp), %rax	#,
(+)	movq	%rdx, -40(%rsp)	#,
	shrq	$32, %rcx	#,
	cmpl	%esi, s2848(%rip)	# tmp73, s2848.a
>>>	movq	-80(%rsp), %rdx	#, tmp74
	movq	%rax, -112(%rsp)	#, <variable>.overflow_arg_area
	leaq	-56(%rsp), %rax	#,
	movq	%rsi, -48(%rsp)	#,
	movl	$8, -120(%rsp)	#, <variable>.gp_offset
	movq	%rsi, -88(%rsp)	# tmp70,
	movq	%rax, -104(%rsp)	#, <variable>.reg_save_area
	movq	%rsi, -72(%rsp)	# tmp73, arg
	movq	%rdx, -64(%rsp)	# tmp74, arg
	je	.L4	#,
	addl	$1, fails(%rip)	#, fails
.L4:
	cmpl	%ecx, s2848+4(%rip)	# arg$b$real, s2848.b
	setne	%cl	#, tmp79
(++)	cmpl	%edx, s2848+8(%rip)	# arg$b$imag, s2848.b
	setne	%al	#, tmp82
	orb	%al, %cl	# tmp82, tmp79
	je	.L6	#,
	addl	$1, fails(%rip)	#, fails

rdx is saved at the point of (+), corrupted at ">>>" and this corrupted value is used at (++). The insn at ">>>" just falls in the insn stream from the sky, it is not present if "a" is changed to "unsigned long" in S2848 structure.

In case when "a" is changed to "unsigned long", testcase works OK. The testcase also works when insn at ">>>" is removed.
Comment 20 Jack Howarth 2008-11-21 00:05:01 UTC
The test case in comment 16 passes on i686-apple-darwin9 when compiled with -m32 but fails when compiled with -m64.
Comment 21 Uroš Bizjak 2008-11-22 12:33:04 UTC
This is a trace what happens in the testcase, from .expand dump:

(2) [frame + 8 ]	<- si
(3) [frame + 16]	<- dx
(4) r62			<- di

(8) r63			<- virtual-incoming-args + 0
(9) r64			<- virtual-stack-vars - 64
(10) [r64]		<- 8				;; gp_offset
(11) r65		<- virtual-stack-vars - 64
(12) [r65 + 8 ]		<- virtual-incoming-args	;; overflow_arg_area
(13) r66		<- virtual-stack-vars - 64
(14) [r66 + 16]		<- frame			;; reg_save_area
(15) r61		<- [virtual-stack-vars - 64]	;; gp_offset
if (r61 > 39)
   goto label 27

(19) r67		<- virtual-stack-vars - 32
(20) r68		<- zext (r61)
(21) r69		<- [virtual-stack-vars - 48]	;; reg_save_area
(22) r70		<- [r69 + r68]
(23) [r67]		<- r70
(24) r58		<- virtual-stack-vars - 32
goto label 32

label 27:
(29) r72		<- [virtual-stack-vars - 56]	;; overflow_arg_area
(30) r71		<- r72 + 15
(31) r58		<- r71 & -16

label 32:
(34) r73		<- [r58]
(35) [virtual-stack-vars - 16] <- r73
(36) r74		<- [r58 + 8]
(37) [virtual-stack-vars - 8 ] <- r74
(38) r60		<- [virual-stack-vars - 12]	;; arg$b$real
(39) r59		<- [virual-stack-vars - 8 ]	;; arg$b$imag

So, around insn (22), gcc forgets to copy dx register to reg_save_area. r74 is then read from uninitialized reg_save_area slot.

I'm looking at va-arg handling implementation in i386.c. But I'm not familiar with this code, so a bit of help would be most welcome here.
Comment 22 Uroš Bizjak 2008-11-22 17:07:29 UTC
Aliasing problems, gcc shoots himself in the foot...

When container consists of registers in different modes (due to X86_64_INTEGERSI_CLASS optimization):

(parallel:BLK [
        (expr_list:REG_DEP_TRUE (reg:DI 0 ax)
            (const_int 0 [0x0]))
        (expr_list:REG_DEP_TRUE (reg:SI 1 dx)
            (const_int 8 [0x8]))
    ])

then ix86_gimplify_va_arg happily creates code with invalid aliasing (from _.gimple dump):

  addr.0 = &va_arg_tmp.3;
> addr.4 = (long unsigned int *) addr.0;
  int_addr.5 = (long unsigned int *) int_addr.1;
  D.1615 = *int_addr.5;
  *addr.4 = D.1615;
> addr.6 = (unsigned int *) addr.0;
  D.1617 = addr.6 + 8;

We access addr.0 as (long unsigned int *) and as (unsigned int *. Following optimization passes are more than happy to remove the second access.

So to prove my point, testcase compiled with -fno-strict-aliasing works OK.

These problems also apply to FPmode parameters passing through SSE regs, so following patch moves registers from register area to tmp area in generic mode that fits argument passing registers best: DImode for integer and V4SFmode for SSE registers. This avoids aliasing problems.

Index: i386.c
===================================================================
--- i386.c	(revision 142120)
+++ i386.c	(working copy)
@@ -6750,7 +6750,8 @@ ix86_gimplify_va_arg (tree valist, tree 
 	    {
 	      rtx slot = XVECEXP (container, 0, i);
 	      rtx reg = XEXP (slot, 0);
-	      enum machine_mode mode = GET_MODE (reg);
+	      enum machine_mode mode
+		= SSE_REGNO_P (REGNO (reg)) ? V4SFmode : DImode;
 	      tree piece_type = lang_hooks.types.type_for_mode (mode, 1);
 	      tree addr_type = build_pointer_type (piece_type);
 	      tree src_addr, src;
Comment 23 Jack Howarth 2008-11-22 17:39:40 UTC
Patch in Comment 22 eliminates the va-arg test case failure at -m64 on i686-apple-darwin9.
Comment 24 Richard Biener 2008-11-22 18:20:18 UTC
The va-arg code should probably use ref-all pointers all over the place instead.
Comment 25 Uroš Bizjak 2008-11-22 19:30:49 UTC
Deassigning me, this is tree stuff.
Comment 26 Richard Biener 2008-11-22 20:41:04 UTC
Even with ref-all pointers we end up with

<bb 5>:
  # addr.0{0}_1 = PHI <&va_arg_tmp.3(3), addr.0{0}_22(4)>
  # ap_38 = PHI <ap_56(3), ap_57(4)>
  # va_arg_tmp.3_39 = PHI <va_arg_tmp.3_55(3), va_arg_tmp.3_50(4)>
  addr.8_25 = (struct S2848 * {ref-all}) addr.0{0}_1;
  # VUSE <fails_48, ap_38, SMT.26_53>
  # arg_59 = VDEF <arg_58(D)>
  arg = *addr.8_25;

where the last stmt misses a VUSE of va_arg_tmp.3.  This looks like a bug
in alias computation.

I have a fix for that parts.  The gimplify_va_arg still needs to use ref-all
pointers properly.
Comment 27 Richard Biener 2008-11-22 20:41:24 UTC
I have patches.
Comment 28 Richard Biener 2008-11-25 10:34:50 UTC
Fixed.
Comment 29 Richard Biener 2008-11-25 10:35:39 UTC
Subject: Bug 38151

Author: rguenth
Date: Tue Nov 25 10:34:11 2008
New Revision: 142189

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142189
Log:
2008-11-25  Richard Guenther  <rguenther@suse.de>

	PR middle-end/38151
	PR middle-end/38236
	* tree-ssa-alias.c (struct alias_info): Remove written_vars.
	Remove dereferenced_ptrs_store and dereferenced_ptrs_load
	in favor of dereferenced_ptrs.
	(init_alias_info): Adjust.
	(delete_alias_info): Likewise.
	(compute_flow_insensitive_aliasing): Properly
	include all aliased variables.
	(update_alias_info_1): Use dereferenced_ptrs.
	(setup_pointers_and_addressables): Likewise.
	(get_smt_for): Honor ref-all pointers and pointers with known alias
	set properly.
	* config/i386/i386.c (ix86_gimplify_va_arg): Use ref-all pointers.

	* gcc.c-torture/execute/pr38151.c: New testcase.
	* gcc.c-torture/execute/pr38236.c: Likewise.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr38151.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr38236.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-alias.c