feat(ext/ffi): structs by value

[DjDeveloperr]

Adds support for passing and returning structs as buffers to FFI.
Does not implement fastapi support for structs.
Needed for certain system APIs such as AppKit on macOS

[DjDeveloperr]

@littledivy PTAL

[aapoalas]

LGTM! Really nice to have the last piece of the puzzle, so to say :)

[vimirage]

Can tests be added for passing structs by value?

[aapoalas]

I do think we should actually be checking the buffer for size validity. Trying to pass a 0 size buffer as an eg. ["u32"] struct should error, I think.

[bartlomieju]

@littledivy please take a look. @DjDeveloperr could you please rebase?

[DjDeveloperr]

@littledivy @aapoalas PTAL

[aapoalas]

Left some comments. Looks mostly good to me, but I'd change a few things before landing.

[vimirage]

Can tests be added for passing structs by value?

Er, we need nested struct tests.

Furthermore, recursive structs should be rejected.

[DjDeveloperr]

Furthermore, recursive structs should be rejected.

Seems to panic when deserializing in Rust land. Should this be handled in JS side? I think it's very edge case, we don't really need a check for this.

[vimirage]

Furthermore, recursive structs should be rejected.

Seems to panic when deserializing in Rust land. Should this be handled in JS side? I think it's very edge case, we don't really need a check for this.

Yes; panicking represents a bug - panics shouldn't actually be reached.
I expected that this would panic, that's why I mentioned it.

[DjDeveloperr]

Shouldn't this be actually handled in serde_v8? Its overflowing stack when deserializing this object.

[vimirage]

Shouldn't this be actually handled in serde_v8? Its overflowing stack when deserializing this object.

Yup, that's the issue.
Alternatively, you could accept v8 values and check for cycles using that, but... this is definitely a serde_v8 issue that is waiting to be opened.

[DjDeveloperr]

Alternatively, you could accept v8 values and check for cycles using that, but... this is definitely a serde_v8 issue that is waiting to be opened.

So, do I need to implement a workaround for this bug of serde_v8 right now?

[littledivy]

How does ownership of the buffer work? Are we copying the buffer?

[DjDeveloperr]

How does ownership of the buffer work? Are we copying the buffer?

When passing buffer as struct argument, we directly pass pointer from backing store. Libffi doesn't seem to take any ownership of it.
Buffer is probably internally copied only once, like pushed onto stack at time of call or something according to ABI.

See: #15060 (comment)

When returning struct we pre allocate buffer in JS side and pass its pointer to libffi so return value can be written to it.

[littledivy]

Buffer is probably internally copied only once, like pushed onto stack at time of call or something according to ABI.

Ah ok that makes sense. libffi pushes the struct onto the stack.

What happens when structs are too big? On aarch64, pass-by-value structs large enough are actually passed by reference.

From https://devblogs.microsoft.com/oldnewthing/20220823-00/?p=107041 "The AArch64 processor (aka arm64), part 20: The classic calling convention"

If a parameter is a large structure (larger than 16 bytes), then it is passed by address. Small structures are passed by value, packed into integer registers. (Execption: If the small structure consists entirely of floats or entirely of doubles, then it is passed in floating point registers, one for each member.)

[DjDeveloperr]

What happens when structs are too big? On aarch64, pass-by-value structs large enough are actually passed by reference.

I'm not sure how libffi handles this internally, but it would be pretty weird (and problematic) if it is taking ownership of arguments only in certain cases.

[littledivy]

https://github.com/python/cpython/blob/bb8b931385ba9df4e01f7dd3ce4575d49f60efdf/Modules/_ctypes/callproc.c#L1238-L1250

#ifdef CTYPES_PASS_BY_REF_HACK
        size_t size = atypes[i]->size;
        if (IS_PASS_BY_REF(size)) {
            void *tmp = alloca(size);
            if (atypes[i]->type == FFI_TYPE_STRUCT)
                memcpy(tmp, args[i].value.p, size);
            else
                memcpy(tmp, (void*)&args[i].value, size);

            avalues[i] = tmp;
        }
        else
#endif

We need to do this workaround for large structs. Code from cpython above. libffi issue: libffi/libffi#694

[littledivy]

Oh i misunderstood the PR, the calls seems to accept the raw buffer from the user instead of a JS object. We could make this inbuilt and have JIT unwrapping for object -> buffer conversion?

call({ x: 1, y : 2 });

// call(...) codegen:
const u8 = new Uint8Array(size);
function call(obj) {
  const { x, y } = obj;
  u8[0] = x;
  u8[1] = y;
  ffi_call(u8)
}

[littledivy]

Also how is struct padding defined? Right now it is only "packed" structs IIUC

[aapoalas]

Also how is struct padding defined? Right now it is only "packed" structs IIUC

It's only C repr structs, non-packed. libffi does not give any options for structs, they're C repr always. Doing a packed struct needs to be done manually with u8s.

[aapoalas]

Oh i misunderstood the PR, the calls seems to accept the raw buffer from the user instead of a JS object. We could make this inbuilt and have JIT unwrapping for object -> buffer conversion?

call({ x: 1, y : 2 });

// call(...) codegen:
const u8 = new Uint8Array(size);
function call(obj) {
  const { x, y } = obj;
  u8[0] = x;
  u8[1] = y;
  ffi_call(u8)
}

I think this would be unnecessary / can be done in user-land fairly easily. From my libclang bindings where I'm using this branch I'm often not at all interested in the struct internals, the C API just returns structs and accepts the same structs as parameters so I'm passing the Uint8Array back and forth.

[DjDeveloperr]

Oh i misunderstood the PR, the calls seems to accept the raw buffer from the user instead of a JS object. We could make this inbuilt and have JIT unwrapping for object -> buffer conversion?

call({ x: 1, y : 2 });



// call(...) codegen:

const u8 = new Uint8Array(size);

function call(obj) {

  const { x, y } = obj;

  u8[0] = x;

  u8[1] = y;

  ffi_call(u8)

}

I don't think we should do this, at least only for passing structs. As AapoAlas stated like it is easily doable in user land. If we do however add struct serialization, it should be complete and also support deserialization (e.g for returning). It's also useful for APIs that expect structs by pointer too. But I think that's for another PR.

[littledivy]

I want to land this, let's get the CI green. @DjDeveloperr Do you need help with something?

[DjDeveloperr]

I want to land this, let's get the CI green. @DjDeveloperr Do you need help with something?

Passing structs in nonblocking calls doesn't seem to work, I couldn't find the reason though. In normal sync calls, struct is passed correctly by value but in nonblocking calls the struct value is actually a pointer to another pointer of struct's memory, which is weird because both type of calls use same function for parsing struct arguments.

After some debugging, found that when we pass the same struct buffer as argument to both sync & async calls, literally same libffi Arg structs are created (which is expected). But I don't get why at the time of call the value is becoming like that only in async calls.

Also, the windows fastci is failing for unknown reason...

EDIT: Commented out the test case for it. CI is green now but it's still a bug. Seems windows fastci had seg fault because of this (unlike on Linux where I tested, it had no segmentation fault).

[aapoalas]

LGTM.

[littledivy]

LGTM! Thank you, this is a nice feature to have.