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

proposed breaking change to dart:ffi: stop reifying Pointer<T> #49935

Closed
mraleph opened this issue Sep 9, 2022 · 8 comments
Closed

proposed breaking change to dart:ffi: stop reifying Pointer<T> #49935

mraleph opened this issue Sep 9, 2022 · 8 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@mraleph
Copy link
Member

mraleph commented Sep 9, 2022

I propose we stop reifying underlying element type for Pointer<T> objects in preparation to unboxing pointer or migrating FFI to views. I am filing this issue for discussion.

Currently Pointer<T> objects are carrying their reified type around, but most of the APIs are based on the static type.

I propose we fix any APIs that are still using dynamic type and switch Pointer<T> factory to produce Pointer<Never> instead.

/cc @dcharkes @mkustermann

@mraleph mraleph added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Sep 9, 2022
@dcharkes
Copy link
Contributor

dcharkes commented Sep 9, 2022

most of the APIs are based on the static type.

Correct, we migrated almost everything to extension methods.

I see elementAt is still a method, but it is documented that it requires a compile-time constant for the type argument and transformed in the CFE. I don't remember why it is not an extension method, it probably should be.

We own all the constructors, and Pointer cannot be extended, so we should be able to ensure all pointers will have Never as type argument at runtime. I can whip up a CL to do that (including patch files, kernel-gen in CFE, IL-gen in VM, and direct Pointer object creation in VM).

@eernstg I'd love to know if defining Pointer (with a type argument) as a view on int would work with your proposal for views.

@dcharkes dcharkes self-assigned this Sep 9, 2022
@mraleph
Copy link
Member Author

mraleph commented Sep 10, 2022

as a view on int

One interesting consideration here is that int will be too wide for 32-bit ARM. We need something like _int32, a hidden type which reifies to int when boxed but can also be stored in a smaller unboxed location.

copybara-service bot pushed a commit that referenced this issue Nov 28, 2022
All `Pointer`s will now have the `Never` type argument at runtime.

TEST=tests/ffi(_2)/*

Bug: #49935

Change-Id: I83c8bba9461c2cab22992ba2e3cf42b7b5f43c36
Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-win-debug-x64-try,vm-kernel-precomp-win-debug-x64c-try,vm-kernel-mac-debug-x64-try,dart-sdk-mac-arm64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-ffi-android-debug-arm64c-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/272201
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Slava Egorov <[email protected]>
@knopp
Copy link
Contributor

knopp commented Dec 10, 2022

The merged CL seems to have caused a regression: #50678.

@mkustermann
Copy link
Member

@knopp Thank you for the report. We are already aware, a fix is on its way

@mkustermann
Copy link
Member

@knopp We have identified the issue with the CL, was fixed in e3e355e.

I see elementAt is still a method, but it is documented that it requires a compile-time constant for the type argument and transformed in the CFE. I don't remember why it is not an extension method, it probably should be.

@dcharkes should we go ahead and do that before Dart 3?

@knopp
Copy link
Contributor

knopp commented Dec 14, 2022

I can confirm that e3e355e fixes the error for me. Thanks!

@dcharkes
Copy link
Contributor

I see elementAt is still a method, but it is documented that it requires a compile-time constant for the type argument and transformed in the CFE. I don't remember why it is not an extension method, it probably should be.

@dcharkes should we go ahead and do that before Dart 3?

I've filed #50714 to track this.

This issue can be closed.

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

4 participants