[L] Change in dart/sdk[main]: [ffi] Reland: Add DynamicLibrary.openFromAssetId
Shikhar Soni (Gerrit)
Shikhar Soni added 4 comments![]( "Open in Gerrit")
This CL fixes that bug and relands the changes.Shikhar SoniWhat was the bug? And what was the diff?
Daco HarkesThere was a logic error in case an embedder(like flutter engine) provided a `dlopen` hook but didn't find the asset.
Diff:
```
if (native_assets_api->dlopen != nullptr) {
NoActiveIsolateScope no_active_isolate_scope;
handle = native_assets_api->dlopen(asset.ToCString(), error);
- *asset_found = *error == nullptr;
- return handle;
+ if (*error != nullptr || handle != nullptr) {
+ *asset_found = true;
+ return handle;
+ }
}
```I don't know what the procedure is in cases like this: submitting a patch only for the bug or a complete new patch for this.
Shikhar SoniUsually we put the reland in the first patch set and then we put the fix in patchset 2.
And the CL is usually titled: Reland: ...
Acknowledged
void* NativeAssets::DlopenProcess(char** /*error*/) {Shikhar Soniunrelated change?
Acknowledged
*asset_found = true;Shikhar SoniI don't think this behavior is correct. `dlsym(nullptr, ...)` on linux does process lookup and can be used by the embedder. So, we cannot get the correct behavior with the current embedder API.
The flutter embedder uses:
```
void* NativeAssets::DlopenProcess(char** /*error*/) {
#if defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_MACOS) || \
defined(DART_HOST_OS_ANDROID) || defined(DART_HOST_OS_FUCHSIA)
return RTLD_DEFAULT;
#else
return kWindowsDynamicLibraryProcessPtr;
#endif
}
```from engine/src/flutter/runtime/dart_isolate.cc
for `native_assets_api->dlopen`.
So here we have the same issue as we saw before in the Dart standalone VM we cannot distinguish between lookup succeeding and returning `RTLD_DEFAULT` and the asset missing.
I think we need to introduce an `native_assets_api->dlopen_v2` that takes an extra `asset_found` param. (Adding a `native_assets_api->has_asset` would be simpler, but slower.)
Then if `native_assets_api->dlopen_v2` is not nullptr, we use that. Then if `native_assets_api->dlopen` is not nullptr we use that and are on the safe-side with asset-found like in your change here. And otherwise we fall back to the ffi:native-assets library.
(Using `native_assets_api->dlopen` for `DynamicLibrary.fromAssetId` will give the wrong behavior. So, it would be even better if the code-path for that would give an `error` saying that the operation is not supported. And then when we move Flutter to _v2 Flutter will start supporting the new API.)
Acknowledged
if (*error == nullptr && handle == nullptr) {Shikhar SoniThis is always true due to early return.
Acknowledged
Related details
- Daco Harkes
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shikhar Soni (Gerrit)
Shikhar Soni added 1 comment![]( "Open in Gerrit")
- added `NativeAssetsApi::dlopen_v2`
- `dlopen_v2` reports `asset_found`
- it also report `can_be_closed`, because `DynamicLibrary.openFromAssetId` must preserve close semantics
- legacy `dlopen` remains only for `@Native` compatibility
Daco Harkes (Gerrit)
Daco Harkes voted and added 6 comments![]( "Open in Gerrit")
Votes added by Daco Harkes
| Commit-Queue | +1 |
6 comments
} else if (allow_legacy_embedder_dlopen &&If only the legacy embedder dlopen is available and you try to do dlopenFromAssetId, the asset will not be found (via LoadAssetLibraryFromKernelMapping) that will probably lead to confusion for the end user.
I think it's better we do:
```
if (native_assets_api->dlopen != nullptr) {
if (!allow_legacy_embedder_dlopen) {
*error = // ....
return result;
}
``` Exceptions::ThrowUnsupportedError(I'd probably read the `error` value here and set it inside the helper function.
(We'll still need the if to branch correctly to a unsupported error instead of argument error 👍)
const String& msg = String::Handle(String::NewFormatted(Use `native_assets_api->available_assets` if available for the error message so that we print which assets are available. (In case someone does a typo in the asset id.)
/// Loads a library registered under the given [assetId] in the native assetsLoads a code asset registered with [assetId] in a build hook.
/// The [assetId] must be registered in the active native assets mapping."native assets mapping" is not public API terminology.
Instead maybe refer to https://dart.dev/tools/hooks.
testOpenFfiTestFunctionsAssetCanClose(assetId);also the system dylibs test.
Related details
- Shikhar Soni
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 2 comments![]( "Open in Gerrit")
R=dacoh...@google.com, veg...@google.comAnd possibly this line too.
This whitespace prevents the Cq try bots from triggering properly. (only one of the 3 dyn bots seems to be running)
Shikhar Soni (Gerrit)
Shikhar Soni added 9 comments![]( "Open in Gerrit")
Shikhar SoniI just realized that not knowing if the asset was found is also an issue for the dlopen call from the embedder API.
So we'll need to add a dlopen_v2 in the embedder API (to prevent manual rolls) that also has an asset-found out param.
You'll need to make a Flutter engine PR as well to migrate the Flutter engine.
https://github.com/flutter/flutter/blob/master/CONTRIBUTING.md
https://github.com/flutter/flutter/blob/master/docs/engine/contributing/Setting-up-the-Engine-development-environment.md
engine/src/flutter/runtime/dart_isolate.cc -> InitDartFFIForIsolateGroup
Working on the flutter engine part.
And possibly this line too.
Done
This whitespace prevents the Cq try bots from triggering properly. (only one of the 3 dyn bots seems to be running)
Done
If only the legacy embedder dlopen is available and you try to do dlopenFromAssetId, the asset will not be found (via LoadAssetLibraryFromKernelMapping) that will probably lead to confusion for the end user.
I think it's better we do:
```
if (native_assets_api->dlopen != nullptr) {
if (!allow_legacy_embedder_dlopen) {
*error = // ....
return result;
}
```
Done
Exceptions::ThrowUnsupportedError(I'd probably read the `error` value here and set it inside the helper function.
(We'll still need the if to branch correctly to a unsupported error instead of argument error 👍)
PTAL
const String& msg = String::Handle(String::NewFormatted(Use `native_assets_api->available_assets` if available for the error message so that we print which assets are available. (In case someone does a typo in the asset id.)
PTAL
/// Loads a library registered under the given [assetId] in the native assetsLoads a code asset registered with [assetId] in a build hook.
Done
/// The [assetId] must be registered in the active native assets mapping."native assets mapping" is not public API terminology.
Instead maybe refer to https://dart.dev/tools/hooks.
Done
also the system dylibs test.
- Daco Harkes
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daco Harkes
| Code-Review | +1 |
| Commit-Queue | +1 |
1 comment
native_assets_api->dlopen != nullptr;The second condition is not needed right?
Related details
- Shikhar Soni
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shikhar (Gerrit)
Shikhar added 1 comment![]( "Open in Gerrit")
native_assets_api->dlopen != nullptr;The second condition is not needed right?
It is, because `LoadAssetLibraryFromKernelMapping` can also return errors, so distinguish errors from that path and `UnsupportedError`...
I think I should return an `unsupported` flag from `LoadAssetLibrary`, that would be more clean.
Related details
- Shikhar Soni
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes voted and added 4 comments![]( "Open in Gerrit")
Votes added by Daco Harkes
| Code-Review | +1 |
| Commit-Queue | +1 |
- added `NativeAssetsApi::dlopen_v2`
- `dlopen_v2` reports `asset_found`
- it also report `can_be_closed`, because `DynamicLibrary.openFromAssetId` must preserve close semantics
- legacy `dlopen` remains only for `@Native` compatibility
Done
Exceptions::ThrowUnsupportedError(Shikhar SoniI'd probably read the `error` value here and set it inside the helper function.
(We'll still need the if to branch correctly to a unsupported error instead of argument error 👍)
PTAL
Done
ShikharThe second condition is not needed right?
It is, because `LoadAssetLibraryFromKernelMapping` can also return errors, so distinguish errors from that path and `UnsupportedError`...
I think I should return an `unsupported` flag from `LoadAssetLibrary`, that would be more clean.
Acknowledged
const String& msg = String::Handle(String::NewFormatted(Shikhar SoniUse `native_assets_api->available_assets` if available for the error message so that we print which assets are available. (In case someone does a typo in the asset id.)
PTAL
Done
Related details
- Shikhar
- Shikhar Soni
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 3 comments![]( "Open in Gerrit")
bool* can_be_closed,I sense some API design issue here: we have hooks for `dlopen` and `dlsym` but not for `dlclose`. I think a better way to do this would be to actually have a hook for `dlclose` which returns appropriate error on unclosable handles returned by `dlopen`.
bool* asset_found,It seems that this is only relevant for error reporting? Why have it then - let the embedder put `No asset ${id} found` into error.
external factory DynamicLibrary.openFromAssetId(String assetId);Maybe better name is just `DynamicLibrary.openAsset` or `DynamicLibrary.fromAsset` or `DynamicLibrary.asset` or something like this, `openFromAssetId` is a mouthful.
Related details
- Daco Harkes
- Shikhar Soni
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 3 comments![]( "Open in Gerrit")
bool* can_be_closed,I sense some API design issue here: we have hooks for `dlopen` and `dlsym` but not for `dlclose`. I think a better way to do this would be to actually have a hook for `dlclose` which returns appropriate error on unclosable handles returned by `dlopen`.
Good point. If we someone adds a new operating system with different system calls, it would be a different close system call!
Related issue which mentions the need for having a dlclose in the embedder API: https://github.com/dart-lang/sdk/issues/55521
We'll need to add an extra bool to the DynamicLibrary class to register that it was created via the embedder API for code assets, so that close can forward to that.
external factory DynamicLibrary.openFromAssetId(String assetId);Maybe better name is just `DynamicLibrary.openAsset` or `DynamicLibrary.fromAsset` or `DynamicLibrary.asset` or something like this, `openFromAssetId` is a mouthful.
`DynamicLibrary.openCodeAsset` or `DynamicLibrary.codeAsset` to be consistent with https://dart.dev/tools/hooks and https://pub.dev/packages/code_assets terminology.
Maybe the latter.
Related details
- Shikhar Soni
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shikhar Soni (Gerrit)
Shikhar Soni added 3 comments![]( "Open in Gerrit")
bool* can_be_closed,Daco HarkesI sense some API design issue here: we have hooks for `dlopen` and `dlsym` but not for `dlclose`. I think a better way to do this would be to actually have a hook for `dlclose` which returns appropriate error on unclosable handles returned by `dlopen`.
Good point. If we someone adds a new operating system with different system calls, it would be a different close system call!
Related issue which mentions the need for having a dlclose in the embedder API: https://github.com/dart-lang/sdk/issues/55521
We'll need to add an extra bool to the DynamicLibrary class to register that it was created via the embedder API for code assets, so that close can forward to that.
Added a `dlcose` callback and a on `DyanimcLibrary` flag to remember when the handle came from `NativeAssetApi`, so close now goes back throught the embedder.
PTAL if this looks good.
It seems that this is only relevant for error reporting? Why have it then - let the embedder put `No asset ${id} found` into error.
Done
external factory DynamicLibrary.openFromAssetId(String assetId);Daco HarkesMaybe better name is just `DynamicLibrary.openAsset` or `DynamicLibrary.fromAsset` or `DynamicLibrary.asset` or something like this, `openFromAssetId` is a mouthful.
`DynamicLibrary.openCodeAsset` or `DynamicLibrary.codeAsset` to be consistent with https://dart.dev/tools/hooks and https://pub.dev/packages/code_assets terminology.
Maybe the latter.
- Daco Harkes
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shikhar Soni (Gerrit)
Shikhar Soni added 1 comment![]( "Open in Gerrit")
bool* can_be_closed,Daco HarkesI sense some API design issue here: we have hooks for `dlopen` and `dlsym` but not for `dlclose`. I think a better way to do this would be to actually have a hook for `dlclose` which returns appropriate error on unclosable handles returned by `dlopen`.
Shikhar SoniGood point. If we someone adds a new operating system with different system calls, it would be a different close system call!
Related issue which mentions the need for having a dlclose in the embedder API: https://github.com/dart-lang/sdk/issues/55521
We'll need to add an extra bool to the DynamicLibrary class to register that it was created via the embedder API for code assets, so that close can forward to that.
Added a `dlcose` callback and a on `DyanimcLibrary` flag to remember when the handle came from `NativeAssetApi`, so close now goes back throught the embedder.
PTAL if this looks good.
PS: Patchset 10
Related details
- Daco Harkes
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 2 comments![]( "Open in Gerrit")
} else if (!dlib.CanBeClosed()) {@veg...@google.com The original implementation stores `can_be_closed` in the Dart object. It would be kind of weird if we store it in the object for `DynamicLibrary.process()` but not for `DynamicLibrary.codeAsset(assetThatResolvesToProcess)`. Would you suggest to refactor the existing implementation to unconditionally call `close` as well?
That means that `Utils::UnloadDynamicLibrary` needs to do the closing of `DynamicLibrary.process` and `DynamicLibrary.executable()`.
We can check for `RTLD_DEFAULT` and `kWindowsDynamicLibraryProcessPtr` and `LoadDynamicLibrary(nullptr)` equality and return the error from `Utils::UnloadDynamicLibrary`.
if (dlib.UseNativeAssetsApi()) {Oh, good catch!
Related details
- Shikhar Soni
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 4 comments![]( "Open in Gerrit")
typedef void* (*Dart_NativeAssetsDlopenAssetIdV2)(const char* asset_id,This is now the same as `Dart_NativeAssetsDlopenAssetId` so you can just delete it and specify new behavior on `dlopen_v2`.
But the question is: does behavior actually meaningfully differ between `dlopen` and `dlopen_v2`? Because I think they behave pretty much the same.
} else if (!dlib.CanBeClosed()) {@veg...@google.com The original implementation stores `can_be_closed` in the Dart object. It would be kind of weird if we store it in the object for `DynamicLibrary.process()` but not for `DynamicLibrary.codeAsset(assetThatResolvesToProcess)`. Would you suggest to refactor the existing implementation to unconditionally call `close` as well?
That means that `Utils::UnloadDynamicLibrary` needs to do the closing of `DynamicLibrary.process` and `DynamicLibrary.executable()`.
We can check for `RTLD_DEFAULT` and `kWindowsDynamicLibraryProcessPtr` and `LoadDynamicLibrary(nullptr)` equality and return the error from `Utils::UnloadDynamicLibrary`.
I thought we would change `can_be_closed` field to become the function instead... To point to the close function which should be used (so that we could differentiate whether we need to call native assets API one or `Utils` one). If we want a short-cut we could make `nullptr` mean that it can't be closed.
bool canBeClosed_;Instead of having `canBeClosed_` and `useNativeAssetsApi_` we should just have `Dart_NativeAssetsDlcloseCallback dlclose_;`.
bool isClosed_;nit: style guide violation, must be `is_closed_` instead.
Related details
- Shikhar Soni
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 2 comments![]( "Open in Gerrit")
} else if (!dlib.CanBeClosed()) {Slava Egorov@veg...@google.com The original implementation stores `can_be_closed` in the Dart object. It would be kind of weird if we store it in the object for `DynamicLibrary.process()` but not for `DynamicLibrary.codeAsset(assetThatResolvesToProcess)`. Would you suggest to refactor the existing implementation to unconditionally call `close` as well?
That means that `Utils::UnloadDynamicLibrary` needs to do the closing of `DynamicLibrary.process` and `DynamicLibrary.executable()`.
We can check for `RTLD_DEFAULT` and `kWindowsDynamicLibraryProcessPtr` and `LoadDynamicLibrary(nullptr)` equality and return the error from `Utils::UnloadDynamicLibrary`.
I thought we would change `can_be_closed` field to become the function instead... To point to the close function which should be used (so that we could differentiate whether we need to call native assets API one or `Utils` one). If we want a short-cut we could make `nullptr` mean that it can't be closed.
We can't just blindly route it to the function for `DynamicLibrary`s created via the other constructors. They are created by the VM not by the embedder. So you the ones created via native assets can be routed to that function.
bool canBeClosed_;Instead of having `canBeClosed_` and `useNativeAssetsApi_` we should just have `Dart_NativeAssetsDlcloseCallback dlclose_;`.
No. `DynamicLibrary` created with `DynamicLibrary.open` are not created via the embedder API, so they should not be closed via the embedder API.
But we can close them via `Utils::UnloadDynamicLibrary` as long as that throws an error on trying to close the process and executable.
Related details
- Shikhar Soni
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
} else if (!dlib.CanBeClosed()) {Slava Egorov@veg...@google.com The original implementation stores `can_be_closed` in the Dart object. It would be kind of weird if we store it in the object for `DynamicLibrary.process()` but not for `DynamicLibrary.codeAsset(assetThatResolvesToProcess)`. Would you suggest to refactor the existing implementation to unconditionally call `close` as well?
That means that `Utils::UnloadDynamicLibrary` needs to do the closing of `DynamicLibrary.process` and `DynamicLibrary.executable()`.
We can check for `RTLD_DEFAULT` and `kWindowsDynamicLibraryProcessPtr` and `LoadDynamicLibrary(nullptr)` equality and return the error from `Utils::UnloadDynamicLibrary`.
Daco HarkesI thought we would change `can_be_closed` field to become the function instead... To point to the close function which should be used (so that we could differentiate whether we need to call native assets API one or `Utils` one). If we want a short-cut we could make `nullptr` mean that it can't be closed.
We can't just blindly route it to the function for `DynamicLibrary`s created via the other constructors. They are created by the VM not by the embedder. So you the ones created via native assets can be routed to that function.
We could add another `dlopen_legacy` to the embedder API and make all `DynamicLibrary.open` calls go there. And then route `DynamicLibrary.proccess` and `DynamicLibrary.executable` to the embedder API as well. Then everything would be embedder API.
Related details
- Shikhar Soni
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |