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] support redirecting external const factory constructors #45101

Closed
dcharkes opened this issue Feb 24, 2021 · 3 comments
Closed

[vm] support redirecting external const factory constructors #45101

dcharkes opened this issue Feb 24, 2021 · 3 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi

Comments

@dcharkes
Copy link
Contributor

[...]
dart.ffi.Array.Array

stderr:
Unimplemented handling of missing static target

--- Re-run this test:
python tools/test.py -n dartk-linux-release-x64 lib_2/mirrors/invocation_fuzz_test/none

Happens when using patch file to implement an external const factory as a redirecting factory constructor.

class Array<T extends NativeType> extends NativeType {
  external const factory Array(int dimension1);
}

patch file:

@patch
@pragma("vm:entry-point")
class Array<T extends NativeType> {
  // ...

  @patch
  const factory Array(int dimension1) = _ArraySize<T>;
}

class _ArraySize<T extends NativeType> implements Array<T> {
  final int dimension1;

  const _ArraySize(this.dimension1);
}

Does not happen without using patch files:

class Array<T extends NativeType> extends NativeType {
  const factory Array(int dimension1) = _ArraySize<T>;
}

class _ArraySize<T extends NativeType> implements Array<T> {
  final int dimension1;

  const _ArraySize(this.dimension1);
}
@dcharkes dcharkes added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Feb 24, 2021
dart-bot pushed a commit that referenced this issue Feb 24, 2021
Adds support for single dimension inline arrays in structs. Multi-
dimensional arrays will be supported in a future CL.

This CL adds:
- CFE static error checks for inline arrays.
- CFE transformations for inline arrays.
- VM consumption of inline array fields for NativeType.
- Test generator support for inline arrays + generated tests.

Previous CLs added support for inline arrays in:
- analyzer https://dart-review.googlesource.com/c/sdk/+/183684
  - updated in this CL to new API.
- ABI calculation https://dart-review.googlesource.com/c/sdk/+/183682

Closes: #35763

Open issue: #45101

CFE transformations are tested with expectation files:
TEST=pkg/front_end/testcases/(.*)/ffi_struct_inline_array.dart

Trampolines and CArray API are tested with end-to-end Dart tests:
TEST=tests/ffi(_2)/(.*)by_value(.*)test.dart
TEST=tests/ffi(_2)/inline_array_test.dart

Compile-time errors (both CFE and analyzer) are tested in:
TEST=tests/ffi(_2)/vmspecific_static_checks_test.dart

Change-Id: I014c0e4153f1b885638adce80de6ab3cac8e6bb2
Cq-Include-Trybots: luci.dart.try:dart-sdk-linux-try,dart-sdk-mac-try,dart-sdk-win-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-mac-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-nnbd-linux-debug-ia32-try,vm-kernel-nnbd-mac-release-x64-try,vm-kernel-nnbd-win-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,vm-kernel-msan-linux-release-x64-try,vm-kernel-precomp-msan-linux-release-x64-try,vm-kernel-precomp-android-release-arm_x64-try,analyzer-analysis-server-linux-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183640
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
@dcharkes dcharkes added area-front-end Use area-front-end for front end / CFE / kernel format related issues. library-ffi labels Feb 24, 2021
@dcharkes dcharkes added this to the March Beta Release milestone Feb 24, 2021
@chloestefantsova chloestefantsova self-assigned this Feb 24, 2021
@dcharkes
Copy link
Contributor Author

The workaround in mentioned CL should be undone before next stable release.

@dcharkes
Copy link
Contributor Author

dcharkes commented Feb 24, 2021

In debug mode

../../runtime/vm/compiler/frontend/kernel_translation_helper.cc: 436: error: expected: IsProcedure(procedure)
IsConstructor(procedure)
true
IsProcedure(procedure)
false
@#C17
    @#C17
    static factory /* from org-dartlang-sdk:///sdk/lib/_internal/vm/lib/ffi_patch.dart */ •<T extends ffi::NativeType = ffi::NativeType>(core::int dimension1) → ffi::Array<ffi::Array::•::T>
      let dynamic #redirecting_factory = ffi::_ArraySize::• in let ffi::Array::•::T #typeArg0 = null in invalid-expression;
dart::Assert::Fail(dart::Assert * this, const char * format) (/usr/local/google/home/dacoharkes/master/sdk/runtime/platform/assert.cc:44)
dart::kernel::TranslationHelper::DartProcedureName(dart::kernel::TranslationHelper * this, dart::kernel::NameIndex procedure) (/usr/local/google/home/dacoharkes/master/sdk/runtime/vm/compiler/frontend/kernel_translation_helper.cc:436)
dart::kernel::TranslationHelper::LookupStaticMethodByKernelProcedure(dart::kernel::TranslationHelper * this, dart::kernel::NameIndex procedure) (/usr/local/google/home/dacoharkes/master/sdk/runtime/vm/compiler/frontend/kernel_translation_helper.cc:610)
dart::kernel::StreamingFlowGraphBuilder::BuildStaticGet(dart::kernel::StreamingFlowGraphBuilder * this, dart::TokenPosition * p) 

The target of a static get can be a Constructor, which is not supported by the VM currently.

N.b. The StaticGet an encoding of the redirection, there's no executable semantics.

Thanks @stefantsov for the background knowledge!

@dcharkes dcharkes removed the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Feb 24, 2021
@dcharkes dcharkes changed the title "Unimplemented handling of missing static target" on external const factory constructor [vm] Support StaticGet target to be a constructor Feb 24, 2021
@dcharkes
Copy link
Contributor Author

dcharkes commented Feb 25, 2021

Related issues #28421 and #28424.

Turns out the VM was trying to compile the invalid expressions because of a missing flag (which was not visible in the textual representation of kernel): 2 line fix. Combining that fix with parsing an additional tag in the VM might work.

dart-bot pushed a commit that referenced this issue Feb 26, 2021
Bug: #45101
Change-Id: Id0c9f3ce319111a79b2085f44a4e4243dbe6597a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187440
Commit-Queue: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Feb 26, 2021
This CL introduces the first combination of `external const factory`
with `@patch const factory (.*) = (.*)`. This specific combination was
never used in the core libraries, and did not work.

Half of the fix is a missing flag from the CFE:
https://dart-review.googlesource.com/c/sdk/+/187440

The other half of the fix is matching on the
kRedirectingFactoryConstructor tag, and allowing constructor names
in procedure targets (even though we never compile those expressions
because they are invalid).

Bug: #45101

TEST=Using redirecting const factory in patch file in core lib.
TEST=tests/ffi(_2)/*

Change-Id: I5524bde928290bf32aaea9170eda1f2d03127fa6
Cq-Include-Trybots: luci.dart.try:benchmark-linux-try,front-end-linux-release-x64-try,vm-kernel-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/187003
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
@dcharkes dcharkes changed the title [vm] Support StaticGet target to be a constructor [vm] support redirecting external const factory constructors Feb 26, 2021
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, FFI, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

2 participants