Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ffi] Add support for abi-specific integer sizes #42563

Closed
mkustermann opened this issue Jul 2, 2020 · 18 comments
Closed

[ffi] Add support for abi-specific integer sizes #42563

mkustermann opened this issue Jul 2, 2020 · 18 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@mkustermann
Copy link
Member

We should consider exposing our Abi enum and allow users to have ABI-specific types. This would avoid us having to continuously increase the set of types we support in the core dart:ffi library. It would also allow a ffi bindings generator to produce ABI agnostic bindings.

We would support the primitive integer, double and pointer types in dart:ffi anything else can be user defined.

One option would be using non-function type aliases (see dart-lang/language#65), which could look like this:

typedef WChar = AbiSpecificType<Int8, Int16, ...>;

class MyStruct extends Struct {
  @WChar()
  int char;
}

void foo(Pointer<WChar> chars);

where dart:ffi would have

class AbiSpecificType<ABI1, ABI2, ...> {}

When the VM compiles code it will know which ABI it runs on and can use the corresponding type from the instantiated AbiSpecificType.

One downside here is that we would have a hard time adding more ABIs in the future (though in reality we rarely add new architectures / operating systems / compilers)

/cc @dcharkes wdyt?

@mkustermann mkustermann added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Jul 2, 2020
@mkustermann
Copy link
Member Author

@dcharkes There is actually a way to make this more extensible to future ABIs:

abstract class WChar implements LinuxArmHardFp<Int8>,
                                LinuxArmSoftFp<Int16>,
                                WindowsX64FastCallAbi<Int32>,
                                ... {}

Adding a new Abi would then just add a new class to dart:ffi which is not breaking compatibility. When running the program on an Abi that WChar was not implemented the VM could throw an exception (or a compile-time error).

@dcharkes
Copy link
Contributor

I like it! We might still need to make it extend AbiSpecificInteger or make all those individual ABIs do that, in order to be able to have Pointer<WChar> and be able to store/load from it through extension methods.

This also means we would not have to wait on generalized typedefs (or result to subtyping) to be able to reuse the type in multiple places.

@mkustermann
Copy link
Member Author

There's also a design choice: In C code two typedefs are compatible and assignable to each other. If we do the generalized typedef approach we will have the same behavior, if we go with a separate class then one needs to explicitly cast in Dart (but not in C).

@dcharkes
Copy link
Contributor

True, if the one package decides to call wchar_t WChar and another package decides on WCharT they wont match.

Maybe we should preempt that by adding some of those to package:ffi, and have people make PRs for adding more common types.

@dcharkes dcharkes changed the title Add support for abi-specific types Add support for abi-specific integer sizes Jul 23, 2020
@dcharkes dcharkes changed the title Add support for abi-specific integer sizes [ffi] Add support for abi-specific integer sizes Jul 23, 2020
@dcharkes
Copy link
Contributor

We also need something for structs. We cannot merge the two concepts in one Dart type, because the int loads/stores only work for ABI-sized integers, and the struct field loads/stores only work for structs.

@mkustermann
Copy link
Member Author

Not sure how common it is in C to have a typedef to be integer typed on one platform and non-integer on another. Maybe that is not worth adding support for.

@artob
Copy link

artob commented Jul 23, 2020

abstract class WChar implements LinuxArmHardFp<Int8>,
                                LinuxArmSoftFp<Int16>,
                                WindowsX64FastCallAbi<Int32>,
                                ... {}

Adding a new Abi would then just add a new class to dart:ffi which is not breaking compatibility. When running the program on an Abi that WChar was not implemented the VM could throw an exception (or a compile-time error).

That looks pretty great.

True, if the one package decides to call wchar_t WChar and another package decides on WCharT they wont match.

Maybe we should preempt that by adding some of those to package:ffi, and have people make PRs for adding more common types.

Yes, these really should be provided by the Dart project in one form or another. You'd want to avoid the situation with e.g. Java JNA when there's a bunch of different definitions of the same common types (e.g., ssize_t), and nobody knows which type they ought to be using.

@dcharkes
Copy link
Contributor

@mkustermann The typedef solution does not work for struct fields (annotation constructor calls cannot have type arguments), so that solution also requires subtyping.

I've prototyped three API variants in https://gist.github.com/dcharkes/8d41b7def9bf82c74bf0b7f8a1d66a6c.

@lrhn @eernstg Any feedback from a language design point of view on the possible APIs?

@mraleph
Copy link
Member

mraleph commented Sep 8, 2020

Just to make sure we are considering this. Original FFI Vision document featured "dictionary" based portability design. I think we should consider going the same way instead of trying to use types to express things which are not types (this makes it somewhat C++esque). More concretely we could use something along the lines of:

enum Os { Linux, Windows, MacOS }
enum Cpu { X64, IA32, ARM32, ARM64 }
enum FP { Soft, Hard }

class IfAbi {
  final Os os;
  final Cpu cpu;
  final FP fp;
  const IfAbi({this.os, this.cpu, this.fp});
}

class Typedef {
  const Typedef(Map<IfAbi, Type> mapping, {Type default_});
}

@Typedef({
  IfAbi(os: Os.Linux, cpu: Cpu.X64): Int64,
  IfAbi(os: Os.Windows, cpu: Cpu.X64): Int32,
}, default_: Int32)
abstract class WChar {}

Similar approach can be taken to ABI specific structure layouts.

@lrhn
Copy link
Member

lrhn commented Sep 8, 2020

I like @mraleph's approach, but I'm wary about using enums for something which isn't naturally bounded. There are more than three operatings systems, and more than four CPUs. We might not support them yet, but baking the list into our libraries makes it harder to expand for third-party users. If they make the SDK compile on FreeBSD on PowerPC, they would still have to edit all the right places to make it recognizable. (Does X32 count as a CPU?)

So, I think I'd rather use strings. Maybe skip the IfAbi class too, and allow free-form patterns:

@Typedef(Int32, {
   {os: "linux", cpu: "x64"}: Int64,
   {os: "windows", cpu: "x64"}: Int64,
   {os: "bsd", cpu: "power" : Int64
})
...

Then there'd be some way to figure out the current configuration, and perhaps even override it.
Maybe allow -Dffi.os=windows to override the os detection for testing.

(On the other hand, I don't want to introduce a new micro-language for specifying constraints like "osVersion": "<10", which is where this could end if taken too far).

@mkustermann
Copy link
Member Author

Annotations work, yes. Not sure why I wasn't thinking of them.

One downside is that one cannot enforce the constraint that all of the possible native types for the typedef should be _NativeIntegers. In case of WChar we want to ensure it is an integer type, so we can write

class Foo extends ffi.Struct {
  @WChar
  int character;
}

I guess if we wanted to surface this we would need to have the special analyzer + CFE logic to issue additional compile-time errors based on examining the evaluated typedef constant annotation.

There's also the question whether we want to support mixed type definitions (e.g. typedefs where it's a pointer on one OS and integer on OS). The types in Dart would then be dynamic I assume and the program has to test abi at runtime and do a dynamic cast (In C code accessing such fields would be if/def'ed out)

@mraleph
Copy link
Member

mraleph commented Sep 8, 2020

One downside is that one cannot enforce the constraint that all of the possible native types for the typedef should be _NativeIntegers.

It is true that we would not be able to enforce this requirement with just through normal Dart typesystem.

But in any approach there would need to be some logic somewhere actually evaluating what WChar stands for - that logic can do enforcement of type consistency.

This unfortunately means we would need analyzer changes - which maybe otherwise would not be required.

I also realised that

class Foo extends ffi.Struct {
  @WChar
  int character;
}

would not actually work out of the box in either approaches. You need to do:

@Typedef({...})
class WChar {
  const WChar();
}

@WChar()
int character;

Which is a bit of mouthful - but I think the best we can get now. If only we could use annotations on function type parameters, then we could just go for:

const WChar = Typedef({...});

But this approach sadly does not work with our approach to the NativeFunction<...> type.

@lrhn

So, I think I'd rather use strings. Maybe skip the IfAbi class too, and allow free-form patterns:

My original design was using simple language for conditions - but I agree that that would be too much. The idea behind using IfAbi was to make it self documenting. Using maps as conditions is an interesting variation, though one would have to write:

// Keys need to be strings
{'os': "linux", 'cpu': "x64"}: Int64

// Alternatively we could have markers
enum AbiKey { os, cpu, fp }
{os: "linux", cpu: "x64" } // Map<AbiKey, String>

@dcharkes
Copy link
Contributor

dcharkes commented Sep 8, 2020

Abi-specific types use

Note that the ABI-specific types also need to extend NativeType in order to use them in function signatures, and that ABI-specific integer types need to extend AbiSpecificInteger in order to be able to use them in loads and stores.

extension AbiSpecificIntegerPointer on Pointer<AbiSpecificInteger> {
  int get value => 0;
  void set value(int value) {}
  int operator [](int index) => 0;
  void operator []=(int index, int value) {}
}

In short, the user-defined types and consts need to work in:

Whether the definition per architecture is defined as type arguments, const fields, or annotations does not matter too much for that functionality.

Abi-specific types definition

Using types, const fields, or annotation matters for how much manual checking (with manual error messages) we want to add.

It also matters for whether we have an open-world vs closed-world assumption, and whether we require the user defining the type to define it for all known Dart target ABIs. I see three options here:

  • Closed-world, user has to define the type for all target ABIs:
    • We know all types are provided. (Though, the user might fill in garbage for ABIs he does not know about, causing segfaults.)
    • We need to do a breaking change when we add a target ABI. Or as @lrhn mentioned when someone else targets Dart to a new ABI.
    • We can enforce this without extra rules in the type system.
  • Closed-world, user can define types for a subset of target ABIs:
    • Trying to run your FFI code on a different target ABI might cause compile errors because of missing typedefs.
    • We error on target ABIs we don't know about, preventing typos in the definition. (Using enums or types helps here.)
    • Anyone extending Dart to a new target ABI would need to extend the closed-world.
  • Open-world, user can define types for a subset of target ABIs:
    • Same, but we don't error on unknown ABIs.
    • This is fragile, a mistype of target ABI that the user is not testing on will go unnoticed.

@ndusart
Copy link

ndusart commented Jul 16, 2021

Hello.

I am interested to know if there is any work going on this ?
This is a big issue currently for binding to a C API using types with different sizes regarding the target architecture.

Thanks

@cmc5788
Copy link

cmc5788 commented Oct 28, 2021

@ndusart agreed - I added a brief comment to dart-lang/native#530 ; for our use cases at Toyota right now, distributing platform-specific ffigen code is not a hard blocker, but quite a strain on our workflow.

copybara-service bot pushed a commit that referenced this issue Nov 3, 2021
This CL expands the ABIs in the FFI transform and VM from 3 groups to
18 individual ABIs in preparation of adding ABI-specific integer sizes
and structs.

This increases const lists with offsets in the kernel representation
from 3 to 18 elements. We do not expect significant size increases or
performance regressions from this. The constants are deduplicated in
Dart and the optimizer should optimize indexed lookups in const lists
away. However, landing this separately will enable us to verify that
assumption.

This also moves `Abi` to its own file and makes it an opaque class with
a predefined set of instances so that we do not depend on its internals.
That will enable us to keep `pkg/vm/lib/transformations/ffi/abi.dart`
consistent with the `Abi` abstraction to be introduced in `dart:ffi`
later for specifying ABI-specific integer sizes.

Bug: #42563
Bug: #42816

List of ABIs decided based on what's currently used (as Dart SDK target
platform, or Flutter target platform, or G3 target) and added
windows_arm64 in anticipation of
(flutter/flutter#53120).
Excluded are macos_ia32 (#39810)
because we discontinued support; and windows_arm, fuchsia_arm,
fuchsia_ia32, ios_ia32, and macos_arm because these are
unlikely to be added.

TEST=pkg/front_end/testcases/*.expect
TEST=tests/ffi/*

Change-Id: I437707c18d8667490c063272a5f8a33d846e6061
Cq-Include-Trybots: luci.dart.try:vm-kernel-linux-debug-x64-try,vm-ffi-android-debug-arm-try,vm-kernel-mac-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-ffi-android-debug-arm64c-try,vm-kernel-mac-release-arm64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-precomp-android-release-arm64c-try,vm-kernel-precomp-android-release-arm_x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/217184
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>
Reviewed-by: Clement Skau <[email protected]>
@dcharkes
Copy link
Contributor

Offline discussion with @mkustermann earlier: We should go for open world, to avoid breaking changes when ABIs are added.

As for an API, I've prototyped const objects as keys in the mapping used in annotations (not types or strings as suggested earlier in the thread). @lrhn wdyt about the API below?

In dart:ffi:

/// Application binary interface.
///
/// The Dart VM can run on a variety of [Abi]s.
class Abi {
  /// The operating system of this [Abi].
  final _OS _os;

  /// The architecture of this [Abi].
  final _Architecture _architecture;

  external factory Abi.current();

  const Abi._(this._architecture, this._os);

  @override
  String toString() => '${_os.name}_${_architecture.name}';
}

const androidArm = Abi._(_Architecture.arm, _OS.android);
const androidArm64 = Abi._(_Architecture.arm64, _OS.android);
const androidIA32 = Abi._(_Architecture.ia32, _OS.android);
// ...

/// The supertype of all [Abi]-specific integer types.
///
/// [Abi]-specific integers should extend this class and annotate it with
/// [AbiSpecificIntegerMapping] to declare the integer size and signedness
/// for all [supportedAbisOrdered].
///
/// For example:
///
/// ```
/// /// Represents a `uintptr_t` in C.
/// ///
/// /// [UintPtr] is not constructible in the Dart code and serves purely as
/// /// marker in type signatures.
/// @AbiSpecificIntegerMapping({
///   androidArm: Uint32(),
///   androidArm64: Uint64(),
///   androidIA32: Uint32(),
///   androidX64: Uint64(),
///   fuchsiaArm64: Uint32(),
///   fuchsiaX64: Uint64(),
///   iosArm: Uint32(),
///   iosArm64: Uint64(),
///   linuxArm: Uint32(),
///   linuxArm64: Uint64(),
///   linuxIA32: Uint32(),
///   linuxX64: Uint64(),
///   macosArm64: Uint64(),
///   macosX64: Uint64(),
///   windowsIA32: Uint32(),
///   windowsX64: Uint64(),
/// })
/// class UintPtr extends AbiSpecificInteger {
///   const UintPtr();
/// }
/// ```
class AbiSpecificInteger extends NativeType {
  const AbiSpecificInteger();
}

/// Mapping for a subtype of [AbiSpecificInteger].
///
/// See documentation on [AbiSpecificInteger].
class AbiSpecificIntegerMapping {
  final Map<Abi, NativeType> mapping;

  const AbiSpecificIntegerMapping(this.mapping);
}

User code:

/// Represents a native unsigned pointer-sized integer in C.
///
/// [UintPtr] is not constructible in the Dart code and serves purely as marker in
/// type signatures.
@AbiSpecificIntegerMapping({
  androidArm: Uint32(),
  androidArm64: Uint64(),
  androidIA32: Uint32(),
  androidX64: Uint64(),
  fuchsiaArm64: Uint32(),
  fuchsiaX64: Uint64(),
  iosArm: Uint32(),
  iosArm64: Uint64(),
  iosX64: Uint64(),
  linuxArm: Uint32(),
  linuxArm64: Uint64(),
  linuxIA32: Uint32(),
  linuxX64: Uint64(),
  macosArm64: Uint64(),
  macosX64: Uint64(),
  windowsArm64: Uint64(),
  windowsIA32: Uint32(),
  windowsX64: Uint64(),
})
class UintPtr extends AbiSpecificInteger {
  const UintPtr();
}

@lrhn
Copy link
Member

lrhn commented Nov 29, 2021

That looks horribly verbose for the user. Will package:ffi have this UintPtr declaration, or will users have to write it themselves?

You say that the ABI should be open, but it should match supportedAbisOrdered? Is that "open" in some sense, or is it simply all combinations of architecture and OS (themselves closed enums?) that the dart:ffi library supports?
(If so, if someone ports Dart to another architecture/OS and extends supportedAbisOrdered, it won't help people who have already written the large map annotation above.)

Could the Abi class contain a "default integer size" as well, so:

const androidArm = Abi._(_Architecture.arm, _OS.android, _IntegerSize.bits32);
const androidArm64 = Abi._(_Architecture.arm64, _OS.android, _IntegerSize.bits64);
...
const fucsiaArm64 = Abi._(_Architecture.arm64, _OS.fucsia, IntegerSize.bits32);
...

and you only have to explicitly write a type for AbiSpecificIntegerMapping when the size differs from the default?

@dcharkes
Copy link
Contributor

dcharkes commented Nov 29, 2021

Thanks for your input @lrhn!

Some notes from our discussion:

  • We introduce an Abi concept that we can reuse. We start with an opaque one with the 18 const instances. This way it works a bit like an enum.
    • We use a Map<Abi, NativeInteger> rather than 18 named parameters for AbiSpecificIntegerMapping. The downsides are extra { and } on the use sites, and no editor completion. However, the upsides are that the keys refer to the Abi concept, and that if we ever add Abis it is in one place instead of many. (Also, users will likely copy-paste the example in the comment, which contains all the keys of the map.)
    • Update: in order to avoid polluting the global namespace the const instances are static consts in the class.
  • Verbosity. This approach is verbose. But we expect to land 5-10 ones in package:ffi and have users maybe write one or two in their own code.
    • We dismissed the idea of a default value and only specifying the Abis deviating from that, because if the default is wrong, end users will get segmentation faults instead of useful exceptions that a mapping is missing.
  • Open-world: We do not error out on missing Abis at compile time, but throw at runtime when running on said Abi. That way adding new Abis is not a breaking change.
    • We dismissed using strings as keys instead of const instances. That would allow users to add mappings for Abis that are not part of dart:ffi yet. But it would also lead to typo's not being spotted until users using it on an Abi that is then all of a sudden missing.
  • const supportedAbisOrdered -> static const Abi.values.

Let me know if I missed anything.

copybara-service bot pushed a commit that referenced this issue Dec 1, 2021
This introduces the application binary interface (ABI) concept to
`dart:ffi` as a class with opaque instances.

We will use this for providing mappings for ABI-specific types.
Bug: #42563
Bug: #42816
Closes: #45254

Later, we might open up the contents of `Abi` if need be.

Some API design discussion notes:
#42563 (comment)

TEST=tests/ffi/abi_test.dart

Change-Id: Iebf290fc12f37d6f4236432ddf27ab3e12bde06d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/221627
Reviewed-by: Ryan Macnak <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
copybara-service bot pushed a commit that referenced this issue Dec 2, 2021
When ABI-specific integers are introduced, their mappings can be
partial. We need to account for this in the transformation and the code
we generate.

In the transformation, all sizes and offsets become nullable.
In the generated code we add `null` constants and a call to check
whether the value is not-null at runtime.

Note that with only this CL we can not generate nulls yet, because all
size and offset mappings are still complete.

TEST=pkg/front_end/testcases/nnbd/ffi*
TEST=tests/ffi*

Bug: #42563

Change-Id: I80d45f3f52001670bc0679a033f7daa22198d55e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/221631
Reviewed-by: Ryan Macnak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

7 participants