Skip to content

dart format support for records #50798

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

Closed
dcharkes opened this issue Dec 20, 2022 · 1 comment
Closed

dart format support for records #50798

dcharkes opened this issue Dec 20, 2022 · 1 comment
Assignees
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. closed-duplicate Closed in favor of an existing report dart-cli-format Issues related to the 'dart format' tool

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Dec 20, 2022

$ dart analyze --enable-experiment=records record.dart
Analyzing record.dart...               0.4s
No issues found!

$ dart format --enable-experiment=records record.dart
Could not find an option named "enable-experiment".

$ dart --enable-experiment=records format record.dart
# and
$ dart format record.dart
# choke on record types.

Snippet that causes issues:

final returnStruct3BytesHomogeneousUint8VariadicLeaf =
    ffiTestFunctions.lookupFunction<
        Struct3BytesHomogeneousUint8 Function(Uint8, VarArgs<(Uint8, Uint8)>),
        Struct3BytesHomogeneousUint8 Function(int, int,
              int)>("ReturnStruct3BytesHomogeneousUint8Variadic", isLeaf: true);

@munificent

@dcharkes dcharkes added the dart-cli-format Issues related to the 'dart format' tool label Dec 20, 2022
@lrhn lrhn added the area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. label Dec 21, 2022
@munificent
Copy link
Member

Duplicate of dart-lang/dart_style#1128.

@munificent munificent closed this as not planned Won't fix, can't repro, duplicate, stale Jan 6, 2023
@munificent munificent added the closed-duplicate Closed in favor of an existing report label Jan 6, 2023
copybara-service bot pushed a commit that referenced this issue Jan 20, 2023
This CL introduces `VarArgs` to `NativeFunction` signatures. The
`VarArgs` type takes a single type argument. This type argument is a
subtype of `NativeType` if there is a single variadic argument, and a
record with native types if there are multiple variadic arguments.
For example:
`NativeFunction<Void Function(Pointer<Char>, VarArgs<(Int32,Int32)>)>`
for calling refering to a `printf` binding with two `int32_t` arguments
passed as variadic arguments.

The logic of the native calling conventions are detailed in
https://dart-review.googlesource.com/c/sdk/+/278342.
Here we explain how this influences the FFI pipeline.

First, now that `VarArgs` is part of signatures, we have to unwrap
that when with the C types in the CFE transform and checking (analyzer
is in a separate CL), and also in the marshaller when looking up the
C type of arguments.

Second, we have to deal with `BothNativeLocations`. On windows x64,
floating point arguments must be passed both in FPU _and_ CPU
registers. For FFI calls, we solve this in the argument moves by just
copying to both locations. For FFI callbacks, we just take the FPU
register location (which avoids an extra bitcast).

Third, on System-V, we have to pass an upper bound of the number of
XMM registers used in AL. This means we instead RAX, we use R13 for the
target address. For variadic calls, we always pass 8 in AL as the valid
upper bound. We could consider passing the actual number of XMM
registers used.
We keep using RAX as default register for the function address on non-
variadic calls, because changing to R13 (the first free) register
creates more spilling in leaf calls. R13 is callee-saved while RAX is
not, so using R13 instead of RAX causes us to have to spill the value
from RAX on leaf calls.

Fourth, on both x64 and RISC-V, we pass floats in integer locations.
`EmitNativeMove` has been modified to deal with this, so that we do not
have to insert more `BitCastInstr`s.

The tests are generated by a test generator: `tests/ffi/generator/`.

The formatter doesn't support records yet, so the tests are not properly
formatted.
Bug: #50798

TEST=tests/ffi/*_varargs_*

Closes: #38578
Closes: #49460
Closes: #50858

Change-Id: I6a6296fe972527f8a54ac75a630131769e3cc540
Cq-Include-Trybots: luci.dart.try:vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-nnbd-linux-debug-ia32-try,vm-kernel-win-debug-ia32-try,vm-kernel-linux-debug-x64-try,vm-kernel-mac-debug-x64-try,vm-kernel-win-debug-x64-try,vm-kernel-nnbd-win-release-ia32-try,vm-kernel-nnbd-win-debug-x64-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-kernel-precomp-android-release-arm64c-try,vm-kernel-precomp-android-release-arm_x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-precomp-ffi-qemu-linux-release-riscv64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-precomp-asan-linux-release-x64-try,vm-kernel-msan-linux-release-x64-try,vm-kernel-precomp-msan-linux-release-x64-try,app-kernel-linux-debug-x64-try,vm-kernel-mac-release-arm64-try,vm-kernel-nnbd-mac-debug-arm64-try,vm-kernel-nnbd-mac-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/276921
Reviewed-by: Devon Carew <devoncarew@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. closed-duplicate Closed in favor of an existing report dart-cli-format Issues related to the 'dart format' tool
Projects
None yet
Development

No branches or pull requests

3 participants