* [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
@ 2007-07-26 9:25 Richard Guenther
2007-07-26 14:17 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guenther @ 2007-07-26 9:25 UTC (permalink / raw)
To: gcc-patches
This patch:
2007-07-09 Richard Guenther <rguenther@suse.de>
cp/
* decl.c (start_preparsed_function): Do not promote return type.
* c-decl.c (start_function): Do not promote return type.
changed how we extend a signed char to fill %eax on x86 from sign to
zero extension which now makes libffi.call/return_sc.c fail because
this testcase explicitly checks for sign-extension of signed character
return values.
Fixed by verifying the return value as signed char instead.
This patch was approved by Andreas Tobler in the PR audit trail and
committed to mainline.
Richard.
2007-07-26 Richard Guenther <rguenther@suse.de>
PR testsuite/32843
* testsuite/libffi.call/return_sc.c (main): Verify call
result as signed char, not ffi_arg.
Index: testsuite/libffi.call/return_sc.c
===================================================================
*** testsuite/libffi.call/return_sc.c (revision 126677)
--- testsuite/libffi.call/return_sc.c (working copy)
*************** int main (void)
*** 30,36 ****
sc < (signed char) 127; sc++)
{
ffi_call(&cif, FFI_FN(return_sc), &rint, values);
! CHECK(rint == (ffi_arg) sc);
}
exit(0);
}
--- 30,36 ----
sc < (signed char) 127; sc++)
{
ffi_call(&cif, FFI_FN(return_sc), &rint, values);
! CHECK((signed char) rint == sc);
}
exit(0);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-26 9:25 [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values Richard Guenther
@ 2007-07-26 14:17 ` Paolo Bonzini
2007-07-26 14:25 ` Richard Guenther
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2007-07-26 14:17 UTC (permalink / raw)
To: Richard Guenther; +Cc: gcc-patches, Andreas Tobler
> This patch was approved by Andreas Tobler in the PR audit trail and
> committed to mainline.
Couldn't it be also a FFI bug? If the ABI is unclear, I would have
expected libffi to do the cast itself...
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-26 14:17 ` Paolo Bonzini
@ 2007-07-26 14:25 ` Richard Guenther
2007-07-26 14:28 ` Andrew Haley
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guenther @ 2007-07-26 14:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: gcc-patches, Andreas Tobler
On Thu, 26 Jul 2007, Paolo Bonzini wrote:
>
> > This patch was approved by Andreas Tobler in the PR audit trail and
> > committed to mainline.
>
> Couldn't it be also a FFI bug? If the ABI is unclear, I would have expected
> libffi to do the cast itself...
Maybe. Still it shouldn't matter -- the only valid thing to do with
a signed char is to use it as signed char. Everything else requires
a proper conversion.
Richard.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-26 14:25 ` Richard Guenther
@ 2007-07-26 14:28 ` Andrew Haley
2007-07-26 14:48 ` Richard Guenther
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Haley @ 2007-07-26 14:28 UTC (permalink / raw)
To: Richard Guenther; +Cc: Paolo Bonzini, gcc-patches, Andreas Tobler
Richard Guenther writes:
> On Thu, 26 Jul 2007, Paolo Bonzini wrote:
>
> >
> > > This patch was approved by Andreas Tobler in the PR audit trail and
> > > committed to mainline.
> >
> > Couldn't it be also a FFI bug? If the ABI is unclear, I would have expected
> > libffi to do the cast itself...
>
> Maybe. Still it shouldn't matter -- the only valid thing to do with
> a signed char is to use it as signed char. Everything else requires
> a proper conversion.
Perhaps so, but even if this is not strictly speaking a libffi bug, it
is a libffi quality of implementation issue, and it does potentially
break code.
The intention of the testcase that has been broken is precisely to
check that signed characters are sign extended. When this fails
libffi should be changed, not the testcase.
Andrew.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-26 14:28 ` Andrew Haley
@ 2007-07-26 14:48 ` Richard Guenther
2007-07-26 14:54 ` Andrew Haley
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guenther @ 2007-07-26 14:48 UTC (permalink / raw)
To: Andrew Haley; +Cc: Paolo Bonzini, gcc-patches, Andreas Tobler
On Thu, 26 Jul 2007, Andrew Haley wrote:
> Richard Guenther writes:
> > On Thu, 26 Jul 2007, Paolo Bonzini wrote:
> >
> > >
> > > > This patch was approved by Andreas Tobler in the PR audit trail and
> > > > committed to mainline.
> > >
> > > Couldn't it be also a FFI bug? If the ABI is unclear, I would have expected
> > > libffi to do the cast itself...
> >
> > Maybe. Still it shouldn't matter -- the only valid thing to do with
> > a signed char is to use it as signed char. Everything else requires
> > a proper conversion.
>
> Perhaps so, but even if this is not strictly speaking a libffi bug, it
> is a libffi quality of implementation issue, and it does potentially
> break code.
>
> The intention of the testcase that has been broken is precisely to
> check that signed characters are sign extended. When this fails
> libffi should be changed, not the testcase.
If you are familiar with libffi can you try to fix this then and
revert the testsuite change?
Thanks a lot,
Richard.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-26 14:48 ` Richard Guenther
@ 2007-07-26 14:54 ` Andrew Haley
2007-07-27 18:28 ` Andrew Haley
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Haley @ 2007-07-26 14:54 UTC (permalink / raw)
To: Richard Guenther; +Cc: Paolo Bonzini, gcc-patches, Andreas Tobler
Richard Guenther writes:
> On Thu, 26 Jul 2007, Andrew Haley wrote:
>
> > Richard Guenther writes:
> > > On Thu, 26 Jul 2007, Paolo Bonzini wrote:
> > >
> > > >
> > > > > This patch was approved by Andreas Tobler in the PR audit trail and
> > > > > committed to mainline.
> > > >
> > > > Couldn't it be also a FFI bug? If the ABI is unclear, I would have expected
> > > > libffi to do the cast itself...
> > >
> > > Maybe. Still it shouldn't matter -- the only valid thing to do with
> > > a signed char is to use it as signed char. Everything else requires
> > > a proper conversion.
> >
> > Perhaps so, but even if this is not strictly speaking a libffi bug, it
> > is a libffi quality of implementation issue, and it does potentially
> > break code.
> >
> > The intention of the testcase that has been broken is precisely to
> > check that signed characters are sign extended. When this fails
> > libffi should be changed, not the testcase.
>
> If you are familiar with libffi can you try to fix this then and
> revert the testsuite change?
Sure.
Andrew.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-26 14:54 ` Andrew Haley
@ 2007-07-27 18:28 ` Andrew Haley
2007-07-27 19:28 ` Richard Guenther
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Haley @ 2007-07-27 18:28 UTC (permalink / raw)
To: Richard Guenther, Paolo Bonzini, gcc-patches, Andreas Tobler
Andrew Haley writes:
> Richard Guenther writes:
> > On Thu, 26 Jul 2007, Andrew Haley wrote:
> >
> > > Richard Guenther writes:
> > > > On Thu, 26 Jul 2007, Paolo Bonzini wrote:
> > > >
> > > > >
> > > > > > This patch was approved by Andreas Tobler in the PR audit trail and
> > > > > > committed to mainline.
> > > > >
> > > > > Couldn't it be also a FFI bug? If the ABI is unclear, I would have expected
> > > > > libffi to do the cast itself...
> > > >
> > > > Maybe. Still it shouldn't matter -- the only valid thing to do with
> > > > a signed char is to use it as signed char. Everything else requires
> > > > a proper conversion.
> > >
> > > Perhaps so, but even if this is not strictly speaking a libffi bug, it
> > > is a libffi quality of implementation issue, and it does potentially
> > > break code.
> > >
> > > The intention of the testcase that has been broken is precisely to
> > > check that signed characters are sign extended. When this fails
> > > libffi should be changed, not the testcase.
> >
> > If you are familiar with libffi can you try to fix this then and
> > revert the testsuite change?
>
> Sure.
OK, I've looked and every libffi target except x86 has explicit code
to do sign extend retvals. I presume that such code isn't present for
x86 because hitherto $eax has always been properly sign extended for
the return type.
So, are we *absolutely* sure that gcc hasn't broken the ABI? I can
certainly imagine some cases where a change like this one (gcc no
longer sign extends retvals) would break code in interesting ways.
Andrew.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-27 18:28 ` Andrew Haley
@ 2007-07-27 19:28 ` Richard Guenther
2007-07-30 13:58 ` Andrew Haley
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guenther @ 2007-07-27 19:28 UTC (permalink / raw)
To: Andrew Haley; +Cc: Paolo Bonzini, gcc-patches, Andreas Tobler
On Fri, 27 Jul 2007, Andrew Haley wrote:
> Andrew Haley writes:
>
> OK, I've looked and every libffi target except x86 has explicit code
> to do sign extend retvals. I presume that such code isn't present for
> x86 because hitherto $eax has always been properly sign extended for
> the return type.
>
> So, are we *absolutely* sure that gcc hasn't broken the ABI? I can
> certainly imagine some cases where a change like this one (gcc no
> longer sign extends retvals) would break code in interesting ways.
Well, if every other libffi target has explicit sign extension code
then the change (which is to generic code!) if it breaks the ABI shows
that the x86 target machinery is lacking code to do sign extend retvals.
Of course I'd like to see what other language frontends to for returning
signed/unsigned quantities of QImode or HImode on x86. For example
Fortran does zero extension in 4.2.1 (well, really the backend is -
the frontend simply returns HImode, like mainline now does for C as well
after the change):
foo_:
pushl %ebp
movl %esp, %ebp
movl 8(%ebp), %eax
movzwl (%eax), %eax
popl %ebp
ret
function foo(bar)
integer*2 foo,bar
foo = bar
end
if backends other than x86 still sign-extend for QImode returns in C
the it is the x86 backend that needs to be fixed if we decide that
it makes a difference that we need to care about.
Richard.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-27 19:28 ` Richard Guenther
@ 2007-07-30 13:58 ` Andrew Haley
2007-08-06 12:46 ` Andrew Haley
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Haley @ 2007-07-30 13:58 UTC (permalink / raw)
To: Richard Guenther; +Cc: Paolo Bonzini, gcc-patches, Andreas Tobler
This fixes the real bug, which is that x86 libffi wasn't correctly
sign/zero extending retvals. I also changed the long chain of
if/then/elses into a switch and tidied things a bit.
While I was doing this, I found a bug in ffi_call_SYSV: in some cases
we were popping too many values from the stack, but that was being
concealed by a later movl %ebp,%esp in the epilogue. As it turns out,
there isn't any need to pop the args from the stack after calling the
user's target function, so I just removed the code that did that.
Passes full testsuite on 32/64, no regressions.
Andrew.
2007-07-30 Andrew Haley <aph@redhat.com>
PR testsuite/32843
* src/x86/ffi.c (ffi_prep_cif_machdep): in x86 case, add code for
signed/unsigned int8/16.
* src/x86/sysv.S (ffi_call_SYSV): Rewrite to:
Use a jump table.
Remove code to pop args from the stack after call.
Special-case signed/unsigned int8/16.
* testsuite/libffi.call/return_sc.c (main): Revert.
Index: src/x86/ffi.c
===================================================================
--- src/x86/ffi.c (revision 127059)
+++ src/x86/ffi.c (working copy)
@@ -121,7 +121,12 @@
case FFI_TYPE_VOID:
#ifdef X86
case FFI_TYPE_STRUCT:
+ case FFI_TYPE_UINT8:
+ case FFI_TYPE_UINT16:
+ case FFI_TYPE_SINT8:
+ case FFI_TYPE_SINT16:
#endif
+
case FFI_TYPE_SINT64:
case FFI_TYPE_FLOAT:
case FFI_TYPE_DOUBLE:
Index: src/x86/sysv.S
===================================================================
--- src/x86/sysv.S (revision 127059)
+++ src/x86/sysv.S (working copy)
@@ -59,16 +59,15 @@
call *28(%ebp)
- /* Remove the space we pushed for the args */
- movl 16(%ebp),%ecx
- addl %ecx,%esp
-
/* Load %ecx with the return type code */
movl 20(%ebp),%ecx
+ /* Protect %esi. We're going to pop it in the epilogue. */
+ pushl %esi
+
/* If the return value pointer is NULL, assume no return value. */
cmpl $0,24(%ebp)
- jne retint
+ jne 0f
/* Even if there is no space for the return value, we are
obliged to handle floating-point values. */
@@ -78,51 +77,84 @@
jmp epilogue
-retint:
- cmpl $FFI_TYPE_INT,%ecx
- jne retfloat
- /* Load %ecx with the pointer to storage for the return value */
- movl 24(%ebp),%ecx
- movl %eax,0(%ecx)
- jmp epilogue
+0:
+ call 1f
+
+.Lstore_table:
+ .long noretval-.Lstore_table /* FFI_TYPE_VOID */
+ .long retint-.Lstore_table /* FFI_TYPE_INT */
+ .long retfloat-.Lstore_table /* FFI_TYPE_FLOAT */
+ .long retdouble-.Lstore_table /* FFI_TYPE_DOUBLE */
+ .long retlongdouble-.Lstore_table /* FFI_TYPE_LONGDOUBLE */
+ .long retuint8-.Lstore_table /* FFI_TYPE_UINT8 */
+ .long retsint8-.Lstore_table /* FFI_TYPE_SINT8 */
+ .long retuint16-.Lstore_table /* FFI_TYPE_UINT16 */
+ .long retsint16-.Lstore_table /* FFI_TYPE_SINT16 */
+ .long retint-.Lstore_table /* FFI_TYPE_UINT32 */
+ .long retint-.Lstore_table /* FFI_TYPE_SINT32 */
+ .long retint64-.Lstore_table /* FFI_TYPE_UINT64 */
+ .long retint64-.Lstore_table /* FFI_TYPE_SINT64 */
+ .long retstruct-.Lstore_table /* FFI_TYPE_STRUCT */
+ .long retint-.Lstore_table /* FFI_TYPE_POINTER */
+
+1:
+ pop %esi
+ add (%esi, %ecx, 4), %esi
+ jmp *%esi
+
+ /* Sign/zero extend as appropriate. */
+retsint8:
+ movsbl %al, %eax
+ jmp retint
+
+retsint16:
+ movswl %ax, %eax
+ jmp retint
+
+retuint8:
+ movzbl %al, %eax
+ jmp retint
+
+retuint16:
+ movzwl %ax, %eax
+ jmp retint
retfloat:
- cmpl $FFI_TYPE_FLOAT,%ecx
- jne retdouble
/* Load %ecx with the pointer to storage for the return value */
movl 24(%ebp),%ecx
fstps (%ecx)
jmp epilogue
retdouble:
- cmpl $FFI_TYPE_DOUBLE,%ecx
- jne retlongdouble
/* Load %ecx with the pointer to storage for the return value */
movl 24(%ebp),%ecx
fstpl (%ecx)
jmp epilogue
retlongdouble:
- cmpl $FFI_TYPE_LONGDOUBLE,%ecx
- jne retint64
/* Load %ecx with the pointer to storage for the return value */
movl 24(%ebp),%ecx
fstpt (%ecx)
jmp epilogue
retint64:
- cmpl $FFI_TYPE_SINT64,%ecx
- jne retstruct
/* Load %ecx with the pointer to storage for the return value */
movl 24(%ebp),%ecx
movl %eax,0(%ecx)
movl %edx,4(%ecx)
+ jmp epilogue
+retint:
+ /* Load %ecx with the pointer to storage for the return value */
+ movl 24(%ebp),%ecx
+ movl %eax,0(%ecx)
+
retstruct:
/* Nothing to do! */
noretval:
epilogue:
+ popl %esi
movl %ebp,%esp
popl %ebp
ret
@@ -162,7 +194,15 @@
movl -12(%ebp), %ecx
cmpl $FFI_TYPE_INT, %eax
je .Lcls_retint
- cmpl $FFI_TYPE_FLOAT, %eax
+
+ /* Handle FFI_TYPE_UINT8, FFI_TYPE_SINT8, FFI_TYPE_UINT16,
+ FFI_TYPE_SINT16, FFI_TYPE_UINT32, FFI_TYPE_SINT32. */
+ cmpl $FFI_TYPE_UINT64, %eax
+ jge 0f
+ cmpl $FFI_TYPE_UINT8, %eax
+ jge .Lcls_retint
+
+0: cmpl $FFI_TYPE_FLOAT, %eax
je .Lcls_retfloat
cmpl $FFI_TYPE_DOUBLE, %eax
je .Lcls_retdouble
Index: testsuite/libffi.call/return_sc.c
===================================================================
--- testsuite/libffi.call/return_sc.c (revision 127059)
+++ testsuite/libffi.call/return_sc.c (working copy)
@@ -30,7 +30,7 @@
sc < (signed char) 127; sc++)
{
ffi_call(&cif, FFI_FN(return_sc), &rint, values);
- CHECK((signed char) rint == sc);
+ CHECK(rint == (ffi_arg) sc);
}
exit(0);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-07-30 13:58 ` Andrew Haley
@ 2007-08-06 12:46 ` Andrew Haley
2007-08-07 12:46 ` Andrew Haley
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Haley @ 2007-08-06 12:46 UTC (permalink / raw)
To: Richard Guenther, Paolo Bonzini, gcc-patches, Andreas Tobler
The PR32843 fix caused libgcj regressions. This turned out to be
because ffi_closure_raw_SYSV duplicates code from ffi_closure_SYSV but
in a way that is difficult to share, and the libffi testsuite doesn't
test raw closures at all. The PR32843 patch needed to be applied in
two places, not just one. :-(
Fixed thusly.
Andrew.
2007-08-06 Andrew Haley <aph@redhat.com>
PR testsuite/32843
* src/x86/sysv.S (ffi_closure_raw_SYSV): Handle FFI_TYPE_UINT8,
FFI_TYPE_SINT8, FFI_TYPE_UINT16, FFI_TYPE_SINT16, FFI_TYPE_UINT32,
FFI_TYPE_SINT32.
Index: sysv.S
===================================================================
*** sysv.S (revision 127238)
--- sysv.S (working copy)
*************** ffi_closure_raw_SYSV:
*** 266,271 ****
--- 266,278 ----
movl CIF_FLAGS_OFFSET(%esi), %eax /* rtype */
cmpl $FFI_TYPE_INT, %eax
je .Lrcls_retint
+ /* Handle FFI_TYPE_UINT8, FFI_TYPE_SINT8, FFI_TYPE_UINT16,
+ FFI_TYPE_SINT16, FFI_TYPE_UINT32, FFI_TYPE_SINT32. */
+ cmpl $FFI_TYPE_UINT64, %eax
+ jge 0f
+ cmpl $FFI_TYPE_UINT8, %eax
+ jge .Lcls_retint
+ 0:
cmpl $FFI_TYPE_FLOAT, %eax
je .Lrcls_retfloat
cmpl $FFI_TYPE_DOUBLE, %eax
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values
2007-08-06 12:46 ` Andrew Haley
@ 2007-08-07 12:46 ` Andrew Haley
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Haley @ 2007-08-07 12:46 UTC (permalink / raw)
To: Richard Guenther, Paolo Bonzini, gcc-patches, Andreas Tobler
I typo'd yesterday's checkin. Sorry for the inconvenience.
Andrew.
2007-08-07 Andrew Haley <aph@redhat.com>
* src/x86/sysv.S (ffi_closure_raw_SYSV): Fix typo in previous
checkin.
Index: sysv.S
===================================================================
--- sysv.S (revision 127266)
+++ sysv.S (working copy)
@@ -272,7 +272,7 @@
cmpl $FFI_TYPE_UINT64, %eax
jge 0f
cmpl $FFI_TYPE_UINT8, %eax
- jge .Lcls_retint
+ jge .Lrcls_retint
0:
cmpl $FFI_TYPE_FLOAT, %eax
je .Lrcls_retfloat
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2007-08-07 12:46 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-07-26 9:25 [PATCH] Fix PR32843, testsuite bug wrt sign/zero-extension of return values Richard Guenther
2007-07-26 14:17 ` Paolo Bonzini
2007-07-26 14:25 ` Richard Guenther
2007-07-26 14:28 ` Andrew Haley
2007-07-26 14:48 ` Richard Guenther
2007-07-26 14:54 ` Andrew Haley
2007-07-27 18:28 ` Andrew Haley
2007-07-27 19:28 ` Richard Guenther
2007-07-30 13:58 ` Andrew Haley
2007-08-06 12:46 ` Andrew Haley
2007-08-07 12:46 ` Andrew Haley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).