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] NNBD treatment of nullptr #40234

Closed
dcharkes opened this issue Jan 20, 2020 · 2 comments
Closed

[vm/ffi] NNBD treatment of nullptr #40234

dcharkes opened this issue Jan 20, 2020 · 2 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi NNBD Issues related to NNBD Release vm-nnbd-unfork-sdk Label for all issues that need to be done before the nnbd sdk can be unforked

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Jan 20, 2020

We've given nullptr a Null type arg to make it easily assignable without casts to arbitrary type args.

sdk/sdk/lib/ffi/ffi.dart

Lines 31 to 33 in e7b1392

/// Represents a pointer into the native C memory corresponding to "NULL", e.g.
/// a pointer with address 0.
final Pointer<Null> nullptr = Pointer.fromAddress(0);

Pointer<Int8> value = nullptr;

However, this is no longer allowed with NNBD.

ERROR|STATIC_TYPE_WARNING|INVALID_ASSIGNMENT|/usr/local/google/home/dacoharkes/dart-sdk/sdk/tests/ffi/data_test.dart|331|25|7|A value of type 'Pointer' can't be assigned to a variable of type 'Pointer'.

cc @mkustermann

Proposed solution is to change it to the following.

Pointer<T> nullptr<T extends NativeType>() => fromAddress<T>(0);

Pointer<Int8> value = nullptr();

However, this has some downsides:

  • We cannot mark the current nullptr as deprecated, complicating migration.
  • For what it does, a factory constructor would be better, but it would be more verbose.

(Note that we want to keep the API for the NNBD sdk and the legacy sdk compatible, so we also need to migrate the old sdk.)

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi NNBD Issues related to NNBD Release labels Jan 20, 2020
@dcharkes
Copy link
Contributor Author

cc @lrhn

@dcharkes dcharkes added the vm-nnbd-unfork-sdk Label for all issues that need to be done before the nnbd sdk can be unforked label Jan 20, 2020
@lrhn
Copy link
Member

lrhn commented Jan 21, 2020

You probably want final nullptr = Pointer<Never>.fromAddress(0);.
I assume reading from address 0 will throw, which is exactly the correct behavior for a method returning Never.

dart-bot pushed a commit that referenced this issue Jan 29, 2020
Closes: #40233

This CL creates nnbd versions of the tests and runs them on the nnbd sdk.

This CL does not (1) migrate sdk_nnbd/lib/ffi fully yet, and does not (2) fix all the tests/ffi (which is NNBD tests) yet.

Uncovered new issues:
Issue: #40234 nullptr should have type Pointer<Never>.
Issue: #40247 Structs need external fields.
Issue: #40271 Callbacks hit assert in debug.

Change-Id: Icb1b83577e03ed283165eb17703fc8dfc7fa5960
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try,analyzer-nnbd-linux-release-try,dart2js-nnbd-linux-x64-chrome-try,ddc-nnbd-linux-release-chrome-try,front-end-nnbd-linux-release-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-nnbd-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/132604
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Reviewed-by: Bob Nystrom <rnystrom@google.com>
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 NNBD Issues related to NNBD Release vm-nnbd-unfork-sdk Label for all issues that need to be done before the nnbd sdk can be unforked
Projects
None yet
Development

No branches or pull requests

2 participants