endianness issue with FFI (`ext/ffi/tests/gh11934.phpt`)

[NattyNarwhal]

Description

Caught this when provisioning a big endian box for CI.

diff output on test:

--
 --- Struct ---
 bool(true)
 --- Callback return type ---
044- int(42)
044+ int(0)
 --- Other FFI\CData assignment ---
 int(123)
 --- Array element ---
--

The relevant part of that test:

echo "--- Callback return type ---\n";

$ffi = FFI::cdef('
typedef uint32_t (*test_callback)();
typedef struct {
 test_callback call_me;
} my_struct;
');

$struct = $ffi->new('my_struct');
$struct->call_me = function () use ($ffi) {
 $int = $ffi->new('uint32_t');
 $int->cdata = 42;
 return $int;
};

var_dump(($struct->call_me)());

Not familiar with FFI, so haven't looked into it yet, but making a note.

PHP Version

PHP 8.4.0-DEV

Operating System

Gentoo/ppc64

[cmb69]

For instance, the following looks fishy:

php-src/ext/ffi/ffi.c

Lines 787 to 790 in ba748e7

	case ZEND_FFI_TYPE_UINT32:
	lval = zval_get_long(value);
	*(uint32_t*)ptr = lval;
	break;

[NattyNarwhal]

I think reading and writing to the cdata is OK; if I do i.e.

<?php

echo "--- Callback return type ---\n";

$ffi = FFI::cdef('
typedef uint32_t (*test_callback)();
typedef struct {
 test_callback call_me;
} my_struct;
');

$struct = $ffi->new('my_struct');
$struct->call_me = function () use ($ffi) {
 $int = $ffi->new('uint32_t');
 $int->cdata = 42;
 var_dump($int->cdata);
 return $int;
};

var_dump(($struct->call_me)());

...the $int->data var_dump will return 42. What seems broken is function return types. I recall vaguely facing this kind of issue with return types years ago, which made me less interested in using FFI.

[NattyNarwhal]

Oh, found it. Still holds on Linux, which makes it easier to debug.

[NattyNarwhal]

Breakpoint 1, zend_ffi_cdata_to_zval (cdata=0x0, ptr=0x3ffff7a030f0, type=0x1008900a0 <zend_ffi_type_sint32>, read_type=0, rv=0x3ffff7a150d0, 
 flags=0, is_ret=true, debug_union=false) at /home/calvin/php-src/ext/ffi/ffi.c:549
549	fprintf(stderr, "Reading pointer %p type %d\n", ptr, kind);
[...]
(gdb) p/x ((uint32_t*)ptr)[0]
$5 = 0x0
(gdb) p/x ((uint32_t*)ptr)[1]
$6 = 0x11223344

the classic

[NattyNarwhal]

I think the problem is types smaller than machine register size are widened to the size of registers; ffi_trampoline is already doing this with ret = do_alloca(MAX(ret_type->size, sizeof(ffi_arg)), ret_use_heap);. On little endian, this doesn't matter, because of how it gets laid out in memory, but it's fatal for big endian.

Since you can i.e. be working with actual 32-bit stuff referenced by pointer for a cdata and that works fine, I'm suspecting the fix might be to change the return FFI type before calling zend_ffi_cdata_to_zval to one that matches the machine width. I think values are widened in arguments too, so it might need to be done there too - except it's really ABI dependent.

[ndossche]

And most annoyingly is that almost none of the core developers have access to big endian machines. A similar problem already exists for AArch64, but less problematic than big endian platforms.

[NattyNarwhal]

I can provide access to a BE machine for developers. I'm also going to use it for CI, but it should have plenty of spare resources.

[ndossche]

I have a preliminary patch that makes all tests pass and fixes support for bool, char, etc as well on big endian: https://gist.github.com/nielsdos/b9b5cae93caa5c485e9b259dc3a43600
It's incomplete as I still need to fully fix casting support and support for cdata assignment (preferably in a generic way)
I'll continue tomorrow or so

[fds242]

I just noticed this issue myself, and was about to file another dupe bug report; glad I searched (I see there were plenty of other mentions of this issue even beyond this). What a coincidence it's being worked on right now after all this time. :) Thank you!!

The patch looks a little complicated but I failed to notice there were issues with input parameters as well -- must have got lucky with the parameter types or values of my calls. I should look into it more.

On the return value issue, I patched zend_ffi_cdata_to_zval() as follows, adding the following before the pre-existing switch:

		if (is_ret && type->size <= sizeof(ffi_arg)) {
			switch (kind) {
				case ZEND_FFI_TYPE_SINT8:
				case ZEND_FFI_TYPE_SINT16:
				case ZEND_FFI_TYPE_SINT32:
				case ZEND_FFI_TYPE_SINT64:
					ZVAL_LONG(rv, *(ffi_sarg*)ptr);
					return;
				case ZEND_FFI_TYPE_CHAR:
				case ZEND_FFI_TYPE_BOOL:
				case ZEND_FFI_TYPE_UINT8:
				case ZEND_FFI_TYPE_UINT16:
				case ZEND_FFI_TYPE_UINT32:
				case ZEND_FFI_TYPE_UINT64:
					ZVAL_LONG(rv, *(ffi_arg*)ptr);
					return;
				default:
					break;
			}
		}

Which seemed to be sufficient to make everything function correctly in my PHP scripts in practice.