- Notifications
You must be signed in to change notification settings - Fork543
[Bgen] Ensure that GC.KeepAlive is called in extension methods.#22433
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is an interesting change. After working on PR#22417 we noticed that we had aserious problem in how we called extension methods.To understand the problem we first need to look at the code generationfor 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' objectwhile is being used by the objc_msgSend method. For this type of methods to besafe 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 thatreturns a wrapped object (an object that is a managed representation ofan unmanaged object) it would not in all other case. The changes in thegenerator 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 objWith 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 extensionmethods that have parameters that are wrapped objects that do notreturn wrapped objects, but that is work for a different PR.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -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);"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like we don't need theassign_to_temp variable/parameter, so it can be removed.
src/bgen/Generator.cs Outdated
| cast_a, sig, target_name, | ||
| handle, | ||
| selector_field, args, cast_b); | ||
| if (postproc.Length > 0) | ||
| print (postproc.ToString ()); | ||
| } | ||
| if (target_name == "This") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO this should not be condition on"This", but instead of whether the method is a an actual extension method. So further above:
varuseExtensionMethod=(category_typeisnull&&!minfo.is_extension_method&&!minfo.is_protocol_implementation_method);stringtarget_name=useExtensionMethod?"this":"This";
and then here:
| if(target_name=="This"){ | |
| if(useExtensionMethod){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, but will be the opposite, useInstanceMethod:
varuseExtensionMethod=(category_typeisnull&&!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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah, yes, flip the logic:
varuseExtensionMethod=!(category_typeisnull&&!minfo.is_extension_method&&!minfo.is_protocol_implementation_method);stringtarget_name=useExtensionMethod?"This":"this";
boolean logic is hard!
src/bgen/Generator.cs Outdated
| 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Debug statement?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 after Rolf's comments are addressed
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
✅ [CI Build #cddc790] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #cddc790] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [CI Build #cddc790] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #cddc790] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [CI Build #cddc790] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #cddc790] Tests on macOS M1 - Mac Ventura (13) passed 💻✅All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
💻 [CI Build #cddc790] Tests on macOS M1 - Mac Monterey (12) passed 💻✅All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
✅ API diff for current PR / commit.NET ( No breaking changes )❗ API diff vs stable (Breaking changes).NET ( ❗ Breaking changes ❗ )ℹ️ Generator diffGenerator 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 |
This comment has been minimized.
This comment has been minimized.
🚀 [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 Pipeline on Agent |
6b7892a intomainUh oh!
There was an error while loading.Please reload this page.
Thanks! |
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:
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:
With this commit, the generated code now looks like:
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.