public inbox for libffi-discuss@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix handling of uint32_t arguments on the MIPS N32 ABI.
@ 2013-10-23 18:27 Mark H Weaver
  2013-10-26 12:32 ` Anthony Green
  0 siblings, 1 reply; 2+ messages in thread
From: Mark H Weaver @ 2013-10-23 18:27 UTC (permalink / raw)
  To: libffi-discuss

Hello all,

A failing glib test on MIPS N32 has alerted me to a bug in the handling
of unsigned 32-bit integers in libffi.  The MIPSpro N32 ABI Handbook [*]
states:

  "32-bit integer (int) parameters are always sign-extended when passed
   in registers, whether of signed or unsigned type.  [This issue does
   not arise in the o32-bit ABI.]"  (chapter 2, page 6)

[*] http://techpubs.sgi.com/library/manuals/2000/007-2816-005/pdf/007-2816-005.pdf

and indeed GCC generates code that assumes this.
For example, GCC 4.7.3 compiles:

--8<---------------cut here---------------start------------->8---
#include <stdint.h>
#include <stdlib.h>

void
do_test (uint32_t a)
{
  if (a != 4294967253U)
    exit (1);
}
--8<---------------cut here---------------end--------------->8---

as follows (with extraneous cruft stripped):

--8<---------------cut here---------------start------------->8---
do_test:
	li	$2,-43			# 0xffffffffffffffd5
	beq	$4,$2,.L5
	nop

	addiu	$sp,$sp,-16
	sd	$31,8($sp)
	jal	exit
	li	$4,1			# 0x1

.L5:
	j	$31
	nop
--8<---------------cut here---------------end--------------->8---

However, the current libffi code zero-extends 32-bit unsigned integers,
which violates this N32 FFI requirement.

See below for a patch to fix this problem.  I've tested this patch on
the preliminary port of GNU Guix to MIPS N32, which uses a minimally
patched GNU toolchain targetting mips64el-linux-gnu.

Without this patch, glib-2.38.0 fails its test suite, specifically the
gobject/test/signal.c test, which uses libffi to pass a large 32-bit
unsigned integer to a test function that checks the received value.
With this patch, glib-2.38.0 passes its test suite.

Comments and suggestions welcome.

     Regards,
       Mark


Signed-off-by: Mark H Weaver <mhw@netris.org>
---
 ChangeLog      |    5 +++++
 src/mips/ffi.c |    7 +++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index eceef84..e7e5d94 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-10-23  Mark H Weaver  <mhw@netris.org>
+
+	* src/mips/ffi.c: Fix handling of uint32_t arguments on the
+	MIPS N32 ABI.
+
 2013-10-13  Sandra Loosemore  <sandra@codesourcery.com>
 
 	* README: Add Nios II to table of supported platforms.
diff --git a/src/mips/ffi.c b/src/mips/ffi.c
index 03121e3..5d0dd70 100644
--- a/src/mips/ffi.c
+++ b/src/mips/ffi.c
@@ -170,7 +170,14 @@ static void ffi_prep_args(char *stack,
 		break;
 		  
 	      case FFI_TYPE_UINT32:
+#ifdef FFI_MIPS_N32
+		/* The N32 ABI requires that 32-bit integers
+		   be sign-extended to 64-bits, regardless of
+		   whether they are signed or unsigned. */
+		*(ffi_arg *)argp = *(SINT32 *)(* p_argv);
+#else
 		*(ffi_arg *)argp = *(UINT32 *)(* p_argv);
+#endif
 		break;
 
 	      /* This can only happen with 64bit slots.  */
-- 
1.7.5.4

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] Fix handling of uint32_t arguments on the MIPS N32 ABI.
  2013-10-23 18:27 [PATCH] Fix handling of uint32_t arguments on the MIPS N32 ABI Mark H Weaver
@ 2013-10-26 12:32 ` Anthony Green
  0 siblings, 0 replies; 2+ messages in thread
From: Anthony Green @ 2013-10-26 12:32 UTC (permalink / raw)
  To: Mark H Weaver; +Cc: libffi-discuss

Thanks Mark.  I've committed this change.

https://github.com/atgreen/libffi/commit/d3372c54ce7117e80d389ba875dc5b6b2213c71e

AG

On Wed, Oct 23, 2013 at 2:25 PM, Mark H Weaver <mhw@netris.org> wrote:
> Hello all,
>
> A failing glib test on MIPS N32 has alerted me to a bug in the handling
> of unsigned 32-bit integers in libffi.  The MIPSpro N32 ABI Handbook [*]
> states:
>
>   "32-bit integer (int) parameters are always sign-extended when passed
>    in registers, whether of signed or unsigned type.  [This issue does
>    not arise in the o32-bit ABI.]"  (chapter 2, page 6)
>
> [*] http://techpubs.sgi.com/library/manuals/2000/007-2816-005/pdf/007-2816-005.pdf
>
> and indeed GCC generates code that assumes this.
> For example, GCC 4.7.3 compiles:
>
> --8<---------------cut here---------------start------------->8---
> #include <stdint.h>
> #include <stdlib.h>
>
> void
> do_test (uint32_t a)
> {
>   if (a != 4294967253U)
>     exit (1);
> }
> --8<---------------cut here---------------end--------------->8---
>
> as follows (with extraneous cruft stripped):
>
> --8<---------------cut here---------------start------------->8---
> do_test:
>         li      $2,-43                  # 0xffffffffffffffd5
>         beq     $4,$2,.L5
>         nop
>
>         addiu   $sp,$sp,-16
>         sd      $31,8($sp)
>         jal     exit
>         li      $4,1                    # 0x1
>
> .L5:
>         j       $31
>         nop
> --8<---------------cut here---------------end--------------->8---
>
> However, the current libffi code zero-extends 32-bit unsigned integers,
> which violates this N32 FFI requirement.
>
> See below for a patch to fix this problem.  I've tested this patch on
> the preliminary port of GNU Guix to MIPS N32, which uses a minimally
> patched GNU toolchain targetting mips64el-linux-gnu.
>
> Without this patch, glib-2.38.0 fails its test suite, specifically the
> gobject/test/signal.c test, which uses libffi to pass a large 32-bit
> unsigned integer to a test function that checks the received value.
> With this patch, glib-2.38.0 passes its test suite.
>
> Comments and suggestions welcome.
>
>      Regards,
>        Mark
>
>
> Signed-off-by: Mark H Weaver <mhw@netris.org>
> ---
>  ChangeLog      |    5 +++++
>  src/mips/ffi.c |    7 +++++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index eceef84..e7e5d94 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2013-10-23  Mark H Weaver  <mhw@netris.org>
> +
> +       * src/mips/ffi.c: Fix handling of uint32_t arguments on the
> +       MIPS N32 ABI.
> +
>  2013-10-13  Sandra Loosemore  <sandra@codesourcery.com>
>
>         * README: Add Nios II to table of supported platforms.
> diff --git a/src/mips/ffi.c b/src/mips/ffi.c
> index 03121e3..5d0dd70 100644
> --- a/src/mips/ffi.c
> +++ b/src/mips/ffi.c
> @@ -170,7 +170,14 @@ static void ffi_prep_args(char *stack,
>                 break;
>
>               case FFI_TYPE_UINT32:
> +#ifdef FFI_MIPS_N32
> +               /* The N32 ABI requires that 32-bit integers
> +                  be sign-extended to 64-bits, regardless of
> +                  whether they are signed or unsigned. */
> +               *(ffi_arg *)argp = *(SINT32 *)(* p_argv);
> +#else
>                 *(ffi_arg *)argp = *(UINT32 *)(* p_argv);
> +#endif
>                 break;
>
>               /* This can only happen with 64bit slots.  */
> --
> 1.7.5.4
>

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2013-10-26 12:32 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-23 18:27 [PATCH] Fix handling of uint32_t arguments on the MIPS N32 ABI Mark H Weaver
2013-10-26 12:32 ` Anthony Green

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).