[ffi] Support inline array of variable length

[dcharkes]

trafficstars

Right now we're disallowing inline arrays of length 0.

I believe when I added inline arrays, I read that length 0 is undefined behavior, so I disallowed length 0.

Declaring zero-length arrays is allowed in GNU C as an extension. A zero-length array can be useful as the last element of a structure that is really a header for a variable-length object

https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

_Originally posted by @jpnurmi in https://github.com/dart-lang/native/issues/450

We should double check.

[dcharkes]

And on Windows apparently variable length can be size 1 (instead of 0).

• https://github.com/dart-lang/native/issues/1200#issuecomment-2157897238

[halildurmus]

@dcharkes Is there any chance we could configure the variableLengthLength value in the @Array.variable() constructor?
https://github.com/dart-lang/sdk/blob/c50c194d419c09bdbc9267eb8ed045819fb0beed/sdk/lib/ffi/ffi.dart#L247-L260

In Win32 structs, the majority of the variable-length arrays have a length of 1, though I've come across some cases with lengths of 3 and even 6 😱. In its current state, I can't use @Array.variable() in Win32 structs exposed in package:win32 because sizeOf doesn't account for these fields.

It would be really helpful if this could be configurable to support this use case.

[dcharkes]

In its current state, I can't use @Array.variable() in Win32 structs exposed in package:win32 because sizeOf doesn't account for these fields.

You mean you cannot use it at all, or that for using Allocators and sizeOf in Dart you have to account for the extra size. Can you elaborate on what issues you're facing?

though I've come across some cases with lengths of 3 and even 6

🤣

Is there any chance we could configure the variableLengthLength value in the @Array.variable() constructor?

If you have a convincing use case, that sounds like a reasonable solution. 👍

[halildurmus]

You mean you cannot use it at all, or that for using Allocators and sizeOf in Dart you have to account for the extra size. Can you elaborate on what issues you're facing?

I'm concerned that calloc not initializing these fields and sizeOf not taking them into account, could lead to some unexpected results.

For example, some Win32 structs include a field (typically the first one) for their size and we're supposed to set this field with sizeOf.

In one example from package:win32 there's this code:

// ...
      final deviceInterfaceDetailDataPtr = calloc<BYTE>(requiredSizePtr.value)
          .cast<SP_DEVICE_INTERFACE_DETAIL_DATA_>()
        ..ref.cbSize = sizeOf<SP_DEVICE_INTERFACE_DETAIL_DATA_>();

      try {
        final hr = SetupDiGetDeviceInterfaceDetail(
            hDevInfo,
            deviceInterfaceDataPtr,
            deviceInterfaceDetailDataPtr,
            requiredSizePtr.value,
            nullptr,
            nullptr);
        if (hr != TRUE) {
          print(
              'SetupDiGetDeviceInterfaceDetail - Get Data error ${GetLastError()}');
          continue;
        }

// ...

SP_DEVICE_INTERFACE_DETAIL_DATA_ struct has a variable-length array with a length of 1. If I use @Array.variable() for the variable-length array in this struct, the call to SetupDiGetDeviceInterfaceDetail fails with "SetupDiGetDeviceInterfaceDetail - Get Data error xx" due to sizeOf not accounting for the variable-length array.

While trying out @Array.variable() in this struct, I noticed a bug and opened #56399.

[dcharkes]

I'm concerned that calloc not initializing these fields and sizeOf not taking them into account, could lead to some unexpected results.

Right, I agree that this is a gotcha. It makes your Dart code not parallel the C code. So adding the the variable length param to @Array.variable() makes sense to me. One problem with your proposed design is how to deal with @Array.variable(, because I believe that variable dimension that you want to the declare would be the very first optional argument. And we're already using optional positional arguments for the non-variable dimensions. (And combining optional positional params and named params is not supported in Dart.)

We could either:

1. Always have users specify the first (variable) dimension as 0. The downside is boilerplate.
2. Introduce a new constructor for @Array.variableSpecifyVariableDimension (name TBD) and a variableDimension optional named argument for @Array.variableMulti.

// before
final class MyStruct extends Struct {
  @Array.variable(10, 10)
  external Array<Array<Array<Uint8>>> inlineArray;

  @Array.variableMulti([2, 2])
  external Array<Array<Array<Uint8>>> threeDimensionalInlineArray;
}

// option 1
final class MyStruct extends Struct {
  @Array.variable(0, 10, 10)
  external Array<Array<Array<Uint8>>> inlineArray;

  @Array.variableMulti([0, 2, 2])
  external Array<Array<Array<Uint8>>> threeDimensionalInlineArray;
}

// option 2
final class MyStruct2 extends Struct {
  @Array.variableSpecifyVariableDimension(0, 10, 10)
  external Array<Array<Array<Uint8>>> inlineArray;

  @Array.variableMulti(variableDimision: 0, [2, 2])
  external Array<Array<Array<Uint8>>> threeDimensionalInlineArray;
}

Maybe @halildurmus @lrhn or @mkustermann have better ideas.

Once we have decided on what API to use, the fix would be to do the following:

• Take the size into account in the size calculation in pkg/vm/lib/modular/transformations/ffi/common.dart.
• And, take the size into account in the size calculation in runtime/vm/compiler/ffi/native_type.cc.
  • In order for that to work, the @pragma('vm:ffi:struct-fields') on structs needs to encode the length (instead of the 0 it's passed now). I don't believe the .cc file has to be changed, we just need to pass the right length instead of 0 from the frontend.
  • But the final class Array<T extends NativeType> extends _Compound { final int _size; which is the runtime length should stay 0 so that no bounds checks are done. sdk/lib/_internal/vm/lib/ffi_patch.dart. (Possibly a cleaner solution is to actually have the _size be whatever was declared statically, but add a bool whether it's variable length.)
• And add tests in
  • pkg/vm/testcases/transformations/ffi/native_fields.dart.expect (for the array length encodeing in 'vm:ffi:struct-fields')
  • tests/ffi/generator/structs_by_value_tests_configuration.dart to pass some structs by value with non-0 elements
  • tests/ffi/inline_array_variable_length_test.dart to test sizeOf and Allocators.

(I might not have time to work on this, so open to contributions. Some pointers can be found in https://dart-review.googlesource.com/c/sdk/+/371960.)

[halildurmus]

We could either:

1. Always have users specify the first (variable) dimension as 0. The downside is boilerplate.

2. Introduce a new constructor for `@Array.variableSpecifyVariableDimension` (name TBD) and a `variableDimension` optional named argument for `@Array.variableMulti`.

I'm leaning towards option 2. We should avoid adding unnecessary boilerplate for users. Here's one idea for the new constructor: @Array.variableWithVariableDimension:

final class MyStruct extends Struct {
  @Array.variableWithVariableDimension(1)
  external Array<Uint8> inlineArray;

  @Array.variableWithVariableDimension(1, 10)
  external Array<Array<Uint8>> twoDimensionalInlineArray;

  @Array.variableMulti(variableDimension: 6, [2, 2])
  external Array<Array<Array<Uint8>>> threeDimensionalInlineArray;
}

Once we have decided on what API to use, the fix would be to do the following: ... (I might not have time to work on this, so open to contributions. Some pointers can be found in https://dart-review.googlesource.com/c/sdk/+/371960.)

I’m ready to tackle this as soon as the API design is finalized. Thanks for the pointers! :heart:

[halildurmus]

@dcharkes I've put together a CL based on the API design outlined in https://github.com/dart-lang/sdk/issues/52366#issuecomment-2275784246 (happy to change it based on possible suggestions).

• But the final class Array<T extends NativeType> extends _Compound { final int _size; which is the runtime length should stay 0 so that no bounds checks are done. sdk/lib/_internal/vm/lib/ffi_patch.dart. (Possibly a cleaner solution is to actually have the _size be whatever was declared statically, but add a bool whether it's variable length.)

Adding a bool field to the Array class in ffi_patch.dart turned out to be trickier than I expected. It required changes in several parts of the CFE and a few tweaks in runtime/vm/compiler/ffi/native_type.cc. I think I’ve covered everything, but I’d appreciate it if you could take a look to confirm that I didn’t miss anything.

One thing to note: the CL has a pretty big diff because of the generated tests in tests/ffi/. The existing test files weren’t formatted with the tall-style formatter, so when I added new tests in tests/ffi/generator/structs_by_value_tests_configuration.dart and ran the test generator, the reformatting caused a lot of changes. I’m wondering if it might make sense to reformat those files in a separate CL first to make this one easier to review.

Looking forward to hearing your thoughts – thanks!

[dcharkes]

Awesome! ❤️

I think I’ve covered everything, but I’d appreciate it if you could take a look to confirm that I didn’t miss anything.

Let's kick off the CQ to see. 👌 I left a comment on the CL on how to ensure that all the relevant bots will be included. (But I'll need to hit the button to run the CQ.)

I’m wondering if it might make sense to reformat those files in a separate CL first to make this one easier to review.

Probably yes! @munificent I believe seeing a message from you that you are going to format everything in the Dart SDK that is Dart 3.7. Are you in the process of doing so? If no, can we kick of formatting ourselves?

Edit: Attempting to format in https://dart-review.googlesource.com/c/sdk/+/399102.

[dcharkes]

I’m wondering if it might make sense to reformat those files in a separate CL first to make this one easier to review.

Done (38e250b4d579b44b6418a30ae8fd6609e38f8824). Thanks for the suggestion! 👍 Please rebase. 🙏

[halildurmus]

Thanks! ❤️ I just rebased.