Skip to content

FfiNative PictureRecorder::endRecording wrong return type #108309

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
Closed
dcharkes opened this issue Jul 25, 2022 · 5 comments · Fixed by flutter/engine#34891
Closed

FfiNative PictureRecorder::endRecording wrong return type #108309

dcharkes opened this issue Jul 25, 2022 · 5 comments · Fixed by flutter/engine#34891
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Jul 25, 2022 •
edited
Loading 

  @FfiNative<Handle Function(Pointer<Void>, Handle)>('PictureRecorder::endRecording')
  external void _endRecording(Picture outPicture);

https://github.com/flutter/engine/blob/main/lib/ui/painting.dart#L5664-L5665

Either the Dart return type should be Object or the native return type should be Void.

cc @dnfield, landing the static checks found another issue.

I'll revert the added checks in Dart for now.
@dnfield
Copy link
Contributor

dnfield commented Jul 25, 2022

The native return type shoul dbe Void.

@dnfield dnfield added easy fix engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list labels Jul 25, 2022
@dnfield
Copy link
Contributor

dnfield commented Jul 25, 2022

Can we just fix this forward and keep the checks?

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jul 25, 2022
@dcharkes
Revert "[cfe/ffi] Improve FFI call mismatched types compile errors"
f972214 
This reverts commit 206fdf1.

Reason for revert: Flutter issues are caught by this CL, preventing
Flutter from building.
flutter/flutter#108309

Original change's description:
> [cfe/ffi] Improve FFI call mismatched types compile errors
>
> This CL fixes two issues.
>
> 1. `FfiNative`s now check the Dart and native type for compatiblity.
> 2. Both `FfiNative`, `asFunction`, and `lookupFunction` check the type
>    correspondence between native and Dart type with a subtype check of
>    the expected Dart type and the provided Dart type. For functions,
>    any return type is a subtype of a void type. This is fine for Dart,
>    but not for native calls. This CL manually checks the return type
>    for void.
>
> This CL does not fix the inconsistency between `asFunction` and
> `FfiNative` with regard to allowing more strict return types than
> `Object` for `Handle`s
> Issue: #49518
>
> Analyzer fixes in follow up CL.
>
> TEST=tests/ffi/vmspecific_static_checks_ffinative_test.dart
>
> Closes: #49471
> Change-Id: Ibc7bd6a1a0db59cc5fa5d755d76999fd7e9a06a4
> Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try,analyzer-mac-release-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/252601
> Reviewed-by: Martin Kustermann <[email protected]>
> Commit-Queue: Daco Harkes <[email protected]>

[email protected],[email protected]

Change-Id: Id82b129d491adcc94cdd685a0a0f6a43248c71f2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Issue: #49518
Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try,analyzer-mac-release-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/252662
Reviewed-by: Daco Harkes <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
dnfield added a commit to flutter/engine that referenced this issue Jul 25, 2022
@dnfield
Fixe _endRecording native return time
62158a4 
Fixes flutter/flutter#108309

See discussion there -this will be tested on an upcoming dart roll.
@dnfield dnfield mentioned this issue Jul 25, 2022
Merged
@dcharkes
Copy link
Contributor Author

dcharkes commented Jul 25, 2022 •
edited
Loading

It was breaking g3, I'll reland the checks once the Flutter fix has made it to g3.

Shall I make the PR for the fix here and sent it to you? edit: you're quick!

@dnfield dnfield closed this as completed Jul 25, 2022
@dnfield
Copy link
Contributor

dnfield commented Jul 25, 2022

(feel free to re-open if you need this to track the roll into g3)

@github-actions
Copy link

github-actions bot commented Aug 8, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix _endRecording native return type flutter/engine
2 participants
@dcharkes @dnfield