[Bgen] Ensure that GC.KeepAlive is called in extension methods. #22433

mandel-macaque · 2025-03-20T00:24:51Z

This is an interesting change. After working on PR #22417 we noticed that we had a serious problem in how we called extension methods.

To understand the problem we first need to look at the code generation for an extension method:

[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]
internal static float _GetValue (ICIHueSaturationValueGradientProtocol This)
{
  return global::ObjCRuntime.Messaging.float_objc_msgSend (This.Handle, Selector.GetHandle ("value"));
}

The issue with the above code is that the GC can collect the 'This' object while is being used by the objc_msgSend method. For this type of methods to be safe we would need to perform a call to GC.KeepAlive on This.

Fixing the generator is not as easy as adding the 'GC.KeepAlive (This)' after the invocation because while that would work with a method that returns a wrapped object (an object that is a managed representation of an unmanaged object) it would not in all other case. The changes in the generator have to be:

For all methods that return a value, create a return variable with the following special cases:
- Regular methods that return values that are not wrapped.
- Methods that have been decorated with BindAs.
- Protocol constructors which are converted in Create methods.
Add the GC.KeepAlive call after the obj

With this commit, the generated code now looks like:

[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]
internal static float _GetValue (ICIHueSaturationValueGradientProtocol This)
{
  float ret;
  ret = global::ObjCRuntime.Messaging.float_objc_msgSend (This.Handle, Selector.GetHandle ("value"));
  GC.KeepAlive (This);
  return ret;
}

This change also opens the doors to fix in the future no extension methods that have parameters that are wrapped objects that do not return wrapped objects, but that is work for a different PR.

This is an interesting change. After working on PR #22417 we noticed that we had a serious problem in how we called extension methods. To understand the problem we first need to look at the code generation for an extension method: ```csharp [BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)] internal static float _GetValue (ICIHueSaturationValueGradientProtocol This) { return global::ObjCRuntime.Messaging.float_objc_msgSend (This.Handle, Selector.GetHandle ("value")); } ``` The issue with the above code is that the GC can collect the 'This' object while is being used by the objc_msgSend method. For this type of methods to be safe we would need to perform a call to GC.KeepAlive on This. Fixing the generator is not as easy as adding the 'GC.KeepAlive (This)' after the invocation because while that would work with a method that returns a wrapped object (an object that is a managed representation of an unmanaged object) it would not in all other case. The changes in the generator have to be: 1. For all methods that return a value, create a return variable with the following special cases: * Regular methods that return values that are not wrapped. * Methods that have been decorated with BindAs. * Protocol constructors which are converted in Create methods. 2. Add the GC.KeepAlive call after the obj With this commit, the generated code now looks like: ```csharp [BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)] internal static float _GetValue (ICIHueSaturationValueGradientProtocol This) { float ret; ret = global::ObjCRuntime.Messaging.float_objc_msgSend (This.Handle, Selector.GetHandle ("value")); GC.KeepAlive (This); return ret; } ``` This change also opens the doors to fix in the future no extension methods that have parameters that are wrapped objects that do not return wrapped objects, but that is work for a different PR.

rolfbjarne · 2025-03-20T07:12:27Z

src/bgen/Generator.cs

@@ -2958,7 +2958,7 @@ void GenerateInvoke (bool stret, bool supercall, MethodInfo mi, MemberInformatio
 			print ($"IntPtr {handleName};");
 			print ($"{handleName} = global::{NamespaceCache.Messaging}.IntPtr_objc_msgSend (Class.GetHandle (typeof (T)), Selector.GetHandle (\"alloc\"));");
 			print ($"{handleName} = {sig} ({handleName}, {selector_field}{args});");
-			print ($"{(assign_to_temp ? "ret = " : "return ")} global::ObjCRuntime.Runtime.GetINativeObject<T> ({handleName}, true);");
+			print ($"ret = global::ObjCRuntime.Runtime.GetINativeObject<T> ({handleName}, true);");


Looks like we don't need the assign_to_temp variable/parameter, so it can be removed.

rolfbjarne · 2025-03-20T07:15:00Z

src/bgen/Generator.cs

 					   cast_a, sig, target_name,
 					   handle,
 					   selector_field, args, cast_b);

 			if (postproc.Length > 0)
 				print (postproc.ToString ());
 		}
+
+		if (target_name == "This") {


IMHO this should not be condition on "This", but instead of whether the method is a an actual extension method. So further above:

var useExtensionMethod = (category_type is null && !minfo.is_extension_method && !minfo.is_protocol_implementation_method); string target_name = useExtensionMethod ? "this" : "This";

and then here:

Suggested change

if (target_name == "This") {

if (useExtensionMethod) {

Sure, but will be the opposite, useInstanceMethod:

var useExtensionMethod = (category_type is null && !minfo.is_extension_method && !minfo.is_protocol_implementation_method);

Is ensuring that we are not a category etc.. We could move it to an or, but I don't like to change bool operations just because.

ah, yes, flip the logic:

var useExtensionMethod = !(category_type is null && !minfo.is_extension_method && !minfo.is_protocol_implementation_method); string target_name = useExtensionMethod ? "This" : "this";

boolean logic is hard!

rolfbjarne · 2025-03-20T07:15:40Z

src/bgen/Generator.cs

+				print ("{0} ret;", TypeManager.FormatType (minfo.type, mi.ReturnType));
+			}
+		} else if (minfo.is_ctor && minfo.is_protocol_member) {
+			print ("// {0} is protocol {1} ", mi.Name, minfo.is_protocol_member);


Debug statement?

src/bgen/Generator.cs

Co-authored-by: Rolf Bjarne Kvinge <[email protected]>

github-actions · 2025-03-20T16:19:32Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

src/bgen/Generator.cs

dalexsoto

👍 after Rolf's comments are addressed

Co-authored-by: Rolf Bjarne Kvinge <[email protected]>

vs-mobiletools-engineering-service2 · 2025-03-21T13:21:28Z

✅ [CI Build #cddc790] Build passed (Build packages) ✅

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T13:22:53Z

✅ [PR Build #cddc790] Build passed (Detect API changes) ✅

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T13:31:06Z

✅ [CI Build #cddc790] Build passed (Build macOS tests) ✅

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T13:45:04Z

💻 [CI Build #cddc790] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T13:45:54Z

💻 [CI Build #cddc790] Tests on macOS arm64 - Mac Sequoia (15) passed 💻

✅ All tests on macOS arm64 - Mac Sequoia (15) passed.

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T13:46:30Z

💻 [CI Build #cddc790] Tests on macOS M1 - Mac Ventura (13) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T13:46:54Z

💻 [CI Build #cddc790] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T13:49:13Z

✅ API diff for current PR / commit

.NET ( No breaking changes )

iOS: vsdrops gist ( No breaking changes )
tvOS: vsdrops gist ( No breaking changes )
MacCatalyst: vsdrops gist ( No breaking changes )
macOS: vsdrops gist ( No breaking changes )

❗ API diff vs stable (Breaking changes)

.NET ( ❗ Breaking changes ❗ )

iOS: vsdrops gist ( ❗ Breaking changes ❗ )
tvOS: vsdrops gist ( ❗ Breaking changes ❗ )
MacCatalyst: vsdrops gist ( ❗ Breaking changes ❗ )
macOS: vsdrops gist ( ❗ Breaking changes ❗ )

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) Unable to create gist: Response status code does not indicate success: 422 (Unprocessable Entity). (raw diff) - Please review changes)

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

vs-mobiletools-engineering-service2 · 2025-03-21T19:32:09Z

🚀 [CI Build #cddc790] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 117 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 2 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 5 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 4 tests passed. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 44 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 10 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 9 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 8 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ windows: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: cddc79057ef803c6a4e93e3c7b6a8383a57ce8d7 [PR build]

filipnavara · 2025-03-23T20:40:51Z

Thanks!

mandel-macaque mentioned this pull request Mar 20, 2025

[rgen] Enable analysis of generated code in the native object handle analyzer. #22417

Draft