Re: [Tarantool-patches] [PATCH luajit 3/5] ARM64: Fix pass-by-value struct calling conventions.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5844 bytes --]

Hi, Sergey,

thanks for the patch! Please see my comments.

Sergey

On 5/30/26 19:04, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by AnthonyK213.
>
> (cherry picked from commit c262976486e1e007b56380b6a36bfbea5f51d470)
>
> The FFI call to the function with the pass-by-value structure containing
> the HFA arrays works incorrectly due to the `ccall_classify_struct()`
> lacking the handling of the array case.
>
> This patch adds the corresponding branch to check the single-dimentional
> array. However, the multidimensional arrays are not handled. This will
> be fixed in the next commit.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#12480
> ---
>   src/lj_ccall.c                                | 18 +++++++++++----
>   test/tarantool-tests/ffi-ccall/CMakeLists.txt |  1 +
>   test/tarantool-tests/ffi-ccall/libfficcall.c  | 12 ++++++++++
>   ...57-arm64-struct-array-pass-by-val.test.lua | 23 +++++++++++++++++++
>   4 files changed, 49 insertions(+), 5 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1357-arm64-struct-array-pass-by-val.test.lua
>
> diff --git a/src/lj_ccall.c b/src/lj_ccall.c
> index b2705de5..104c9d34 100644
> --- a/src/lj_ccall.c
> +++ b/src/lj_ccall.c
> @@ -781,17 +781,24 @@ static unsigned int ccall_classify_struct(CTState *cts, CType *ct)
>   {
>     CTSize sz = ct->size;
>     unsigned int r = 0, n = 0, isu = (ct->info & CTF_UNION);
> -  while (ct->sib) {
> +  while (ct->sib && n <= 4) {

The patch adds a condition that strictly checks a number of elements 
with the same type (n <= 4).

I would also add a test for this with the following `n`: 3/4/5.

> +    unsigned int m = 1;
>       CType *sct;
>       ct = ctype_get(cts, ct->sib);
>       if (ctype_isfield(ct->info)) {
>         sct = ctype_rawchild(cts, ct);
> +      if (ctype_isarray(sct->info)) {
> +	CType *cct = ctype_rawchild(cts, sct);
> +	if (!cct->size) continue;
> +	m = sct->size / cct->size;
> +	sct = cct;
> +      }
>         if (ctype_isfp(sct->info)) {
>   	r |= sct->size;
> -	if (!isu) n++; else if (n == 0) n = 1;
> +	if (!isu) n += m; else if (n < m) n = m;

The patch also touches a logic for unions (here and below), and it is 
desired to test it as well.

This change was not caught by our regression tests on Apple M2:

--- a/src/lj_ccall.c
+++ b/src/lj_ccall.c
@@ -742,7 +742,7 @@ static unsigned int ccall_classify_struct(CTState 
*cts, CType *ct, CType *ctf)
        sct = ctype_rawchild(cts, ct);
        if (ctype_isfp(sct->info)) {
         r |= sct->size;
-       if (!isu) n++; else if (n == 0) n = 1;
+       if (!isu) n--; else if (n == 0) n = 1;
        } else if (ctype_iscomplex(sct->info)) {
         r |= (sct->size >> 1);
         if (!isu) n += 2; else if (n < 2) n = 2;

>         } else if (ctype_iscomplex(sct->info)) {
>   	r |= (sct->size >> 1);
> -	if (!isu) n += 2; else if (n < 2) n = 2;
> +	if (!isu) n += 2*m; else if (n < 2*m) n = 2*m;
>         } else if (ctype_isstruct(sct->info)) {
>   	goto substruct;
>         } else {
> @@ -803,10 +810,11 @@ static unsigned int ccall_classify_struct(CTState *cts, CType *ct)
>         sct = ctype_rawchild(cts, ct);
>       substruct:
>         if (sct->size > 0) {
> -	unsigned int s = ccall_classify_struct(cts, sct);
> +	unsigned int s = ccall_classify_struct(cts, sct), sn;
>   	if (s <= 1) goto noth;
>   	r |= (s & 255);
> -	if (!isu) n += (s >> 8); else if (n < (s >>8)) n = (s >> 8);
> +	sn = (s >> 8) * m;
> +	if (!isu) n += sn; else if (n < sn) n = sn;
>         }
>       }
>     }
> diff --git a/test/tarantool-tests/ffi-ccall/CMakeLists.txt b/test/tarantool-tests/ffi-ccall/CMakeLists.txt
> index 27de07ac..1d004591 100644
> --- a/test/tarantool-tests/ffi-ccall/CMakeLists.txt
> +++ b/test/tarantool-tests/ffi-ccall/CMakeLists.txt
> @@ -2,6 +2,7 @@ list(APPEND tests
>     ffi-ccall-arm64-fp-convention.test.lua
>     lj-205-arm64-osx-ffi-enum-arg.test.lua
>     lj-205-arm64-osx-ffi-small-arg.test.lua
> +  lj-1357-arm64-struct-array-pass-by-val.test.lua
>   )
>   
>   BuildTestCLib(libfficcall libfficcall.c "${tests}")
> diff --git a/test/tarantool-tests/ffi-ccall/libfficcall.c b/test/tarantool-tests/ffi-ccall/libfficcall.c
> index fd2d4711..ecb21752 100644
> --- a/test/tarantool-tests/ffi-ccall/libfficcall.c
> +++ b/test/tarantool-tests/ffi-ccall/libfficcall.c
> @@ -77,3 +77,15 @@ float test_float_stack(float f1, float f2, float f3, float f4, float f5,
>   	return f1 + f2 + f3 + f4 + f5 + f6 + f7 + f8 + f9 + f10 + f11;
>   }
>   
> +/****************************************************************/
> +/*       Homogeneous Floating-Point Aggregate (HFA) argument.   */
> +/****************************************************************/
> +
> +typedef struct hfa_float2 {
> +	float v[2];
> +} hfa_float2;
> +
> +float hfa_float2_sum(hfa_float2 h)
> +{
> +	return h.v[0] + h.v[1];
> +}
> diff --git a/test/tarantool-tests/lj-1357-arm64-struct-array-pass-by-val.test.lua b/test/tarantool-tests/lj-1357-arm64-struct-array-pass-by-val.test.lua
> new file mode 100644
> index 00000000..bb500de1
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1357-arm64-struct-array-pass-by-val.test.lua
> @@ -0,0 +1,23 @@
> +local ffi = require('ffi')
> +local tap = require('tap')
> +
> +-- The test file to demonstrate incorrect FFI pass-by-value
> +-- structure with an array HFA member.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1357.
> +local test = tap.test('lj-1357-arm64-struct-array-pass-by-val')
> +
> +test:plan(1)
> +
> +local ffi_ccall = ffi.load('libfficcall')
> +
> +ffi.cdef[[
> +  typedef struct hfa_float2 {
> +    float v[2];
> +  } hfa_float2;
> +
> +  float hfa_float2_sum(hfa_float2 h);
> +]]
> +
> +test:is(ffi_ccall.hfa_float2_sum({{1, 2}}), 3, 'HFA float correct')
> +
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 6746 bytes --]