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

[ffigen] Dedupe ObjC listener block trampolines #1541

Merged
merged 9 commits into from
Sep 12, 2024
Merged

Conversation

liamappelbe
Copy link
Contributor

Listener blocks have an ObjC trampoline that increments their arg's ref counts before doing the async dispatch to the target isolate. At the moment we generate one trampoline per block signature. For example:

typedef void  (^ListenerBlock4)(NSString* );
ListenerBlock4 wrapListenerBlock_ObjCBlock_ffiVoid_NSString(ListenerBlock4 block) NS_RETURNS_RETAINED {
  return ^void(NSString* arg0) {
    block(objc_retain(arg0));
  };
}

This is unnecessarily specific. All we're doing is calling retain on the arg, so we could pass all the args as id (ObjC's version of Object):

typedef void  (^_ListenerTrampoline1)(id arg0);
_ListenerTrampoline1 _wrapListenerBlock1(_ListenerTrampoline1 block) NS_RETURNS_RETAINED {
  return ^void(id arg0) {
    block(objc_retain(arg0));
  };
}

This allows us to dedupe a lot of the trampolines, reducing the size of the bindings. It also works around some bugs related to these bindings not finding their imported types.

One wrinkle is that we still need to distinguish objects and blocks in these args, since they use different retain functions (objc_retain vs objc_retainBlock). So we need to make sure not to dedupe these. Also, since multiple blocks now share the same trampoline, we need to make sure to only generate its code once.

Fixes #1469.

Copy link

github-actions bot commented Sep 10, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️
File Coverage
pkgs/ffigen/lib/src/code_generator/library.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_block.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_interface.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_methods.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objc_block_parser.dart 💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ✔️
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart
Package publish validation ✔️
Package Version Status
package:ffi 2.1.3 already published at pub.dev
package:ffigen 14.1.0-wip WIP (no publish necessary)
package:jni 0.11.0 already published at pub.dev
package:jnigen 0.12.0-wip WIP (no publish necessary)
package:native_assets_cli 0.8.0-wip WIP (no publish necessary)
package:objective_c 2.0.0 already published at pub.dev
package:swift2objc 0.0.1-wip WIP (no publish necessary)
package:swiftgen 0.0.1-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

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

Are there no tests added/modified? Why?

CI seems to be failing.

@liamappelbe
Copy link
Contributor Author

Are there no tests added/modified? Why?

I've mostly just been testing behavior, and this change shouldn't affect the behavior at all. But I guess I should probably add a test that inspects the bindings.

CI seems to be failing.

Still investigating that. I haven't been able to repro locally. Looks similar to this flake (which I also can't repro), but it's more consistent.

@liamappelbe
Copy link
Contributor Author

CI seems to be failing.

Still investigating that. I haven't been able to repro locally. Looks similar to this flake (which I also can't repro), but it's more consistent.

CI failure was reproducible locally by running block_test and block_annotation_test in the same dart test process. Loaded C/ObjC symbols are always global, so we have to make sure they don't collide. The trampolines names used to include the types, but I removed that since it would be confusing now that they're being deduped (eg you'd have the NSString trampoline being used by NSObject blocks). The shorter names were colliding in these tests. To fix it I just added the ID hash to the name (it's not human readable, but it is short and simple).

@coveralls
Copy link

coveralls commented Sep 11, 2024

Coverage Status

coverage: 91.346% (+0.2%) from 91.104%
when pulling a80233f on dedupe_trampolines
into ee49231 on main.

@liamappelbe liamappelbe merged commit 69dd6d1 into main Sep 12, 2024
17 checks passed
@liamappelbe liamappelbe deleted the dedupe_trampolines branch September 12, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated ffi_bindings.dart.m doesn't handle platform differences
3 participants