PATCH: PR target/34001: Incorrect x86 fastcall behavior

PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

From

http://msdn2.microsoft.com/en-us/library/6xa169sk(VS.80).aspx

"The first two DWORD or smaller arguments are passed in ECX and EDX
registers; all other arguments are passed right to left."

But it isn't clear if it applies to structure/union. We tested all MS
compilers we have and verified that the above doesn't apply to
structure/union.  To make fastcall compatible with MS compilers, we
should only put scalar arguments in ECX and EDX.



H.J.
---
2007-11-09  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/34001
	* config/i386/i386.c (function_arg_32): Don't pass aggregate
	arguments in ECX/EDX for fastcall.

--- gcc/config/i386/i386.c.fastcall	2007-11-06 19:52:12.000000000 -0800
+++ gcc/config/i386/i386.c	2007-11-08 11:48:38.000000000 -0800
@@ -4233,8 +4233,10 @@ function_arg_32 (CUMULATIVE_ARGS *cum, e
 	  int regno = cum->regno;
 
 	  /* Fastcall allocates the first two DWORD (SImode) or
-	     smaller arguments to ECX and EDX.  */
-	  if (cum->fastcall)
+	     smaller arguments to ECX and EDX if it isn't an
+	     aggregate type .  */
+	  if (cum->fastcall
+	      && (!type || !AGGREGATE_TYPE_P (type)))
 	    {
 	      if (mode == BLKmode || mode == DImode)
 	        break;

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Richard Guenther]

On Nov 9, 2007 6:00 PM, H.J. Lu <hjl@lucon.org> wrote:
> From
>
> http://msdn2.microsoft.com/en-us/library/6xa169sk(VS.80).aspx
>
> "The first two DWORD or smaller arguments are passed in ECX and EDX
> registers; all other arguments are passed right to left."
>
> But it isn't clear if it applies to structure/union. We tested all MS
> compilers we have and verified that the above doesn't apply to
> structure/union.  To make fastcall compatible with MS compilers, we
> should only put scalar arguments in ECX and EDX.

This would be an ABI change from previous releases.  Regardless of
whether we want to do this, an entry for the 4.3 changes document
should be added.

Richard.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Fri, Nov 09, 2007 at 06:06:17PM +0100, Richard Guenther wrote:
> On Nov 9, 2007 6:00 PM, H.J. Lu <hjl@lucon.org> wrote:
> > From
> >
> > http://msdn2.microsoft.com/en-us/library/6xa169sk(VS.80).aspx
> >
> > "The first two DWORD or smaller arguments are passed in ECX and EDX
> > registers; all other arguments are passed right to left."
> >
> > But it isn't clear if it applies to structure/union. We tested all MS
> > compilers we have and verified that the above doesn't apply to
> > structure/union.  To make fastcall compatible with MS compilers, we
> > should only put scalar arguments in ECX and EDX.
> 
> This would be an ABI change from previous releases.  Regardless of
> whether we want to do this, an entry for the 4.3 changes document
> should be added.
> 

This is for gcc-4.3/changes.html.


H.J.
----
Index: gcc-4.3/changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.3/changes.html,v
retrieving revision 1.82
diff -r1.82 changes.html
40a41,43
>     <li><code>Fastcall</code> for i386 has been changed not to pass
>     aggregate arguments in register, following Microsoft compilers.</li>
> 

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Gerald Pfeifer]

On Fri, 9 Nov 2007, H.J. Lu wrote:
>> This would be an ABI change from previous releases.  Regardless of
>> whether we want to do this, an entry for the 4.3 changes document
>> should be added.
> This is for gcc-4.3/changes.html.

The documentation change looks good, thanks.  You may want to write
"registers" instead of "registers", I believe.

Gerald

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Sat, Nov 17, 2007 at 10:30:09PM +0100, Gerald Pfeifer wrote:
> On Fri, 9 Nov 2007, H.J. Lu wrote:
> >> This would be an ABI change from previous releases.  Regardless of
> >> whether we want to do this, an entry for the 4.3 changes document
> >> should be added.
> > This is for gcc-4.3/changes.html.
> 
> The documentation change looks good, thanks.  You may want to write
> "registers" instead of "registers", I believe.
> 

Like this?

H.J.
----
Index: changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.3/changes.html,v
retrieving revision 1.82
diff -u -p -r1.82 changes.html
--- changes.html	2 Nov 2007 07:27:23 -0000	1.82
+++ changes.html	18 Nov 2007 07:57:47 -0000
@@ -38,6 +38,9 @@
     <li>The i386 <code>-msvr3-shlib</code> option has been removed
     since it is no longer used.</li>
 
+    <li><code>Fastcall</code> for i386 has been changed not to pass
+    aggregate arguments in registers, following Microsoft compilers.</li>
+
     <li>Support for the AOF assembler has been removed from the ARM
     back end; this affects only the targets <code>arm-semi-aof</code>
     and <code>armel-semi-aof</code>, which are no longer recognized.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Gerald Pfeifer]

On Sat, 17 Nov 2007, H.J. Lu wrote:
> Like this?

Yup.

Gerald

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Mark Mitchell]

Richard Guenther wrote:

>> But it isn't clear if it applies to structure/union. We tested all MS
>> compilers we have and verified that the above doesn't apply to
>> structure/union.  To make fastcall compatible with MS compilers, we
>> should only put scalar arguments in ECX and EDX.
> 
> This would be an ABI change from previous releases.  Regardless of
> whether we want to do this, an entry for the 4.3 changes document
> should be added.

I would suggest that this be done only on Windows, if it's for MS
compatibility.  Unless the code before is actually broken, why should
make a incompatible ABI change?  At least, we should add a switch.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Sun, Nov 11, 2007 at 03:21:08PM -0800, Mark Mitchell wrote:
> Richard Guenther wrote:
> 
> >> But it isn't clear if it applies to structure/union. We tested all MS
> >> compilers we have and verified that the above doesn't apply to
> >> structure/union.  To make fastcall compatible with MS compilers, we
> >> should only put scalar arguments in ECX and EDX.
> > 
> > This would be an ABI change from previous releases.  Regardless of
> > whether we want to do this, an entry for the 4.3 changes document
> > should be added.
> 
> I would suggest that this be done only on Windows, if it's for MS
> compatibility.  Unless the code before is actually broken, why should
> make a incompatible ABI change?  At least, we should add a switch.

From "info gcc",

`fastcall'
     On the Intel 386, the `fastcall' attribute causes the compiler to
     pass the first argument (if of integral type) in the register ECX
                              ^^^^^^^^^^^^^^^^^^^
     and the second argument (if of integral type) in the register EDX.
                              ^^^^^^^^^^^^^^^^^^^
     Subsequent and other typed arguments are passed on the stack.
     The called function will pop the arguments off the stack.  If the
     number of arguments is variable all arguments are pushed on the
     stack.

It looks like gcc document follows MS compiler and we didn't implement
it correctly. If we want to add a switch, the default should follow
gcc document.


H.J.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Mark Mitchell]

H.J. Lu wrote:

>> I would suggest that this be done only on Windows, if it's for MS
>> compatibility.  Unless the code before is actually broken, why should
>> make a incompatible ABI change?  At least, we should add a switch.

> It looks like gcc document follows MS compiler and we didn't implement
> it correctly. If we want to add a switch, the default should follow
> gcc document.

Why?  I don't have a strong opinion, and, in any case, this issue is
something for the x86 maintainers to determine.  But what's the virtue
in changing the GCC ABI for GNU/Linux (or any other non-Windows OS)
applications?  Especially in a way that makes it use a less efficient
calling sequence?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Mon, Nov 12, 2007 at 10:48:20PM -0800, Mark Mitchell wrote:
> H.J. Lu wrote:
> 
> >> I would suggest that this be done only on Windows, if it's for MS
> >> compatibility.  Unless the code before is actually broken, why should
> >> make a incompatible ABI change?  At least, we should add a switch.
> 
> > It looks like gcc document follows MS compiler and we didn't implement
> > it correctly. If we want to add a switch, the default should follow
> > gcc document.
> 
> Why?  I don't have a strong opinion, and, in any case, this issue is
> something for the x86 maintainers to determine.  But what's the virtue
> in changing the GCC ABI for GNU/Linux (or any other non-Windows OS)
> applications?  Especially in a way that makes it use a less efficient
> calling sequence?
> 

stdcall is a Windows feature. However, using different calling
conversions on Windows and non-Windows will make OS-independent 
assembly code hard to write. Given that we have been doing the wrong
thing on Windows wthout noticing anything up to now, make it right
shouldn't cause more trouble. We can add a warning when we detect the
ABI correction.

BTW, on Linux, we use regparm, which is more efficient than stdcall.


H.J.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Mark Mitchell]

H.J. Lu wrote:

> stdcall is a Windows feature. However, using different calling
> conversions on Windows and non-Windows will make OS-independent 
> assembly code hard to write. Given that we have been doing the wrong
> thing on Windows wthout noticing anything up to now, make it right
> shouldn't cause more trouble. We can add a warning when we detect the
> ABI correction.
> 
> BTW, on Linux, we use regparm, which is more efficient than stdcall.

Like I said, I think this is for the x86 maintainers to decide.  But, I
think that breaking backwards compatibility, especially at the binary
level, is something we ought to think about very carefully.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Tue, Nov 13, 2007 at 08:28:55AM -0800, Mark Mitchell wrote:
> H.J. Lu wrote:
> 
> > stdcall is a Windows feature. However, using different calling
> > conversions on Windows and non-Windows will make OS-independent 
> > assembly code hard to write. Given that we have been doing the wrong
> > thing on Windows wthout noticing anything up to now, make it right
> > shouldn't cause more trouble. We can add a warning when we detect the
> > ABI correction.
> > 
> > BTW, on Linux, we use regparm, which is more efficient than stdcall.
> 
> Like I said, I think this is for the x86 maintainers to decide.  But, I
> think that breaking backwards compatibility, especially at the binary
> level, is something we ought to think about very carefully.
> 

It depends on how you see it. To me, the current behavior is binary
incompatible with MS compilers, for which this feature is designed
for. My patch makes it binary incompatible with older gcc, but
makes the new gcc binary compatible with all assembly codes and
object files which conform to gcc document as well as MS compilers.


H.J.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Tue, Nov 13, 2007 at 10:36:24AM -0800, H.J. Lu wrote:
> On Tue, Nov 13, 2007 at 08:28:55AM -0800, Mark Mitchell wrote:
> > H.J. Lu wrote:
> > 
> > > stdcall is a Windows feature. However, using different calling
> > > conversions on Windows and non-Windows will make OS-independent 
> > > assembly code hard to write. Given that we have been doing the wrong
> > > thing on Windows wthout noticing anything up to now, make it right
> > > shouldn't cause more trouble. We can add a warning when we detect the
> > > ABI correction.
> > > 
> > > BTW, on Linux, we use regparm, which is more efficient than stdcall.
> > 
> > Like I said, I think this is for the x86 maintainers to decide.  But, I
> > think that breaking backwards compatibility, especially at the binary
> > level, is something we ought to think about very carefully.
> > 
> 
> It depends on how you see it. To me, the current behavior is binary
> incompatible with MS compilers, for which this feature is designed
> for. My patch makes it binary incompatible with older gcc, but
> makes the new gcc binary compatible with all assembly codes and
> object files which conform to gcc document as well as MS compilers.
> 

We have changed the i386 fastcall abi between gcc 3.4 and gcc 4.0:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34001#c7

I think we should fix it for gcc 4.3. Jan, Uros, can you take a look
at it?

Thanks.


H.J.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Uros Bizjak]

On Nov 27, 2007 3:41 AM, H.J. Lu <hjl@lucon.org> wrote:

> > It depends on how you see it. To me, the current behavior is binary
> > incompatible with MS compilers, for which this feature is designed
> > for. My patch makes it binary incompatible with older gcc, but
> > makes the new gcc binary compatible with all assembly codes and
> > object files which conform to gcc document as well as MS compilers.
> >
>
> We have changed the i386 fastcall abi between gcc 3.4 and gcc 4.0:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34001#c7
>
> I think we should fix it for gcc 4.3. Jan, Uros, can you take a look
> at it?

Jan is an expert for ABI issues, so IMO he should have the last word.

Uros.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Jan Hubicka]

> On Nov 27, 2007 3:41 AM, H.J. Lu <hjl@lucon.org> wrote:
> 
> > > It depends on how you see it. To me, the current behavior is binary
> > > incompatible with MS compilers, for which this feature is designed
> > > for. My patch makes it binary incompatible with older gcc, but
> > > makes the new gcc binary compatible with all assembly codes and
> > > object files which conform to gcc document as well as MS compilers.
> > >
> >
> > We have changed the i386 fastcall abi between gcc 3.4 and gcc 4.0:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34001#c7
> >
> > I think we should fix it for gcc 4.3. Jan, Uros, can you take a look
> > at it?
> 
> Jan is an expert for ABI issues, so IMO he should have the last word.

I was participating in the x86-64 ABI decisions so I can give some
insight there, but i386 well predates me :)

My understanding is that fastcall was invented for compatibility with
"standard" conventions used by MS and Watcom compilers, so we should
follow what they do (ie not pass aggregates in registers) more if it
actually is documented in our texinfo.

I don't like much the idea of changing behaviour for Windows targets
only, it seems like good trap for code trying to be portable across
both.  So I personally would vote for changing default behaviour and
documenting it in NEWS document as suggested.

I would not expect much fallout - it is uncommon to pass little
aggregates to assembly code and using fastcall in library API would be
quite silly.

Honza
> 
> Uros.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Tue, Nov 27, 2007 at 11:43:01PM +0100, Jan Hubicka wrote:
> > On Nov 27, 2007 3:41 AM, H.J. Lu <hjl@lucon.org> wrote:
> > 
> > > > It depends on how you see it. To me, the current behavior is binary
> > > > incompatible with MS compilers, for which this feature is designed
> > > > for. My patch makes it binary incompatible with older gcc, but
> > > > makes the new gcc binary compatible with all assembly codes and
> > > > object files which conform to gcc document as well as MS compilers.
> > > >
> > >
> > > We have changed the i386 fastcall abi between gcc 3.4 and gcc 4.0:
> > >
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34001#c7
> > >
> > > I think we should fix it for gcc 4.3. Jan, Uros, can you take a look
> > > at it?
> > 
> > Jan is an expert for ABI issues, so IMO he should have the last word.
> 
> I was participating in the x86-64 ABI decisions so I can give some
> insight there, but i386 well predates me :)
> 
> My understanding is that fastcall was invented for compatibility with
> "standard" conventions used by MS and Watcom compilers, so we should
> follow what they do (ie not pass aggregates in registers) more if it
> actually is documented in our texinfo.
> 
> I don't like much the idea of changing behaviour for Windows targets
> only, it seems like good trap for code trying to be portable across
> both.  So I personally would vote for changing default behaviour and
> documenting it in NEWS document as suggested.
> 
> I would not expect much fallout - it is uncommon to pass little
> aggregates to assembly code and using fastcall in library API would be
> quite silly.
> 
> Honza

This is the change I checked in. I also checked in the change for
gcc 4.3 change document.

H.J.
---
2007-11-27  H.J. Lu  <hongjiu.lu@intel.com>
	    Joey Ye <joey.ye@intel.com>

	PR target/34001
	* config/i386/i386.c (function_arg_32): Don't pass aggregate
	arguments in ECX/EDX for fastcall.

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 130486)
+++ config/i386/i386.c	(working copy)
@@ -4253,10 +4253,13 @@ function_arg_32 (CUMULATIVE_ARGS *cum, e
 	  int regno = cum->regno;
 
 	  /* Fastcall allocates the first two DWORD (SImode) or
-	     smaller arguments to ECX and EDX.  */
+            smaller arguments to ECX and EDX if it isn't an
+            aggregate type .  */
 	  if (cum->fastcall)
 	    {
-	      if (mode == BLKmode || mode == DImode)
+	      if (mode == BLKmode
+		  || mode == DImode
+		  || (type && AGGREGATE_TYPE_P (type)))
 	        break;
 
 	      /* ECX not EAX is the first allocated register.  */

RE: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Ye, Joey]

HJ,

You patch has a minor problem that fastcall will pass non-scalar integer parameter in EAX/ECX/EDX, which is still imcompatible to MS fastcall. MSVC pass parameter on stack unless it is scalar integer less equal to 32 bits.

With this patch GCC fastcall passes first parameter in EAX when it is not a scalar integer less equal to 32 bits. Check following example and review the patch fixing it:

#define MAGIC 123
typedef struct {
char c1, c2;
} a_type;

int
#ifdef _GNU_C_
__attribute__ ((fastcall))
#else
__fastcall
#endif
foo(a_type a, int b, int c, int d)
{
        return a.c1 + b + c + d;
}

int main()
{
        a_type a;
        a.c1=MAGIC;
        return foo(a,0,1,2) == MAGIC + 1 ? 0 : 1;
}


MSVC 6 and 7:
@foo@16:
  00000000: 55              push      ebp
  00000001: 8B EC           mov       ebp,esp
  00000003: 83 EC 08        sub       esp,8
  00000006: 89 55 F8        mov       dword ptr [ebp-8],edx   ;arg3 in edx
  00000009: 89 4D FC        mov       dword ptr [ebp-4],ecx   ;arg2 in ecx
  0000000C: 0F BE 45 08     movsx     eax,byte ptr [ebp+8]   ;arg1 in stack
  00000010: 03 45 FC        add       eax,dword ptr [ebp-4]
  00000013: 03 45 F8        add       eax,dword ptr [ebp-8]
  00000016: 03 45 0C        add       eax,dword ptr [ebp+0Ch] ;arg4 in stack
  00000019: 8B E5           mov       esp,ebp
  0000001B: 5D              pop       ebp
  0000001C: C2 08 00        ret       8
_main:
  0000001F: 55              push      ebp
  00000020: 8B EC           mov       ebp,esp
  00000022: 51              push      ecx
  00000023: C6 45 FC 7B     mov       byte ptr [ebp-4],7Bh
  00000027: 6A 02           push      2                    ;param4 in stack
  00000029: BA 01 00 00 00  mov       edx,1                ;param3 in edx
  0000002E: 33 C9           xor       ecx,ecx              ;param2 in ecx
  00000030: 66 8B 45 FC     mov       ax,word ptr [ebp-4]  ;param1 in eax
  00000034: 50              push      eax
  00000035: E8 00 00 00 00  call      0000003A
  0000003A: 33 C9           xor       ecx,ecx
  0000003C: 83 F8 7C        cmp       eax,7Ch
  0000003F: 0F 95 C1        setne     cl
  00000042: 8B C1           mov       eax,ecx
  00000044: 8B E5           mov       esp,ebp
  00000046: 5D              pop       ebp
  00000047: C3              ret
 
GCC 4.3 with your fastcall patch: 
        .file   "f.c"
        .text
.globl foo
        .type   foo, @function
foo:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $8, %esp
        movw    %ax, -2(%ebp)  ;arg1 in eax
        movl    %edx, -8(%ebp) ;arg2 in edx
        movzbl  -2(%ebp), %eax
        movsbl  %al,%eax
        addl    -8(%ebp), %eax
        addl    8(%ebp), %eax  ;arg3 in stack
        addl    12(%ebp), %eax ;arg4 in stack
        leave
        ret     $8
        .size   foo, .-foo
.globl main
        .type   main, @function
main:
        leal    4(%esp), %ecx
        andl    $-16, %esp
        pushl   -4(%ecx)
        pushl   %ebp
        movl    %esp, %ebp
        pushl   %ecx
        subl    $24, %esp
        movb    $123, -6(%ebp)
        movzwl  -6(%ebp), %eax ; param1 in eax
        movl    $2, 4(%esp)    ; param4 in stack
        movl    $1, (%esp)     ; param3 in stack
        movl    $0, %edx       ; param2 in edx
        call    foo
        subl    $8, %esp
        cmpl    $124, %eax
        setne   %al
        movzbl  %al, %eax
        movl    -4(%ebp), %ecx
        leave
        leal    -4(%ecx), %esp
        ret
        .size   main, .-main
        .ident  "GCC: (GNU) 4.3.0 20071104 (experimental) [trunk revision 129880]"
        .section        .note.GNU-stack,"",@progbits


Here is a patch to fix it:
Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c      (revision 129880)
+++ gcc/config/i386/i386.c      (working copy)
@@ -4229,9 +4229,12 @@
          int regno = cum->regno;

          /* Fastcall allocates the first two DWORD (SImode) or
-            smaller arguments to ECX and EDX.  */
+            smaller arguments to ECX and EDX if it isn't an
+            aggregate type .  */
          if (cum->fastcall)
            {
+             if (type && AGGREGATE_TYPE_P (type))
+               break;
              if (mode == BLKmode || mode == DImode)
                break;
  
With this patch generated code will be:
        .file   "f.c"
        .text
.globl foo
        .type   foo, @function
foo:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $4, %esp
        movl    %edx, -4(%ebp)
        movzbl  8(%ebp), %eax
        movsbl  %al,%eax
        addl    -4(%ebp), %eax
        addl    12(%ebp), %eax
        addl    16(%ebp), %eax
        leave
        ret     $12
        .size   foo, .-foo
.globl main
        .type   main, @function
main:
        leal    4(%esp), %ecx
        andl    $-16, %esp
        pushl   -4(%ecx)
        pushl   %ebp
        movl    %esp, %ebp
        pushl   %ecx
        subl    $28, %esp
        movb    $123, -6(%ebp)
        movl    $2, 8(%esp)
        movl    $1, 4(%esp)
        movzwl  -6(%ebp), %eax
        movw    %ax, (%esp)
        movl    $0, %edx
        call    foo
        subl    $12, %esp
        cmpl    $124, %eax
        setne   %al
        movzbl  %al, %eax
        movl    -4(%ebp), %ecx
        leave
        leal    -4(%ecx), %esp
        ret
        .size   main, .-main
        .ident  "GCC: (GNU) 4.3.0 20071104 (experimental) [trunk revision 129880]"
        .section        .note.GNU-stack,"",@progbits

Thanks - Joey

-----Original Message-----
From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] On Behalf Of H.J. Lu
Sent: 2007年11月10日 1:01
To: gcc-patches@gcc.gnu.org
Cc: jh@suse.cz; ubizjak@gmail.com
Subject: PATCH: PR target/34001: Incorrect x86 fastcall behavior

From

http://msdn2.microsoft.com/en-us/library/6xa169sk(VS.80).aspx

"The first two DWORD or smaller arguments are passed in ECX and EDX
registers; all other arguments are passed right to left."

But it isn't clear if it applies to structure/union. We tested all MS
compilers we have and verified that the above doesn't apply to
structure/union.  To make fastcall compatible with MS compilers, we
should only put scalar arguments in ECX and EDX.



H.J.
---
2007-11-09  H.J. Lu  <hongjiu.lu@intel.com>

        PR target/34001
        * config/i386/i386.c (function_arg_32): Don't pass aggregate
        arguments in ECX/EDX for fastcall.

--- gcc/config/i386/i386.c.fastcall     2007-11-06 19:52:12.000000000 -0800
+++ gcc/config/i386/i386.c      2007-11-08 11:48:38.000000000 -0800
@@ -4233,8 +4233,10 @@ function_arg_32 (CUMULATIVE_ARGS *cum, e
          int regno = cum->regno;

          /* Fastcall allocates the first two DWORD (SImode) or
-            smaller arguments to ECX and EDX.  */
-         if (cum->fastcall)
+            smaller arguments to ECX and EDX if it isn't an
+            aggregate type .  */
+         if (cum->fastcall
+             && (!type || !AGGREGATE_TYPE_P (type)))
            {
              if (mode == BLKmode || mode == DImode)
                break;

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Fri, Nov 16, 2007 at 12:35:47PM +0800, Ye, Joey wrote:
> HJ,
> 
> You patch has a minor problem that fastcall will pass non-scalar integer parameter in EAX/ECX/EDX, which is still imcompatible to MS fastcall. MSVC pass parameter on stack unless it is scalar integer less equal to 32 bits.
> 
> With this patch GCC fastcall passes first parameter in EAX when it is not a scalar integer less equal to 32 bits. Check following example and review the patch fixing it:
> 
> Here is a patch to fix it:
> Index: gcc/config/i386/i386.c
> ===================================================================
> --- gcc/config/i386/i386.c      (revision 129880)
> +++ gcc/config/i386/i386.c      (working copy)
> @@ -4229,9 +4229,12 @@
>           int regno = cum->regno;
> 
>           /* Fastcall allocates the first two DWORD (SImode) or
> -            smaller arguments to ECX and EDX.  */
> +            smaller arguments to ECX and EDX if it isn't an
> +            aggregate type .  */
>           if (cum->fastcall)
>             {
> +             if (type && AGGREGATE_TYPE_P (type))
> +               break;
>               if (mode == BLKmode || mode == DImode)
>                 break;
>   

Yes, this version is correct. Thanks.


H.J.

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[H.J. Lu]

On Thu, Nov 15, 2007 at 08:51:02PM -0800, H.J. Lu wrote:
> On Fri, Nov 16, 2007 at 12:35:47PM +0800, Ye, Joey wrote:
> > HJ,
> > 
> > You patch has a minor problem that fastcall will pass non-scalar integer parameter in EAX/ECX/EDX, which is still imcompatible to MS fastcall. MSVC pass parameter on stack unless it is scalar integer less equal to 32 bits.
> > 
> > With this patch GCC fastcall passes first parameter in EAX when it is not a scalar integer less equal to 32 bits. Check following example and review the patch fixing it:
> > 
> > Here is a patch to fix it:
> > Index: gcc/config/i386/i386.c
> > ===================================================================
> > --- gcc/config/i386/i386.c      (revision 129880)
> > +++ gcc/config/i386/i386.c      (working copy)
> > @@ -4229,9 +4229,12 @@
> >           int regno = cum->regno;
> > 
> >           /* Fastcall allocates the first two DWORD (SImode) or
> > -            smaller arguments to ECX and EDX.  */
> > +            smaller arguments to ECX and EDX if it isn't an
> > +            aggregate type .  */
> >           if (cum->fastcall)
> >             {
> > +             if (type && AGGREGATE_TYPE_P (type))
> > +               break;
> >               if (mode == BLKmode || mode == DImode)
> >                 break;
> >   
> 
> Yes, this version is correct. Thanks.
> 
> 

Here is an equivalent patch.


H.J.
----
2007-11-15  H.J. Lu  <hongjiu.lu@intel.com>
	    Joey Ye <joey.ye@intel.com>

	PR target/34001
	* config/i386/i386.c (function_arg_32): Don't pass aggregate
	arguments in ECX/EDX for fastcall.

--- gcc/config/i386/i386.c.fastcall	2007-11-16 05:41:46.000000000 -0800
+++ gcc/config/i386/i386.c	2007-11-16 05:46:43.000000000 -0800
@@ -4257,10 +4257,12 @@ function_arg_32 (CUMULATIVE_ARGS *cum, e
 	  int regno = cum->regno;
 
 	  /* Fastcall allocates the first two DWORD (SImode) or
-	     smaller arguments to ECX and EDX.  */
+            smaller arguments to ECX and EDX if it isn't an
+            aggregate type .  */
 	  if (cum->fastcall)
 	    {
-	      if (mode == BLKmode || mode == DImode)
+	      if (mode == BLKmode || mode == DImode
+		  || (type && AGGREGATE_TYPE_P (type)))
 	        break;
 
 	      /* ECX not EAX is the first allocated register.  */

Re: PATCH: PR target/34001: Incorrect x86 fastcall behavior

[Ross Ridge]

Mark Mitchell writes:
> Why?  I don't have a strong opinion, and, in any case, this issue is
> something for the x86 maintainers to determine.  But what's the virtue
> in changing the GCC ABI for GNU/Linux (or any other non-Windows OS)
> applications?  Especially in a way that makes it use a less efficient
> calling sequence?

I can only see two reasons why fastcall would've been used on non-Windows
OS.  One is for compatibility with assembler or object code that was
originally written for Windows.  The other is in code that should've
used the more efficient regparam(3) attribute.  Changing the behavior
of fastcall to be compatible with Microsoft's compiler shouldn't cause
problems in the first case.  In the second case I think code that would
break would be very rare, probably non-existant.  Code like this would
have to use the fastcall attribute in a function that both takes a small
aggregate by value and could be called from a function compiled with a
different version of GCC.

						Ross Ridge