Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[fuchsia] [ffi] Basic support for Channels and Handles #27849

Merged

Conversation

iskakaushik
Copy link
Contributor

@iskakaushik iskakaushik commented Aug 2, 2021

Add basic channel support for dart:zircon using dart:ffi.

@google-cla google-cla bot added the cla: yes label Aug 2, 2021
@iskakaushik iskakaushik force-pushed the dart_zircon_channel_support branch 7 times, most recently from bdebe56 to 0be0100 Compare August 5, 2021 16:01
@iskakaushik iskakaushik changed the title [fuchsia] [ffi] WIP channel support [fuchsia] [ffi] Basic support for Channels and Handles Aug 5, 2021
@iskakaushik iskakaushik marked this pull request as ready for review August 5, 2021 16:24
@iskakaushik
Copy link
Contributor Author

Tested in fxr/565162

@iskakaushik iskakaushik force-pushed the dart_zircon_channel_support branch from f506671 to e57aff3 Compare August 5, 2021 16:49
#include "include/dart_api_dl.h"

int zircon_dart_dl_initialize(void* initialize_api_dl_data) {
if (Dart_InitializeApiDL(initialize_api_dl_data) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we gonna expose this through the embedder C API? The version of dart used and access to the dart VM is hidden behind the embedder C API....

It works for now with the code in-engine-tree, but how will we guarantee that it will continue to work when we are consuming only embedder.h & libflutter.so? What happens if the dart API versions of the embedder and engine diverge? Does the Dart API also present a stable ABI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the engine and embedding use diverging versions of dart we will also have AOT considerations to worry about. I think in the short-medium term we will be using the same version for both fuchsia-embedding and the engine. When we decide to allow divergence of dart versions, we can likely piggyback on the same solution as AOT to resolve this.

Also Dart_InitializeApiDL is currently only checking for a min version of dart. I think the dart_api_dl.h does give us a stable ABI, but I will confirm can report back to you.


bool close() {
if (isValid()) {
int? ret = zirconFFIBindings?.zircon_dart_handle_close(_ptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's assert(isValid()) here, then do the if check. 99% of the time, isValid() being false here indicates a coding error in the dart application

This is the behavior that the existing implementation has (DCHECK on handle validity, then proceed forward with an if)

@pragma('vm:entry-point')
ZDChannel._(this.handlePair);

final ZDHandlePair handlePair;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keep both ends of the channel-pair inside of one object? Conceptually, a channel represents only one half of the pair, and very often dart code will only possess one half with no ability to refer to the other half

The old implementation has a shim object called "ChannelPair" to handle this

@arbreng arbreng self-requested a review August 10, 2021 16:33
Copy link
Contributor

@arbreng arbreng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after adding the assert(isValid())

Comment on lines +75 to +77
// ZIRCON_FFI_EXPORT zircon_dart_handle_t* zircon_dart_duplicate_handle(
// zircon_dart_handle_t* handle,
// uint32_t rights);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to leave this in?

@chinmaygarde
Copy link
Member

This looks good to go. Can we commit this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes platform-fuchsia waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants