dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.04k stars 1.55k forks source link

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

Closed dcharkes closed 1 day ago

dcharkes commented 2 months ago

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: https://github.com/dart-lang/sdk/blob/05d9e5bf37a68ec75f8ffc56a65a89e0e86b5a1d/pkg/vm/lib/modular/transformations/ffi/use_sites.dart#L1489-L1504 The implementation should find the cast() invocation and use its receiver instead (effectively ignoring the cast expression as a no-op).

codesculpture commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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

dcharkes commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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:

https://github.com/dart-lang/sdk/blob/e9c7b2b905e0167cdc3402f086f3fa53872e8b04/pkg/vm/lib/modular/transformations/ffi/use_sites.dart#L1521-L1530

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

https://github.com/dart-lang/sdk/blob/e9c7b2b905e0167cdc3402f086f3fa53872e8b04/runtime/vm/compiler/backend/il.cc#L7589-L7599

dcharkes commented 1 month ago

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 commented 4 weeks ago

https://github.com/dart-lang/sdk/pull/56357 @dcharkes , consider this as a WIP and please review if this is correct approach.

codesculpture commented 4 weeks ago

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 commented 3 weeks ago

Yes, that's the right approach!

fileOffset can probably just stay the same.

codesculpture commented 3 weeks ago

@dcharkes , where can i write test for this?

dcharkes commented 3 weeks ago

@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

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

codesculpture commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

Please help if u can @dcharkes

Thanks

Edit: mraleph helped me.

codesculpture commented 3 weeks ago
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 commented 3 weeks ago

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

codesculpture commented 3 weeks ago

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

codesculpture commented 3 weeks ago

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 commented 3 weeks ago
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:

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 commented 3 weeks ago

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

dcharkes commented 3 weeks ago

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

You run the test, and then it complains it expected a certain error string, and you paste the string in. πŸ™ˆ

To run the test for the cfe and analyzer, you can use the following (replace the test name).

$ tools/build.py -mrelease runtime create_platform_sdk && tools/test.py -mrelease -cfasta tests/ffi/static_checks/vmspecific_static_checks_array_test.dart
$ tools/build.py -mrelease runtime create_platform_sdk && tools/test.py -mrelease -cdart2analyzer tests/ffi/static_checks/vmspecific_static_checks_array_test.dart
codesculpture commented 3 weeks ago

You run the test, and then it complains it expected a certain error string, and you paste the string in. πŸ™ˆ

I thought this and imagined this could only be possible in my dreamsπŸ˜‚

Fine then, will copy/paste.

Thanks @dcharkes for all of the help.

I had a comment (which is a question) here

dcharkes commented 3 weeks ago

You run the test, and then it complains it expected a certain error string, and you paste the string in. πŸ™ˆ

I thought this and imagined this could only be possible in my dreamsπŸ˜‚

Fine then, will copy/paste.

Thanks @dcharkes for all of the help.

Thanks for contributing! ❀️

I had a comment (which is a question) here

That was next in my inbox, I've left a reply!

codesculpture commented 3 weeks ago

@dcharkes, i asked a question https://dart-review.googlesource.com/c/sdk/+/378221 here, pls help.

codesculpture commented 3 weeks ago

In tests/ffi what would be the testcase look like for this?

I planned to write a test which would perform addition(Sum42) through ffi (ffi_test_functions.cc) where i try to send Int8 as Pointer<Void> by performing .address.cast() and try to evaluate the results.

Actually it isn't possible since the parameters in ffi is Int8 but am trying to send Pointer<Void>, i may write a Sum42 which accepts pointer and try to evaluate.

But firstly whether this is correct approach to test this .address.cast() support.

I convinced this is correct because anyway the .cast() must not damage the address which can be passed without .cast()

I already wrote a transformation test in pkg/vm/testcases to evaluate .address.cast() transformation in dart.

So all we can evaluate in tests/ffi whether the address passing to ffi as expected.

What do you think @dcharkes

dcharkes commented 3 weeks ago

i may write a Sum42 which accepts pointer and try to evaluate.

πŸ‘

But firstly whether this is correct approach to test this .address.cast() support.

Yes, you want to have some Pointer in Dart which has a different type argument than that what the C function accepts. (And for the TypedData test some different TypedList than expected, and for struct fields some different int type field than the pointer expects.)

What do you think @dcharkes

Yep, sounds good! Just make sure you cover all the cases

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

codesculpture commented 3 weeks ago

Hey @dcharkes, does the ffi_test_functions.cc would get linked into dart binary while building seems its not (and ofc it shouldn't)

The reason why i asked this, i need to link the ffi_test_functions.cc to my local dart build to run some tests in tests/ffi which are based on the symbols from ffi_test_functions.cc

I need to test .address.cast(), but it only supports on Native Leaf calls.

I created a local c files and generated symbols for it, but .address expression are failed as i loaded the symbols using DynamicLibrary.open() which is considered not Native.

Sorry for shooting you lot of questions. And very thanks for helping me to land this.

dcharkes commented 2 weeks ago

I created a local c files and generated symbols for it, but .address expression are failed as i loaded the symbols using DynamicLibrary.open() which is considered not Native.

Ah right, my bad!

You can take a look at tests/ffi/address_of_typeddata_generated_test.dart

  // Force dlopen so @Native lookups in DynamicLibrary.process() succeed.
  dlopenGlobalPlatformSpecific('ffi_test_functions');

After that you can use @Natives πŸ˜‰

codesculpture commented 2 weeks ago

Hi @dcharkes, while i implementing the tests, i found this https://github.com/dart-lang/sdk/issues/56425 I can provide more information if needed

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

codesculpture commented 2 weeks ago
$ 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 commented 2 weeks ago

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

dcharkes commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/dart-lang/sdk/issues/56462. Just expect both errors for now and add a comment that refers to the bug. We can fix that in another PR.

codesculpture commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

codesculpture commented 2 weeks ago

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 commented 2 weeks ago
  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 commented 2 weeks ago

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 commented 2 weeks ago

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

codesculpture commented 2 weeks ago

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?