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

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

Hi, Sergey!
Thanks for the review!
Fixed your comment and force-pushed the branch.

On 01.06.26, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! Please see my comments.
> 
> Sergey
> 
> On 5/30/26 19:04, Sergey Kaplun wrote:
> > From: Mike Pall <mike>

<snipped>

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

It just early return as for condition below.

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

The n==3 is already checked in our tests. Do we need to test 5
specifically here? There are the similar tests for n > 4 in the next
commits as well.

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

Added the test for unions.

===================================================================
diff --git a/test/tarantool-tests/ffi-ccall/libfficcall.c b/test/tarantool-tests/ffi-ccall/libfficcall.c
index 60d30f3c..2145b556 100644
--- a/test/tarantool-tests/ffi-ccall/libfficcall.c
+++ b/test/tarantool-tests/ffi-ccall/libfficcall.c
@@ -94,6 +94,10 @@ typedef struct hfa_float2 {
 	float v[2];
 } hfa_float2;
 
+typedef union uhfa_float2 {
+	float v[2];
+} uhfa_float2;
+
 typedef struct hfa_float22 {
 	float v[2][2];
 } hfa_float22;
@@ -119,6 +123,11 @@ float hfa_float2_sum(hfa_float2 h)
 	return h.v[0] + h.v[1];
 }
 
+float uhfa_float2_sum(uhfa_float2 h)
+{
+	return h.v[0] + h.v[1];
+}
+
 float hfa_float22_sum(hfa_float22 h)
 {
 	return h.v[0][0] + h.v[0][1] + h.v[1][0] + h.v[1][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
index bb500de1..b67533cf 100644
--- 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
@@ -6,7 +6,7 @@ local tap = require('tap')
 -- See also: https://github.com/LuaJIT/LuaJIT/issues/1357.
 local test = tap.test('lj-1357-arm64-struct-array-pass-by-val')
 
-test:plan(1)
+test:plan(2)
 
 local ffi_ccall = ffi.load('libfficcall')
 
@@ -16,8 +16,15 @@ ffi.cdef[[
   } hfa_float2;
 
   float hfa_float2_sum(hfa_float2 h);
+
+  typedef union uhfa_float2 {
+    float v[2];
+  } uhfa_float2;
+
+  float uhfa_float2_sum(uhfa_float2 h);
 ]]
 
 test:is(ffi_ccall.hfa_float2_sum({{1, 2}}), 3, 'HFA float correct')
+test:is(ffi_ccall.uhfa_float2_sum({{1, 2}}), 3, 'union HFA float correct')
 
 test:done(true)
===================================================================

> 
> 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;

This is weird, since it changes non-union behaviour.

>         } else if (ctype_iscomplex(sct->info)) {
>          r |= (sct->size >> 1);
>          if (!isu) n += 2; else if (n < 2) n = 2;
> 

<snipped>

-- 
Best regards,
Sergey Kaplun