Skip to content

FFI creates closures which are not present in the kernel program #54172

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

Closed
alexmarkov opened this issue Nov 28, 2023 · 1 comment
Closed

FFI creates closures which are not present in the kernel program #54172

alexmarkov opened this issue Nov 28, 2023 · 1 comment
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team

Comments

@alexmarkov
Copy link
Contributor

NativeFunctionPointer.asFunction and DynamicLibraryExtension.lookupFunction from dart:ffi are implemented using opaque _asFunctionInternal which is declared in the following way:

@pragma("vm:recognized", "other")
@pragma("vm:external-name", "Ffi_asFunctionInternal")
external DS _asFunctionInternal<DS extends Function, NS extends Function>(
    Pointer<NativeFunction<NS>> ptr, bool isLeaf);

_asFunctionInternal can magically create and return closures which are not present in the kernel program. As a result, whole-program analysis (TFA) cannot see all possible closures in the program which hinders its ability to analyze possible targets of closure calls (this issue blocks #39692).

I see the following possible solutions:

  • Change FFI transformation to create those closures explicitly in kernel AST.
  • Replace those closures with tear-offs of (external) trampoline methods, taken explicitly in kernel AST.
  • Recognize and handle _asFunctionInternal specially during TFA (maybe add a special pragma for that purpose).

@dcharkes @mkustermann Thoughts?

@alexmarkov alexmarkov added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Nov 28, 2023
@dcharkes
Copy link
Contributor

👍 for addressing this.

Change FFI transformation to create those closures explicitly in kernel AST.

I guess the challenge is to wire up what we would create in the kernel AST with what's constructed in BuildFfiAsFunctionInternal and BuildFfiAsFunctionInternalCall. If we create a closure in kernel already, then we need to mark it somehow so that we can recognize it (for example with a pragma such as IsCachableIdempotent, instead of as a recognized method like it is now).

Nothing pops up in my mind why creating the closures in kernel wouldn't work, but I haven't paged everything atm.

Replace those closures with tear-offs of (external) trampoline methods, taken explicitly in kernel AST.

Are you saying you want to create a kernel node of a tear-off invocation? And then wire that up BuildFfiAsFunctionInternal and BuildFfiAsFunctionInternalCall? What's the difference with option 1? Or is it mostly similar? Shouldn't the closure in option 1 also be external or have a bogus body?

Or did you mean something completely different with this option.

Recognize and handle _asFunctionInternal specially during TFA (maybe add a special pragma for that purpose).

That seems like the least clean solution. However, it would allow us to leave FFI internals untouched and might be reusable for other things. It would be kind of similar in nature to the _nativeEffect.

I don't have a strong opinion about which option to choose I think.

@a-siva a-siva added P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team labels Nov 30, 2023
copybara-service bot pushed a commit that referenced this issue Dec 15, 2023
This reverts commit 1800039.

The changes fd2e9b9 and
c20f9ea are relanded as is.

Reason for revert was fixed separately in
https://dart-review.googlesource.com/c/sdk/+/341621

TEST=ci

CoreLibraryReviewExempt: Implementation change only.
Issue: #54172
Issue: #39692
Change-Id: I1a2324768502e5ffbce328127938c0d3c96c38ba
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/341642
Reviewed-by: Siva Annamalai <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>
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 P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

3 participants