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.

Sign up for GitHub

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

Jump to bottom

[vm/ffi] Deprecate empty structs #43974

Closed
dcharkes opened this issue Oct 29, 2020 · 2 comments
Closed

[vm/ffi] Deprecate empty structs #43974

dcharkes opened this issue Oct 29, 2020 · 2 comments
Assignees
@dcharkes
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. deprecation library-ffi type-enhancement A request for a change that isn't a bug
Milestone
February Beta Rel...

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Oct 29, 2020 •
edited
Loading

In https://dart-review.googlesource.com/c/sdk/+/140290 and https://dart-review.googlesource.com/c/sdk/+/169221 we introduce passing structs by value and nested structs respectively. Both features disallow using empty structs, as this is undefined behavior in C.

However, we currently have empty structs in use in for example package:ffi, so disallowing empty structs altogether is a breaking change. So, these CLs just allow empty structs in FFI calls, callbacks, and structs fields.

Having empty structs confusing, allocate<EmptyStruct>() allocates 0 bytes of memory. See dart-archive/ffi#34.

So we should consider disallowing empty structs altogether.

We do however need a replacement for Pointer<Utf8>:

cc @mkustermann
@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Oct 29, 2020
@mit-mit mit-mit added this to the January Beta Release (2.12) milestone Jan 7, 2021
@dcharkes dcharkes mentioned this issue Jan 8, 2021
Closed
@franklinyow franklinyow added P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug labels Jan 12, 2021
dart-bot pushed a commit that referenced this issue Jan 13, 2021
@dcharkes
[vm/ffi] Add Opaque type
5d40d52 
Issue: #44622
Issue: #43974

TEST=samples/ffi/sqlite/lib/src/bindings/types.dart
TEST=tests/ffi/vmspecific_static_checks_test.dart

Change-Id: Ib9e72df6a07b1bc2b72a7db66f945652814baf51
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,analyzer-nnbd-linux-release-try,front-end-linux-release-x64-try,front-end-nnbd-linux-release-x64-try,benchmark-linux-try,dart-sdk-linux-try,pkg-linux-release-try,vm-ffi-android-release-arm-try,vm-ffi-android-release-arm64-try,vm-kernel-nnbd-win-debug-x64-try,vm-kernel-win-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/178984
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Clement Skau <[email protected]>
dart-bot pushed a commit that referenced this issue Jan 15, 2021
@dcharkes
[vm/ffi] Migrate to empty Structs to Opaque
ac805df 
This CL migrates empty `Struct`s to `Opaque` native types.
It stops using `.ref` on `Pointer`s to these types.
And stops using `.ref` on `Pointer<Utf8>` which will be migrated in
`package:ffi`.

Issue: #44622
Issue: #43974

Change-Id: I3aa256af7d4ceaa8ee37b1b2ada71f870f43617b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/179181
Reviewed-by: Vyacheslav Egorov <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
dart-bot pushed a commit that referenced this issue Jan 15, 2021
@dcharkes
[vm/ffi] Add fields to Structs which should not be empty
3a0a33f 
This CL adds fields to `Struct`s which should not be empty or removes
the structs.

Issue: #44622
Issue: #43974

TEST=tests/ffi/vmspecific_static_checks_test.dart

Change-Id: I4a684bd96e3ed16a3fd0dd17019aeabf64ef074a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/179182
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>
dart-bot pushed a commit that referenced this issue Jan 20, 2021
@dcharkes
[vm/ffi] Roll package:ffi to Allocator and Opaque
7d648e3 
This can only be landed when `Allocator` and `Opaque` have rolled into
Flutter/engine, that into Flutter/flutter, and that into g3.
flutter/flutter/commit/a706cd211240f27be3b61f06d70f958c7a4156fe

Deletes all the copies of `_CallocAllocator` and uses the one from
`package:ffi` instead.

Issue: #44622
Issue: #43974
Issue: #44621
Issue: #38721

Change-Id: I50b3b4c31a2b839b35e3e057bd54f463b90bc55e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/179540
Reviewed-by: Aske Simon Christensen <[email protected]>
@dcharkes
Copy link
Contributor Author

dcharkes commented Jan 21, 2021 •
edited
Loading

Reverted commit, requires flutter/engine#23808 to land in g3.

@dcharkes dcharkes reopened this Jan 21, 2021
dart-bot pushed a commit that referenced this issue Jan 22, 2021
@dcharkes
[vm/ffi] Deprecate empty Structs
c3b30df 
The analyzer and CFE now report a warning on subclasses of `Struct`
which have no native fields.

Adapted from https://dart-review.googlesource.com/c/sdk/+/180189 to
only deprecate, but not disallow empty structs.

Rolls `package:ffi` forward to migrate `Utf8` and `Utf16` to `Opaque`
to prevent tests from failing on those generating warnings for empty
structs.

Issue: #43974

TEST=tests/ffi/vmspecific_static_checks_test.dart

Change-Id: Ia364e31da66cb195570c2e2b729d03dd261f28a2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180361
Reviewed-by: Clement Skau <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>
@dcharkes
Copy link
Contributor Author

Empty structs cause a deprecation hint as of c3b30df, keeping issue open to track making these an error.

Making these an error is not blocking the next release, removing this from milestone.

@dcharkes dcharkes removed this from the January Beta Release (2.12) milestone Jan 22, 2021
@dcharkes dcharkes removed the P1 A high priority bug; for example, a single project is unusable or has many test failures label Jan 22, 2021
@dcharkes dcharkes added this to the February Beta Release milestone Jan 22, 2021
dart-bot pushed a commit that referenced this issue Feb 10, 2021
@dcharkes
Reland "[vm/ffi] Roll package:ffi to Allocator and Opaque"
a45276a 
This can only be landed when `Allocator`, `Opaque`, and `AllocatorAlloc`
have rolled into Flutter/engine, that into Flutter/flutter, and into g3.

Deletes all the copies of `_CallocAllocator` and uses the one from
`package:ffi` instead.

Bug: #44622
Bug: #43974
Bug: #44621
Bug: #38721

Change-Id: I486034b379b5a63cad4aefd503ccd0f2ce5dd45e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/180188
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Aske Simon Christensen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dcharkes dcharkes

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. deprecation library-ffi type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
February Beta Release
Development

No branches or pull requests
3 participants
@dcharkes @mit-mit @franklinyow