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

[vm/ffi] Support .address.cast() in leaf calls #55971

Closed
dcharkes opened this issue Jun 10, 2024 · 59 comments
Closed

[vm/ffi] Support .address.cast() in leaf calls #55971

dcharkes opened this issue Jun 10, 2024 · 59 comments
Labels
area-native-interop Used for native interop related issues, including FFI. contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) library-ffi

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Jun 10, 2024

Many functions with buffers take a void*, for example https://en.cppreference.com/w/c/io/fread.

However, using a Uint8List and .address yields a Pointer<Uint8>.

It would be useful to be able to pass uint8list.address.cast(). That would prevent having to modify the FFIgen generated function signature.

Thanks for the suggestion @brianquinlan!

For anyone willing to contribute, the implementation is in:

/// Converts a single parameter with argument for [_replaceNativeCall].
(
/// '' for non-Pointer.
/// 'P' for Pointer.
/// 'T' for TypedData.
/// 'C' for _Compound (TypedData/Pointer and offset in bytes).
/// 'E' for errors.
String methodPostFix,
DartType parameterType,
Expression argument,
) _replaceNativeCallParameterAndArgument(
VariableDeclaration parameter,
DartType parameterType,
Expression argument,
int fileOffset,
) {

The implementation should find the cast() invocation and use its receiver instead (effectively ignoring the cast expression as a no-op).

@dcharkes dcharkes added library-ffi good first issue A good starting issue for contributors (issues with this label will appear in /contribute) area-native-interop Used for native interop related issues, including FFI. labels Jun 10, 2024
@codesculpture
Copy link
Contributor

Hey @dcharkes, so we have to change the data type of address from int to Pointer<Void>, so that address always yields a Pointer<Void ? But this would introduce expression such as address.cast().address.cast().address Wdy? am not sure whether am making sense , Please feel free to correct me

@dcharkes
Copy link
Contributor Author

Hey @dcharkes, so we have to change the data type of address from int to Pointer<Void>, so that address always yields a Pointer<Void ? But this would introduce expression such as address.cast().address.cast().address Wdy? am not sure whether am making sense , Please feel free to correct me

Hi @codesculpture!

No, the .address here refers to the various extension methods on non-Pointers which return a Pointer such as:

@codesculpture
Copy link
Contributor

Hi @dcharkes , i think i don't understand the problem

It would be useful to be able to pass uint8list.address.cast(). That would prevent having to modify the FFIgen generated function signature.

can you please elaborate above with an example :)

@dcharkes
Copy link
Contributor Author

dcharkes commented Jun 26, 2024

The following function in C

size_t fread( void          *buffer, size_t size, size_t count,
              FILE          *stream );

yields the following bindings with package:ffigen:

@Native<Size Function(Pointer<Void>, Size, Size, Pointer<File>)
external int fread(Pointer<Void> buffer, int size, int count, Pointer<File> stream);

But if you want to use for buffer a someUint8List.address the type of that expression will be Pointer<Uint8> and not the required Pointer<Void>.

@codesculpture
Copy link
Contributor

Sorry again @dcharkes , thanks for explaining the problem, i understood the problem, but what is the solution you are suggesting? Thanks

@dcharkes
Copy link
Contributor Author

dcharkes commented Jul 8, 2024

The following code should work:

@Native<Size Function(Pointer<Void>, Size, Size, Pointer<Void>)>()
external int fread(
    Pointer<Void> buffer, int size, int count, Pointer<Void> stream);

void main() {
  final buffer = Uint8List.fromList([1, 2, 3]);
  fread(buffer.address.cast(), 3, 3, nullptr);
}

Currently it is rejected because .address is followed by .cast(). We should change the compiler such that it is allowed.

@codesculpture
Copy link
Contributor

@dcharkes am trying to debug the https://github.com/dart-lang/sdk/blob/05d9e5bf37a68ec75f8ffc56a65a89e0e86b5a1d/pkg%2Fvm%2Flib%2Fmodular%2Ftransformations%2Fffi%2Fuse_sites.dart, but i cannot find a way to do so, i already asked a related question in dart discord server long ago, what would be ideal solution to debug the internal dart codes (now am adding print statements, that would require re building dart, for every changes)

@dcharkes
Copy link
Contributor Author

If you're in VSCode, you can run the CFE like this in the debugger:

			{
				"name": "dart gen_kernel.dart",
				"type": "dart",
				"request": "launch",
				"program": "pkg/vm/bin/gen_kernel.dart",
				"args": [
					"--platform=${workspaceFolder}/xcodebuild/ReleaseARM64/vm_platform_strong.dill",
					"pkg/vm/testcases/transformations/ffi/address_of_struct_element.dart",
				],
				"toolArgs": [],
				"enableAsserts": true,
				"cwd": "${workspaceFolder}",
			},

@codesculpture
Copy link
Contributor

codesculpture commented Jul 29, 2024

Hey @dcharkes , are we going to allow .address.cast() only on direct typed data (Uint8List) or we gonna allow expression such access members which are from struct and unions myStruct.myList.address.cast() ?

@codesculpture
Copy link
Contributor

Also @dcharkes, one small doubt is that _replaceNativeCallParameterAndArgument replacing the address with the actual instance in the AST (internally), for ex: if i fed native(buffer.address) this would transformed into native(buffer) ?
Am i correct ?

@dcharkes
Copy link
Contributor Author

Also @dcharkes, one small doubt is that _replaceNativeCallParameterAndArgument replacing the address with the actual instance in the AST (internally), for ex: if i fed native(buffer.address) this would transformed into native(buffer) ? Am i correct ?

Yes, this is already what is happening:

if (addressOfMethodsTypedData.contains(argument.target)) {
final subExpression = argument.arguments.positional.single;
// Argument is `typedData.address`.
final typedDataType = InterfaceType(
typedDataClass,
Nullability.nonNullable,
const <DartType>[],
);
return ('T', typedDataType, subExpression);
}

The TypedData is converted to the address that points into into the the typed data in the FFI call in the VM:

if (marshaller_.IsTypedDataPointer(arg_index) ||
marshaller_.IsCompoundPointer(arg_index)) {
// Unwrap typed data before move to native location.
__ Comment("Load typed data base address");
if (origin.IsStackSlot()) {
compiler->EmitMove(Location::RegisterLocation(temp0), origin,
&temp_alloc);
origin = Location::RegisterLocation(temp0);
}
ASSERT(origin.IsRegister());
__ LoadFromSlot(origin.reg(), origin.reg(), Slot::PointerBase_data());

@dcharkes
Copy link
Contributor Author

Hey @dcharkes , are we going to allow .address.cast() only on direct typed data (Uint8List) or we gonna allow expression such access members which are from struct and unions myStruct.myList.address.cast() ?

This should be allowable as well. 👍

codesculpture added a commit to codesculpture/sdk that referenced this issue Jul 31, 2024
@codesculpture
Copy link
Contributor

#56357 @dcharkes , consider this as a WIP and please review if this is correct approach.

@codesculpture
Copy link
Contributor

And also i have no idea about whether to change fileOffset while making the recursive call for subexpression of cast.

does fileOffset is line number or character's position in the file?

@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 2, 2024

Yes, that's the right approach!

fileOffset can probably just stay the same.

@codesculpture
Copy link
Contributor

@dcharkes , where can i write test for this?

@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 2, 2024

@dcharkes , where can i write test for this?

You can take a look at the original PR: https://dart-review.googlesource.com/c/sdk/+/360882

  • pkg/vm/testcases/transformations/ffi/* for a test that checks that the transform does what you expect.
  • tests/ffi/address_of_*.dart for tests that run the Dart code
  • tests/ffi/static_checks/vmspecific_static_checks_address_of_test.dart for tests that check that the analyzer and cfe return errors on the expected situations (doing a cast to the wrong type argument for example).

You'll probably also need to change the analyzer to allow the cast:

  • pkg/analyzer/lib/src/generated/ffi_verifier.dart

@codesculpture
Copy link
Contributor

Hey @dcharkes , can you explain how "--platform" argument is making possible debugging, the reason why am asking is, i need to debug the ffi_verifier.dart but i am not sure how to do it. Also if possible please elaborate how i can debug the dart files generally, if there is any references please send. It would be super helpful.
Thanks

@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 2, 2024

If you're using vscode:

			{
				"name": "dart analyzer.dart",
				"type": "dart",
				"request": "launch",
				"program": "pkg/analyzer_cli/bin/analyzer.dart",
				"args": [
					"tests/ffi/static_checks/vmspecific_static_checks_array_test.dart",
				],
				"toolArgs": [],
				"enableAsserts": true,
				"cwd": "${workspaceFolder}",
			},

For the transformation tests:

			{
				"name": "dart pkg/vm/test/transformations/ffi_test.dart",
				"type": "dart",
				"request": "launch",
				"program": "pkg/vm/test/transformations/ffi_test.dart",
				"args": [
					"compound_copies",
				],
				"toolArgs": [
					"-DupdateExpectations=true",
				],
				"enableAsserts": true,
				"cwd": "${workspaceFolder}",
			},

@codesculpture
Copy link
Contributor

codesculpture commented Aug 2, 2024

https://discord.com/channels/608014603317936148/608022273152122881/1268987825081028638

Please help if u can @dcharkes

Thanks

Edit: mraleph helped me.

@codesculpture
Copy link
Contributor

codesculpture commented Aug 3, 2024

void main() {
  final myStruct = Struct.create<MyStruct>();
  myNative(
    myStruct.a.address,
    myStruct.b.address, // It expected to return Pointer<Int>, but it returning Pointer<Never>
  );
}

@Native<
    Void Function(
      Pointer<Int8>,
      Pointer<Void>,
    )>(isLeaf: true)
external void myNative(
  Pointer<Int8> pointer,
  Pointer<Void> pointer2,
);

final class MyStruct extends Struct {
  @Int8()
  external int a;
  @Int8()
  external int b;
}

Does address of any members from Struct is always Pointer<Never> regardless of their type ? @dcharkes

I expected analyzer throw error for above program, but it dint thrown any error. That made me to ask this question

@codesculpture
Copy link
Contributor

Seems members of Union and Struct always yields a Pointer<Never>

@codesculpture
Copy link
Contributor

codesculpture commented Aug 4, 2024

@dcharkes , Here tests/ffi does this(allowing cast on address) change require to generate tests from generators or can be written manually ?

@codesculpture
Copy link
Contributor

codesculpture commented Aug 4, 2024

Also is the tests in tests/ffi/static_checks are generated or manually ? It seems generated but i cannot find generators for those @dcharkes

@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 5, 2024

void main() {
  final myStruct = Struct.create<MyStruct>();
  myNative(
    myStruct.a.address,
    myStruct.b.address, // It expected to return Pointer<Int>, but it returning Pointer<Never>
  );
}

@Native<
    Void Function(
      Pointer<Int8>,
      Pointer<Void>,
    )>(isLeaf: true)
external void myNative(
  Pointer<Int8> pointer,
  Pointer<Void> pointer2,
);

final class MyStruct extends Struct {
  @Int8()
  external int a;
  @Int8()
  external int b;
}

Does address of any members from Struct is always Pointer<Never> regardless of their type ? @dcharkes

I expected analyzer throw error for above program, but it dint thrown any error. That made me to ask this question

It's a Pointer<Never> at runtime (dynamic type), which is a valid subtype of the static type: Pointer<Int8>. (Never is a valid subtype of everything.)

The .cast<T>() returns an object with with a static type Pointer<T> but at runtime also Pointer<Never>.

@dcharkes , Here tests/ffi does this(allowing cast on address) change require to generate tests from generators or can be written manually ?

You can probably write the handful of tests manually. The test generators are set up to produce correct types. 😄 Just make sure to cover some obvious different cases:

  • A cast of a struct field
  • A cast of a typed data
  • A cast of a Pointer

Also is the tests in tests/ffi/static_checks are generated or manually ? It seems generated but i cannot find generators for those @dcharkes

No, those are written by hand. The old fashioned way!

In general, I only write a test generator if I have many tests in exactly the same structure but with one thing varying. (E.g. the FFI call tests all pass complex parameters and sum all the individual ints and then return it. And the one thing varying is what the parameter list is.)

@codesculpture
Copy link
Contributor

But the tests in tests/ffi/static_checks contains the error annotations in code as comments, i would be surprised if those errors were written manually. And does those tests actually evaluated against those errors which are in form of comments ? @dcharkes

@codesculpture
Copy link
Contributor

codesculpture commented Aug 9, 2024

Hi @dcharkes, while i implementing the tests, i found this #56425
I can provide more information if needed

Edit: Nvm it isn't a bug, closed it

@codesculpture
Copy link
Contributor

codesculpture commented Aug 10, 2024

$ tools/build.py -mrelease runtime create_platform_sdk && tools/test.py -mrelease -cfasta tests/ffi/static_checks/address_of_cast.dart

I created a new file called address_of_cast.dart in tests/ffi/static_checks
When i run the above i get this output, which seems the test dint executed,

Generating Visual Studio projects took 149ms
Done. Made 465 targets from 123 files in 11035ms
buildtools/ninja/ninja -C out\ReleaseX64 runtime create_platform_sdk
ninja: Entering directory `out\ReleaseX64'
ninja: no work to do.
The build took 11.274 seconds
No build targets found.
Test configuration:
    custom-configuration-1(architecture: x64, compiler: fasta, mode: release, runtime: none, system: win)
Suites tested: ffi

=== All 0 tests passed ===

Any idea on this @dcharkes

Edit: My bad i dint added _test as the suffix for the file name sorry.

@codesculpture
Copy link
Contributor

codesculpture commented Aug 10, 2024

@dcharkes , I pushed the tests kindly review and seems you need to press some button to run the tests again ?

@dcharkes
Copy link
Contributor Author

@dcharkes , I pushed the tests kindly review and seems you need to press some button to run the tests again ?

Yep, I'll happily hit that button. Just ping me. Also left another round of review comments! We've got some missing test cases but I think we're almost there! 👍

@codesculpture
Copy link
Contributor

@dcharkes , I pushed the tests kindly review and seems you need to press some button to run the tests again ?

Yep, I'll happily hit that button. Just ping me. Also left another round of review comments! We've got some missing test cases but I think we're almost there! 👍

Yup, on it. Thanks!

@codesculpture
Copy link
Contributor

Hey @dcharkes,

There is 2 errors being thrown for this snippet

@Native<Void Function(Pointer<Void>)>(isLeaf: true)
external void myNative(Pointer<Void> buffer);

void main() {
  final typedData = Int8List.fromList([1, 2]);
  myNative(typedData.address);
}

Output of dart <thisfile.dart>:

tests/ffi/static_checks/address_of_cast_test.dart:50:22: Error: The argument type 'Pointer<Int8>' can't be assigned to the parameter type 'Pointer<Void>'.
 - 'Pointer' is from 'dart:ffi'.
 - 'Int8' is from 'dart:ffi'.
 - 'Void' is from 'dart:ffi'.
  myNative(typedData.address);
                     ^
tests/ffi/static_checks/address_of_cast_test.dart:50:22: Error: The '.address' expression can only be used as argument to a leaf native external call.
  myNative(typedData.address);

The second error is not supposed to be thrown, and its not reported by analyzer (confirmed that by running dart analyze and got only one error which is the first one)

I tested this on 3.5.0-292.0.dev, seems this is a existing behavior.

@codesculpture
Copy link
Contributor

codesculpture commented Aug 12, 2024

I resolved all comments except this https://dart-review.googlesource.com/c/sdk/+/378221/comment/0a497574_518e78f1/

Need your attention on the above comment.

@dcharkes
Copy link
Contributor Author

Hey @dcharkes,

There is 2 errors being thrown for this snippet

@Native<Void Function(Pointer<Void>)>(isLeaf: true)
external void myNative(Pointer<Void> buffer);

void main() {
  final typedData = Int8List.fromList([1, 2]);
  myNative(typedData.address);
}

Output of dart <thisfile.dart>:

tests/ffi/static_checks/address_of_cast_test.dart:50:22: Error: The argument type 'Pointer<Int8>' can't be assigned to the parameter type 'Pointer<Void>'.
 - 'Pointer' is from 'dart:ffi'.
 - 'Int8' is from 'dart:ffi'.
 - 'Void' is from 'dart:ffi'.
  myNative(typedData.address);
                     ^
tests/ffi/static_checks/address_of_cast_test.dart:50:22: Error: The '.address' expression can only be used as argument to a leaf native external call.
  myNative(typedData.address);

The second error is not supposed to be thrown, and its not reported by analyzer (confirmed that by running dart analyze and got only one error which is the first one)

I tested this on 3.5.0-292.0.dev, seems this is a existing behavior.

I've filed #56462. Just expect both errors for now and add a comment that refers to the bug. We can fix that in another PR.

@dcharkes dcharkes added contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) and removed good first issue A good starting issue for contributors (issues with this label will appear in /contribute) labels Aug 13, 2024
@codesculpture
Copy link
Contributor

Hey @dcharkes , https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8739796480788876289/+/u/test_results/new_test_failures__logs_
this is one of the test (which i wrote) failing

In order to re-generate expectations run tests with -DupdateExpectations=true VM option:  
tools/test.py -m release --vm-options -DupdateExpectations=true pkg/vm/

It tells me to run above command, but it throwing error on some tests win_sdk_path and i set $env:DEPOT_TOOLS_WIN_TOOLCHAIN=0 (which is i do usually while building dart)

But i run this as u suugest previously
dart -DupdateExpectations=true pkg/vm/test/transformations/ffi_test.dart address_of_cast_compound_element
And but its still making the same kernel files (for both .aot.expect and .expect)

Am i missing something ? :(

@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 13, 2024

pkg/vm/ is indeed trying to do a lot of stuff. How about dart -DupdateExpectations=true pkg/vm/test/transformations/ffi_test.dart?

Edit: Also, try rebuilding the full SDK just in case tools/build.py -mrelease create_platform_sdk runtime.

(Some command in my terminal history on Mac: $ tools/build.py -ax64 -mrelease create_platform_sdk runtime && dart pkg/front_end/tool/update_expectations.dart *ffi* && dart -DupdateExpectations=true pkg/vm/test/transformations/ffi_test.dart I believe the second command is not needed. We didn't change any existing things.)

@codesculpture
Copy link
Contributor

codesculpture commented Aug 14, 2024

@dcharkes , i did ran the above locally but all i got no changes in .expect files and all tests ran successfully.

@codesculpture
Copy link
Contributor

codesculpture commented Aug 14, 2024

Can there be any problem that i dint synced up with latest commits(i think am too behind) comparing upstream? (I cannot find other reason for tests failing)

@dcharkes
Copy link
Contributor Author

  Actual:     @#C13
  Expected:   @#C12

This looks like there might be like a new constant somewhere in the program. That could indeed be that you are too behind. You can try rebasing/merging your PR. If that doesn't work, I can try it locally here.

@codesculpture
Copy link
Contributor

codesculpture commented Aug 14, 2024

I did synced with upstream
This is how i usually do git pull upstream main where upstream is https://github.com/dart-lang/sdk

Just giving this info just in case, if i done something which makes some issue.

@codesculpture
Copy link
Contributor

@dcharkes you can vote commit queue to trigger tests, copybara synced my changes.

@codesculpture
Copy link
Contributor

I will have some questions if the tests passes after i synced with upstream,

I assumed testcases and building dart are all run based on my HEAD(base) (which might not synced with upstream in my case)

So my HEAD (which is here my branch), would not contain any synced source code or testcases. So how then while ci run tests and able to produce different expectations. It can produce different expectations if it the source code changed (might get synced) but i assume it cannot, is it?

@codesculpture
Copy link
Contributor

codesculpture commented Aug 14, 2024

I should rantransformations/ffi_test.dart to see whether it producing different .expect files with latest commits from upstream before submitting to gerrit.
Sorry @dcharkes , am doing that now.

But Hoping that static_checks tests should pass now.

@codesculpture
Copy link
Contributor

Gotcha, .expect file changed with latest changes. Pushed the changes. But now there is a one more bad boy remaining, tests/ffi/static_checks/address_of_cast_test is failing.

@codesculpture
Copy link
Contributor

codesculpture commented Aug 14, 2024

I never actually ran
$ tools/test.py -mrelease -cdart2analyzer tests/ffi/static_checks/address_of_cast_test.dart
Because it simply throws

Error: ProcessException: The system cannot find the file specified.
Command: sdk\bin\dart.exe --packages=.dart_tool/package_config.json pkg/analyzer_cli/bin/analyzer.dart --use-analysis-driver-memory-byte-store --dart-sdk=<location>/dart/sdk/sdk --batch

where --dart-sdk=<location>/dart/sdk/sdk sdk is repeated twice, seems that the reason it cannot find file ?

Btw i ran tests with -cfasta and those are passed locally.
i think i need to make the tests pass with dart2analyzer locally and i might find the reason for the issue ?

@dcharkes

@codesculpture
Copy link
Contributor

I never actually ran
$ tools/test.py -mrelease -cdart2analyzer tests/ffi/static_checks/address_of_cast_test.dart
Because it simply throws

Error: ProcessException: The system cannot find the file specified.
Command: sdk\bin\dart.exe --packages=.dart_tool/package_config.json pkg/analyzer_cli/bin/analyzer.dart --use-analysis-driver-memory-byte-store --dart-sdk=<location>/dart/sdk/sdk --batch

where --dart-sdk=<location>/dart/sdk/sdk sdk is repeated twice, seems that the reason it cannot find file ?

Btw i ran tests with -cfasta and those are passed locally.
i think i need to make the tests pass with dart2analyzer locally and i might find the reason for the issue ?

@dcharkes

I now passed --use-sdk flag to locate the dart sdk in the right path which no longer the above error occurs. But now facing this error which i posted on here

Wrong full snapshot version, expected xxx, found yyy

I am getting this while running tests on dart2analyzer compiler locally (also am passing --use-sdk) . Any idea ? Thanks.

@codesculpture
Copy link
Contributor

@dcharkes
Copy link
Contributor Author

Wrong full snapshot version, expected xxx, found yyy

I am getting this while running tests on dart2analyzer compiler locally (also am passing --use-sdk) . Any idea ? Thanks.

What OS and hardware are you working on? Windows x64?

$ python3 tools/build.py -mrelease -ax64 create_sdk runtime should ensure everything is built (note create_sdk can take quite a while). You can delete the whole out/ReleaseX64/ directory to ensure it's a clean build.

@codesculpture
Copy link
Contributor

This can be closed @dcharkes
Thanks a very lot for literally answering my every silly questions, thanks for your time.
Really appreciate it.

@dcharkes
Copy link
Contributor Author

This can be closed @dcharkes Thanks a very lot for literally answering my every silly questions, thanks for your time. Really appreciate it.

Thanks for contributing! 🚀

I've written https://github.com/dart-lang/sdk/blob/main/runtime/docs/contributing_to_dart_ffi.md to answer some of the frequently asked questions. Maybe you can take a look at it and make a PR for the things that are missing for the next new contributor!

@codesculpture
Copy link
Contributor

Thanks for doing this documentation, looking forward to contribute more !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-native-interop Used for native interop related issues, including FFI. contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) library-ffi
Projects
None yet
Development

No branches or pull requests

2 participants