[ffigen] Migrate to ARC #1441

liamappelbe · 2024-08-22T03:48:30Z

Migrate package:objective_c, ffigen's generated block trampolines, and all the objective_c tests to ARC.

The generated block trampolines have changed from

typedef void  (^ListenerBlock1)(DummyObject* );
ListenerBlock1 wrapListenerBlock_ObjCBlock_ffiVoid_DummyObject(ListenerBlock1 block) {
  ListenerBlock1 wrapper = [^void(DummyObject* arg0) {
    block([arg0 retain]);
  } copy];
  [block release];
  return wrapper;
}

to

typedef void  (^ListenerBlock1)(DummyObject* );
ListenerBlock1 wrapListenerBlock_ObjCBlock_ffiVoid_DummyObject(ListenerBlock1 block) NS_RETURNS_RETAINED {
  return ^void(DummyObject* arg0) {
    block(objc_retain(arg0));
  };
}

We have to retain the args using objc_retain/objc_retainBlock instead of sending a retain message. Also, instead of explicitly copying the wrapper block, we can just add a NS_RETURNS_RETAINED annotation and rely on ARC to copy it for us. Also, instead of releasing the reference to the incoming block in here, we're doing it in Dart land instead.

Other changes:

Added a compile time check to the trampolines file to make sure it's compiled with ARC.
Switched from retaining blocks using copy messages and _Block_copy to objc_retainBlock, which is more stable. Also switched from releasing blocks using _Block_release to the ordinary objc_release used for all objects.
All the ffigen tests have been migrated to ARC except for the old automated_ref_count_test, which has been split into an ARC and non-ARC version (arc_test and ref_count_test). The old name was a bit of a misnomer.
Added a github workflow that verifies that the package:objective_c example builds, to make sure the podspecs are correct (the tests build the package test/setup.dart, so the flutter build path was untested).

Fixes #1421
Follow up work: #1446

github-actions · 2024-08-22T03:49:59Z

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/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/pointer.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/writer.dart	💔 Not covered
pkgs/objective_c/lib/objective_c.dart	💔 Not covered
pkgs/objective_c/lib/src/c_bindings_generated.dart	💔 Not covered
pkgs/objective_c/lib/src/internal.dart	💔 Not covered
pkgs/objective_c/lib/src/objective_c_bindings_generated.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/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_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

coveralls · 2024-08-22T03:53:32Z

coverage: 91.913% (-0.05%) from 91.962%
when pulling 32a38dc on arc
into 3af07fd on main.

dcharkes

Also, instead of releasing the reference to the incoming block in here, we're doing it in Dart land instead.

What is the effect of this?

Added a github workflow that verifies that the package:objective_c example builds, to make sure the podspecs are correct (the tests build the package test/setup.dart, so the flutter build path was untested).

Probably leave a TODO there for switching to native assets, as then we don't use podspecs anymore.

Fixes #1421
Follow up work: #1446

Do we end up in kind of a half-state here? E.g. we don't support non ARC anymore, but for ARC we ignore a bunch of things which would lead to memory errors. Should #1446 be folded in to this PR? Or is the current PR usable? If so in what cases? And how do users know?

(Side note: Should we have some way of specifying in our workflow that something should be addressed before the next version is released? Maybe we need a release-blocker issue label? Or, is the tracker of when we want to release the next version enough? As long as you're the person mostly working on FFIgen, I guess it's fine to leave FFIgen in a unpublishable state on the main branch, but it does prevent us from addressing other bugs and doing a release in the meantime.)

The code changes in this PR, LGTM.
So, LGTM once above question is answered.

dcharkes · 2024-08-23T07:26:22Z

pkgs/objective_c/CHANGELOG.md

@@ -3,6 +3,7 @@
 - Drop API methods that are deprecated in the oldest versions of iOS and macOS
  that flutter supports.
 - Added `ObjCBlock`, which is the new user-facing representation of ObjC blocks.
+- Migrate to ARC.


Maybe spell out the acronym in case a layman comes along.

dcharkes · 2024-08-23T07:33:12Z

pkgs/objective_c/example/macos/Podfile.lock

@@ -0,0 +1,22 @@
+PODS:


Is committing this intentional?

kekland · 2024-08-23T16:35:27Z

Somehow after generating code I got one wrapper function that was still using the retain keyword:

#include <stdint.h>
#import <CoreMIDI/CoreMIDI.h>
#import <CloudKit/CloudKit.h>
#import <CoreFoundation/CoreFoundation.h>
#import <AVFoundation/AVFoundation.h>
#import <AVFoundation/AVAssetExportSession.h>
#import <AVFoundation/AVAssetTrackSegment.h>
#import <AVFoundation/AVAssetImageGenerator.h>
#import <AVFoundation/AVMediaFormat.h>
#import <AVFoundation/AVVideoComposition.h>
#import <CoreGraphics/CGBase.h>
#import <CoreGraphics/CGGeometry.h>
#import <CoreGraphics/CGImage.h>
#import <CoreMedia/CMBase.h>
#import <CoreMedia/CMTime.h>
#import <CoreMedia/CMTimeRange.h>
#import <QuartzCore/CADisplayLink.h>

#if !__has_feature(objc_arc)
#error "This file must be compiled with ARC enabled"
#endif

id objc_retain(id);
id objc_retainBlock(id);

typedef void  (^ListenerBlock)(NSDictionary* , struct _NSRange , BOOL * );
ListenerBlock wrapListenerBlock_ObjCBlock_ffiVoid_NSDictionary_NSRange_bool(ListenerBlock block) NS_RETURNS_RETAINED {
  return ^void(NSDictionary* arg0, struct _NSRange arg1, BOOL * arg2) {
    block(objc_retain(arg0), arg1, arg2);
  };
}

//
// The block below has the issue
//

typedef void  (^ListenerBlock1)(id , struct _NSRange , BOOL * );
ListenerBlock1 wrapListenerBlock_ObjCBlock_ffiVoid_objcObjCObject_NSRange_bool(ListenerBlock1 block) NS_RETURNS_RETAINED {
  return ^void(id arg0, struct _NSRange arg1, BOOL * arg2) {
    block([arg0 retain], arg1, arg2);  // <--- here
  };
}

typedef void  (^ListenerBlock2)(NSDate* , BOOL , BOOL * );
ListenerBlock2 wrapListenerBlock_ObjCBlock_ffiVoid_NSDate_bool_bool(ListenerBlock2 block) NS_RETURNS_RETAINED {
  return ^void(NSDate* arg0, BOOL arg1, BOOL * arg2) {
    block(objc_retain(arg0), arg1, arg2);
  };
}

// bunch more below, but everything is fine there

Not sure which interface led to the generation of this. If needed, I can share ffigen.yaml

liamappelbe · 2024-08-26T00:14:56Z

Somehow after generating code I got one wrapper function that was still using the retain keyword:

Oops. Good catch. Forgot about id. Thanks.

liamappelbe · 2024-08-26T00:28:15Z

Also, instead of releasing the reference to the incoming block in here, we're doing it in Dart land instead.

What is the effect of this?

It doesn't make a functional difference, but it means one less ARC hack in the ObjC bindings.

Added a github workflow that verifies that the package:objective_c example builds, to make sure the podspecs are correct (the tests build the package test/setup.dart, so the flutter build path was untested).

Probably leave a TODO there for switching to native assets, as then we don't use podspecs anymore.

Done.

Fixes #1421
Follow up work: #1446

Do we end up in kind of a half-state here? E.g. we don't support non ARC anymore, but for ARC we ignore a bunch of things which would lead to memory errors. Should #1446 be folded in to this PR? Or is the current PR usable? If so in what cases? And how do users know?

This isn't a regression. The migration to ARC just made some existing bugs more obvious. The way we're not quite handling method families correctly is a very obscure issue that users will probably never actually hit. The bigger issue is the way we are assuming all blocks that return ref countable objects are NS_RETURNS_RETAINED. That could lead to use after free issues. But in practice, blocks that return objects aren't very common (most blocks are completion handlers that return void), which is why no users have ever reported this issue. But I'll do that follow up in the same 14.0.0 release, so users won't see the half-state anyway.

Side note: Should we have some way of specifying in our workflow that something should be addressed before the next version is released?

I've been using milestones. Eg, #1446 is in the ffigen 14.0.0 milestone.

liamappelbe added 7 commits August 14, 2024 17:34

Migrate package:objective_c to ARC

667d059

Migrated block trampolines (sort of)

ef21a26

WIP

bb1fb2b

Fix block_test

2f5e95f

Some cleanup

ec9757f

Fix proxy

e3a6a19

Rename automated_ref_count_test to ref_count_test and opt it out of ARC

e4f08d3

github-actions bot added type-infra A repository infrastructure change or enhancement package:ffigen package:objective_c labels Aug 22, 2024

liamappelbe changed the title ~~WIP: Migrate package:objective_c and ffigen~~ WIP: Migrate package:objective_c and ffigen's ObjC code to ARC Aug 22, 2024

liamappelbe marked this pull request as draft August 22, 2024 03:48

Migrate static_func test

ac0bdbb

liamappelbe mentioned this pull request Aug 23, 2024

[ffigen] Improve handling of ObjC method families and retention annotations #1446

Closed

liamappelbe added 5 commits August 23, 2024 13:30

Fix build-example

1a1c5f2

Merge branch 'main' into arc

0a41857

Changelog and test fixes

5433d71

Another test

08079f1

fmt

89d3f81

liamappelbe added the skip-version-check label Aug 23, 2024

liamappelbe changed the title ~~WIP: Migrate package:objective_c and ffigen's ObjC code to ARC~~ [ffigen] Migrate to ARC Aug 23, 2024

Remove dead code

fc71564

liamappelbe marked this pull request as ready for review August 23, 2024 05:11

liamappelbe requested a review from dcharkes August 23, 2024 05:11

dcharkes approved these changes Aug 23, 2024

View reviewed changes

Comments

32a38dc

liamappelbe merged commit 3d80f02 into main Aug 26, 2024
21 of 22 checks passed

liamappelbe deleted the arc branch August 26, 2024 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ffigen] Migrate to ARC #1441

[ffigen] Migrate to ARC #1441

liamappelbe commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

coveralls commented Aug 22, 2024 •

edited

Loading

dcharkes left a comment •

edited

Loading

dcharkes Aug 23, 2024

liamappelbe Aug 26, 2024

dcharkes Aug 23, 2024

liamappelbe Aug 26, 2024

kekland commented Aug 23, 2024

liamappelbe commented Aug 26, 2024

liamappelbe commented Aug 26, 2024

[ffigen] Migrate to ARC #1441

[ffigen] Migrate to ARC #1441

Conversation

liamappelbe commented Aug 22, 2024 • edited Loading

github-actions bot commented Aug 22, 2024 • edited Loading

PR Health

coveralls commented Aug 22, 2024 • edited Loading

dcharkes left a comment • edited Loading

Choose a reason for hiding this comment

dcharkes Aug 23, 2024

Choose a reason for hiding this comment

liamappelbe Aug 26, 2024

Choose a reason for hiding this comment

dcharkes Aug 23, 2024

Choose a reason for hiding this comment

liamappelbe Aug 26, 2024

Choose a reason for hiding this comment

kekland commented Aug 23, 2024

liamappelbe commented Aug 26, 2024

liamappelbe commented Aug 26, 2024

liamappelbe commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

coveralls commented Aug 22, 2024 •

edited

Loading

dcharkes left a comment •

edited

Loading