From 81351632d57c29032481be19ff1d660331215182 Mon Sep 17 00:00:00 2001 From: Joe Hull Date: Fri, 21 Jan 2022 02:44:04 -0800 Subject: [PATCH] Use WeakReference in NativeToManagedMap to allow finalizers to run. (#1648) --- src/Generator/Generators/C/CppHeaders.cs | 2 +- src/Generator/Generators/C/CppSources.cs | 2 +- src/Generator/Generators/CLI/CLIHeaders.cs | 2 +- src/Generator/Generators/CLI/CLISources.cs | 2 +- .../Generators/CSharp/CSharpSources.cs | 65 +++++++++++++++---- src/Generator/Generators/CodeGenerator.cs | 2 + src/Generator/Options.cs | 36 +++++++++- tests/CSharp/CSharp.CSharp.csproj | 6 +- tests/CSharp/CSharp.Gen.cs | 4 ++ tests/CSharp/CSharp.Tests.cs | 61 +++++++++++++++++ tests/CSharp/CSharp.h | 5 ++ tests/CSharp/CSharpPartialMethods.cs | 16 +++++ 12 files changed, 184 insertions(+), 19 deletions(-) create mode 100644 tests/CSharp/CSharpPartialMethods.cs diff --git a/src/Generator/Generators/C/CppHeaders.cs b/src/Generator/Generators/C/CppHeaders.cs index 63ce8b94..ef0ba7c1 100644 --- a/src/Generator/Generators/C/CppHeaders.cs +++ b/src/Generator/Generators/C/CppHeaders.cs @@ -469,7 +469,7 @@ namespace CppSharp.Generators.Cpp if (destructor != null) { GenerateClassDestructor(@class); - if (Options.GenerateFinalizers) + if (Options.GenerateFinalizerFor(@class)) GenerateClassFinalizer(@class); } } diff --git a/src/Generator/Generators/C/CppSources.cs b/src/Generator/Generators/C/CppSources.cs index b4ed3719..ff298bb8 100644 --- a/src/Generator/Generators/C/CppSources.cs +++ b/src/Generator/Generators/C/CppSources.cs @@ -173,7 +173,7 @@ namespace CppSharp.Generators.Cpp { GenerateClassDestructor(@class); - if (Options.GenerateFinalizers) + if (Options.GenerateFinalizerFor(@class)) GenerateClassFinalizer(@class); } } diff --git a/src/Generator/Generators/CLI/CLIHeaders.cs b/src/Generator/Generators/CLI/CLIHeaders.cs index e71e77c1..1d0ea407 100644 --- a/src/Generator/Generators/CLI/CLIHeaders.cs +++ b/src/Generator/Generators/CLI/CLIHeaders.cs @@ -400,7 +400,7 @@ namespace CppSharp.Generators.CLI if (destructor != null) { GenerateClassDestructor(@class); - if (Options.GenerateFinalizers) + if (Options.GenerateFinalizerFor(@class)) GenerateClassFinalizer(@class); } } diff --git a/src/Generator/Generators/CLI/CLISources.cs b/src/Generator/Generators/CLI/CLISources.cs index 06f512c3..f24d04c7 100644 --- a/src/Generator/Generators/CLI/CLISources.cs +++ b/src/Generator/Generators/CLI/CLISources.cs @@ -193,7 +193,7 @@ namespace CppSharp.Generators.CLI if (destructor != null) { GenerateClassDestructor(@class); - if (Options.GenerateFinalizers) + if (Options.GenerateFinalizerFor(@class)) GenerateClassFinalizer(@class); } } diff --git a/src/Generator/Generators/CSharp/CSharpSources.cs b/src/Generator/Generators/CSharp/CSharpSources.cs index 293ad818..882a3a95 100644 --- a/src/Generator/Generators/CSharp/CSharpSources.cs +++ b/src/Generator/Generators/CSharp/CSharpSources.cs @@ -456,9 +456,44 @@ namespace CppSharp.Generators.CSharp var @interface = @class.Namespace.Classes.FirstOrDefault( c => c.IsInterface && c.OriginalClass == @class); var printedClass = (@interface ?? @class).Visit(TypePrinter); - var dict = $@"global::System.Collections.Concurrent.ConcurrentDictionary"; + var valueType = Options.GenerateFinalizerFor(@class) ? $"global::System.WeakReference<{printedClass}>" : $"{printedClass}"; + var dict = $@"global::System.Collections.Concurrent.ConcurrentDictionary"; + WriteLine("internal static readonly {0} NativeToManagedMap = new {0}();", dict); + PopBlock(NewLineKind.BeforeNextBlock); + + // Create a method to record/recover a mapping to make it easier to call from template instantiation + // implementations. If finalizers are in play, use weak references; otherwise, the finalizer + // will never get invoked: unless the managed object is Disposed, there will always be a reference + // in the dictionary. + PushBlock(BlockKind.Method); + if (Options.GenerateFinalizerFor(@class)) + { + WriteLines($@" +internal static void {Helpers.RecordNativeToManagedMappingIdentifier}(IntPtr native, {printedClass} managed) +{{ + NativeToManagedMap[native] = new global::System.WeakReference<{printedClass}>(managed); +}} + +internal static bool {Helpers.TryGetNativeToManagedMappingIdentifier}(IntPtr native, out {printedClass} managed) +{{ + managed = default; + return NativeToManagedMap.TryGetValue(native, out var wr) && wr.TryGetTarget(out managed); +}}"); + } + else + { + WriteLines($@" +internal static void {Helpers.RecordNativeToManagedMappingIdentifier}(IntPtr native, {printedClass} managed) +{{ + NativeToManagedMap[native] = managed; +}} + +internal static bool {Helpers.TryGetNativeToManagedMappingIdentifier}(IntPtr native, out {printedClass} managed) +{{ + return NativeToManagedMap.TryGetValue(native, out managed); +}}"); + } } PopBlock(NewLineKind.BeforeNextBlock); } @@ -2194,7 +2229,9 @@ namespace CppSharp.Generators.CSharp if (@class.IsRefType) { - GenerateClassFinalizer(@class); + // We used to always call GenerateClassFinalizer here. However, the + // finalizer calls Dispose which is conditionally implemented below. + // Instead, generate the finalizer only if Dispose is also implemented. // ensure any virtual dtor in the chain is called var dtor = @class.Destructors.FirstOrDefault(d => d.Access != AccessSpecifier.Private); @@ -2203,13 +2240,17 @@ namespace CppSharp.Generators.CSharp // virtual destructors in abstract classes may lack a pointer in the v-table // so they have to be called by symbol; thus we need an explicit Dispose override @class.IsAbstract) - if(!@class.IsOpaque)GenerateDisposeMethods(@class); + if (!@class.IsOpaque) + { + GenerateClassFinalizer(@class); + GenerateDisposeMethods(@class); + } } } - private void GenerateClassFinalizer(INamedDecl @class) + private void GenerateClassFinalizer(Class @class) { - if (!Options.GenerateFinalizers) + if (!Options.GenerateFinalizerFor(@class)) return; PushBlock(BlockKind.Finalizer); @@ -2234,7 +2275,7 @@ namespace CppSharp.Generators.CSharp WriteOpenBraceAndIndent(); WriteLine($"Dispose(disposing: true, callNativeDtor : { Helpers.OwnsNativeInstanceIdentifier} );"); - if (Options.GenerateFinalizers) + if (Options.GenerateFinalizerFor(@class)) WriteLine("GC.SuppressFinalize(this);"); UnindentAndWriteCloseBrace(); @@ -2401,11 +2442,11 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetOrCreateInst {{ if (native == {TypePrinter.IntPtrType}.Zero) return null; - if (NativeToManagedMap.TryGetValue(native, out var managed)) + if ({Helpers.TryGetNativeToManagedMappingIdentifier}(native, out var managed)) return ({printedClass})managed; var result = {Helpers.CreateInstanceIdentifier}(native, skipVTables); if (saveInstance) - NativeToManagedMap[native] = result; + {Helpers.RecordNativeToManagedMappingIdentifier}(native, result); return result; }}"); NewLine(); @@ -2417,7 +2458,7 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetOrCreateInst WriteLines($@" internal static{(@new ? " new" : string.Empty)} {printedClass} __GetInstance({TypePrinter.IntPtrType} native) {{ - if (!NativeToManagedMap.TryGetValue(native, out var managed)) + if (!{Helpers.TryGetNativeToManagedMappingIdentifier}(native, out var managed)) throw new global::System.Exception(""No managed instance was found""); var result = ({printedClass})managed; if (result.{Helpers.OwnsNativeInstanceIdentifier}) @@ -2534,7 +2575,7 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetInstance({Ty if (@class.IsRefType) { WriteLine($"{Helpers.OwnsNativeInstanceIdentifier} = true;"); - WriteLine($"NativeToManagedMap[{Helpers.InstanceIdentifier}] = this;"); + WriteLine($"{Helpers.RecordNativeToManagedMappingIdentifier}({Helpers.InstanceIdentifier}, this);"); } else { @@ -2979,7 +3020,7 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetInstance({Ty @class.IsAbstractImpl ? @class.BaseClass : @class); WriteLine($"{Helpers.InstanceIdentifier} = Marshal.AllocHGlobal(sizeof({@internal}));"); WriteLine($"{Helpers.OwnsNativeInstanceIdentifier} = true;"); - WriteLine($"NativeToManagedMap[{Helpers.InstanceIdentifier}] = this;"); + WriteLine($"{Helpers.RecordNativeToManagedMappingIdentifier}({Helpers.InstanceIdentifier}, this);"); if (method.IsCopyConstructor) { diff --git a/src/Generator/Generators/CodeGenerator.cs b/src/Generator/Generators/CodeGenerator.cs index fb7b629c..758d076b 100644 --- a/src/Generator/Generators/CodeGenerator.cs +++ b/src/Generator/Generators/CodeGenerator.cs @@ -1302,6 +1302,8 @@ namespace CppSharp.Generators public static readonly string CreateInstanceIdentifier = Generator.GeneratedIdentifier("CreateInstance"); public static readonly string GetOrCreateInstanceIdentifier = Generator.GeneratedIdentifier("GetOrCreateInstance"); + public static readonly string RecordNativeToManagedMappingIdentifier = Generator.GeneratedIdentifier("RecordNativeToManagedMapping"); + public static readonly string TryGetNativeToManagedMappingIdentifier = Generator.GeneratedIdentifier("TryGetNativeToManagedMapping"); public static string GetSuffixForInternal(Class @class) { diff --git a/src/Generator/Options.cs b/src/Generator/Options.cs index 6ea8d067..4ce91c85 100644 --- a/src/Generator/Options.cs +++ b/src/Generator/Options.cs @@ -114,11 +114,43 @@ namespace CppSharp public List NoGenIncludeDirs; /// - /// Enable this option to enable generation of finalizers. - /// Works in both CLI and C# backends. + /// Enable this option to enable generation of finalizers. Works in both CLI and + /// C# backends. /// + /// + /// Use to specify a filter so that + /// finalizers are generated for only a subset of classes. + /// public bool GenerateFinalizers; + /// + /// A filter that can restrict the classes for which finalizers are generated when + /// is true. + /// + /// + /// The default filter performs no filtering so that whenever is true, finalizers will be generated for + /// all classes. If finalizers should be generated selectively, inspect the + /// supplied parameter and return true if a finalizer + /// should be generated and false if a finalizer should not be generated. + /// + public Func GenerateFinalizersFilter = (@class) => true; + + /// + /// An internal convenience method that combines the effect of and . + /// + /// + /// The to be filtered by . + /// + /// + /// true if a finalizer should be generated for . + /// false if a finalizer should not be generated for . + /// + internal bool GenerateFinalizerFor(Class @class) => GenerateFinalizers && GenerateFinalizersFilter(@class); + /// /// If returns true for a given , /// unmanaged memory allocated in the class' default constructor will be initialized with diff --git a/tests/CSharp/CSharp.CSharp.csproj b/tests/CSharp/CSharp.CSharp.csproj index a6968989..a2535230 100644 --- a/tests/CSharp/CSharp.CSharp.csproj +++ b/tests/CSharp/CSharp.CSharp.csproj @@ -1 +1,5 @@ - \ No newline at end of file + + + + + \ No newline at end of file diff --git a/tests/CSharp/CSharp.Gen.cs b/tests/CSharp/CSharp.Gen.cs index aca08822..d44d791e 100644 --- a/tests/CSharp/CSharp.Gen.cs +++ b/tests/CSharp/CSharp.Gen.cs @@ -66,6 +66,10 @@ namespace CppSharp.Tests Namespace = ctx.TranslationUnits.First(u => u.IsValid && !u.IsSystemHeader) }; + // Generate a finalizer for just the one test case. + driver.Options.GenerateFinalizers = true; + driver.Options.GenerateFinalizersFilter = (@class) => @class.Name == "TestFinalizer"; + // Preserve the original semantics except for our one test class. driver.Options.ZeroAllocatedMemory = (@class) => @class.Name == "ClassZeroAllocatedMemoryTest"; } diff --git a/tests/CSharp/CSharp.Tests.cs b/tests/CSharp/CSharp.Tests.cs index 76e7c06a..33fecdd5 100644 --- a/tests/CSharp/CSharp.Tests.cs +++ b/tests/CSharp/CSharp.Tests.cs @@ -520,6 +520,67 @@ public unsafe class CSharpTests Assert.That(CallDtorVirtually.Destroyed, Is.False); } + // Verify that the finalizer gets called if an instance is not disposed. Note that + // we've arranged to have the generator turn on finalizers for (just) the + // TestFinalizer class. + [Test] + public void TestFinalizerGetsCalledWhenNotDisposed() + { + using var callbackRegistration = new TestFinalizerDisposeCallbackRegistration(); + var nativeAddr = CreateAndReleaseTestFinalizerInstance(false); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + + Assert.That(callbackRegistration.NativeAddr, Is.EqualTo(nativeAddr)); + Assert.That(callbackRegistration.IsDisposing, Is.False); + } + + // Verify that the finalizer is not called if an instance is disposed. Note that we've + // arranged to have the generator turn on finalizers for (just) the TestFinalizer + // class. + [Test] + public void TestFinalizerNotCalledWhenDisposed() + { + using var callbackRegistration = new TestFinalizerDisposeCallbackRegistration(); + var nativeAddr = CreateAndReleaseTestFinalizerInstance(true); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + + Assert.That(callbackRegistration.NativeAddr, Is.EqualTo(nativeAddr)); + Assert.That(callbackRegistration.IsDisposing, Is.True); + } + + // Empirically, the finalizer won't release a reference until the stack frame in which + // the reference was created has been released. Use this method to create/release the + // instance. + private Int64 CreateAndReleaseTestFinalizerInstance(bool dispose) + { + var instance = new TestFinalizer(); + Assert.That(instance.__Instance, !Is.EqualTo(IntPtr.Zero)); + var nativeAddr = instance.__Instance.ToInt64(); + if (dispose) instance.Dispose(); + return nativeAddr; + } + + class TestFinalizerDisposeCallbackRegistration : IDisposable + { + public bool? IsDisposing = null; + public long? NativeAddr = null; + + public TestFinalizerDisposeCallbackRegistration() + { + TestFinalizer.DisposeCallback = (isDisposing, intPtr) => + { + IsDisposing = isDisposing; + NativeAddr = intPtr.ToInt64(); + }; + } + + public void Dispose() => TestFinalizer.DisposeCallback = null; + } + [Test] public void TestParamTypeToInterfacePass() { diff --git a/tests/CSharp/CSharp.h b/tests/CSharp/CSharp.h index 4ad3b453..3a222386 100644 --- a/tests/CSharp/CSharp.h +++ b/tests/CSharp/CSharp.h @@ -1131,6 +1131,11 @@ public: const char16_t* RetrieveString(); }; +class DLL_API TestFinalizer +{ +public: + const int Data[1024]; +}; DLL_API void decltypeFunctionPointer(); diff --git a/tests/CSharp/CSharpPartialMethods.cs b/tests/CSharp/CSharpPartialMethods.cs new file mode 100644 index 00000000..5e5f8709 --- /dev/null +++ b/tests/CSharp/CSharpPartialMethods.cs @@ -0,0 +1,16 @@ +using System; +using System.Collections.Generic; +using System.Text; + +namespace CSharp +{ + public partial class TestFinalizer + { + public static Action DisposeCallback { get; set; } + + partial void DisposePartial(bool disposing) + { + DisposeCallback?.Invoke(disposing, __Instance); + } + } +}