From 117450e3fac0be15c9cb292cb158d9398b6f30ef Mon Sep 17 00:00:00 2001 From: Lucas Cavalcante de Sousa Date: Fri, 28 Feb 2020 15:58:08 +0000 Subject: [PATCH] C#: Update C# code-generation to use a new ICustomMarshaler in some string usages that were leaking When `C` calls a function that return/has an out string and it was overwritten by `C#` inherit class the `C` portion wasn't cleaning its copy. Now, when a `C` calls a `C#` delegate function, `Strings` that are `out` values or `return` values use a new marshaler (specific to this case) that uses Eina short lived strings (`Eina_Slstr`) instead of duplicating it with `strdup`, so at some point, the string passed to `C` is deleted. To do so, a `direction_context` (a new `Context` at `generation_contexts.hh`) was created. It is only used when a C# delegate is being called from C (so this context is only set in `function_definition.hh` and `property_definition.hh`, where it is set to `native_to_manage` to indicate that it is a native call to a managed function). When this `direction_context` is set and the `String` being marshaled is not marked with an `@move` tag and it is an `out` or `return` value, the new `StringOutMarshaler` (implemented at `iwrapper.cs`) is used (instead of `StringKeepOwnershipMarshaler`). When marshaling a managed data to native this marshaler uses eina short lived string (`Eina_Slstr`) that will be automatically deleted. This delete is bounded to "the loop of the current thread or until the clear function is called explicitly" as said at `src/lib/eina/eina_slstr.h`. Reviewed-by: Felipe Magno de Almeida Differential Revision: https://phab.enlightenment.org/D11434 --- .../eolian/mono/function_definition.hh | 24 +++++++----- .../eolian/mono/generation_contexts.hh | 13 +++++++ .../eolian/mono/marshall_annotation.hh | 21 ++++++++-- .../eolian/mono/property_definition.hh | 26 +++++++------ src/bindings/mono/eina_mono/eina_common.cs | 8 ++++ src/bindings/mono/eo_mono/iwrapper.cs | 39 +++++++++++++++++++ 6 files changed, 106 insertions(+), 25 deletions(-) diff --git a/src/bin/eolian_mono/eolian/mono/function_definition.hh b/src/bin/eolian_mono/eolian/mono/function_definition.hh index 5fc0e84fbf..975335f4eb 100644 --- a/src/bin/eolian_mono/eolian/mono/function_definition.hh +++ b/src/bin/eolian_mono/eolian/mono/function_definition.hh @@ -47,7 +47,7 @@ struct native_function_definition_generator { attributes::klass_def const* klass; std::vector properties; - + template bool generate(OutputIterator sink, attributes::function_def const& f, Context const& context) const { @@ -72,7 +72,7 @@ struct native_function_definition_generator if (property_generate_wrapper_setter (*it, context)) return true; } - + auto const& indent = current_indentation(context); // Delegate for the C# method we will export to EO as a method implementation. @@ -90,7 +90,9 @@ struct native_function_definition_generator (marshall_annotation << " " << marshall_parameter) ) % ", ") << ");\n\n") - .generate(sink, std::make_tuple(f.return_type, f.return_type, f.c_name, f.parameters), context)) + .generate(sink, + std::make_tuple(f.return_type, f.return_type, f.c_name, f.parameters), + context_add_tag(direction_context{direction_context::native_to_managed}, context))) return false; // API delegate is the wrapper for the Eo methods exported from C that we will use from C#. @@ -186,7 +188,7 @@ struct native_function_definition_generator , f.c_name , f.parameters ) - , context)) + , context_add_tag(direction_context{direction_context::native_to_managed}, context))) return false; // Static functions do not need to be called from C @@ -196,13 +198,15 @@ struct native_function_definition_generator // This is the delegate that will be passed to Eo to be called from C. if(!as_generator( indent << "private static " << f.c_name << "_delegate " << f.c_name << "_static_delegate;\n\n" - ).generate(sink, attributes::unused, context)) + ).generate(sink, + attributes::unused, + context_add_tag(direction_context{direction_context::native_to_managed}, context))) return false; return true; } }; - + struct function_definition_generator { template @@ -232,7 +236,7 @@ struct function_definition_generator if (property_generate_wrapper_setter (*it, context)) function_scope = "internal "; } - + // Do not generate static function for concrete class if (is_concrete && f.is_static) return true; @@ -402,7 +406,7 @@ struct property_wrapper_definition_generator return true; if (property.setter && !property.setter->keys.empty()) return true; - + if (property.getter && property.setter) { if (property.setter->values.size() != property.getter->values.size()) @@ -498,7 +502,7 @@ struct property_wrapper_definition_generator if (property.setter && property.setter->explicit_return_type.c_type == "Eina_Success_Flag") set_has_return_error = true; - + if (parameters.size() == 1) { if (!as_generator( @@ -674,7 +678,7 @@ struct attributes_needed< ::eolian_mono::property_wrapper_definition_generator> template <> struct attributes_needed< ::eolian_mono::property_wrapper_definition_parameterized> : std::integral_constant {}; } - + } } } #endif diff --git a/src/bin/eolian_mono/eolian/mono/generation_contexts.hh b/src/bin/eolian_mono/eolian/mono/generation_contexts.hh index f7376f056a..6cbdb19a36 100644 --- a/src/bin/eolian_mono/eolian/mono/generation_contexts.hh +++ b/src/bin/eolian_mono/eolian/mono/generation_contexts.hh @@ -50,6 +50,19 @@ struct class_context {} }; +struct direction_context +{ + enum direction { + native_to_managed, + managed_to_native, + }; + direction current_direction; + + direction_context(direction current_direction) + : current_direction(current_direction) + {} +}; + struct indentation_context { constexpr indentation_context(indentation_context const& other) = default; diff --git a/src/bin/eolian_mono/eolian/mono/marshall_annotation.hh b/src/bin/eolian_mono/eolian/mono/marshall_annotation.hh index a9fc45d7da..527771959f 100644 --- a/src/bin/eolian_mono/eolian/mono/marshall_annotation.hh +++ b/src/bin/eolian_mono/eolian/mono/marshall_annotation.hh @@ -28,8 +28,9 @@ namespace eolian_mono { namespace eina = efl::eina; namespace detail { - + template + struct marshall_annotation_visitor_generate { mutable OutputIterator sink; @@ -41,7 +42,7 @@ struct marshall_annotation_visitor_generate typedef marshall_type_visitor_generate visitor_type; typedef bool result_type; - + bool operator()(attributes::regular_type_def const& regular) const { using attributes::regular_type_def; @@ -60,7 +61,13 @@ struct marshall_annotation_visitor_generate {"string", true, [] { return "MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef=typeof(Efl.Eo.StringPassOwnershipMarshaler))"; }}, - {"string", false, [] { + {"string", false, [this] { + auto is_native_to_managed = false; + if constexpr (efl::eolian::grammar::tag_check::value) + is_native_to_managed = context_find_tag(*context).current_direction == direction_context::native_to_managed; + + if((is_out || is_return) && is_native_to_managed) + return "MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef=typeof(Efl.Eo.StringOutMarshaler))"; return "MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef=typeof(Efl.Eo.StringKeepOwnershipMarshaler))"; }}, {"mstring", true, [] { @@ -98,7 +105,13 @@ struct marshall_annotation_visitor_generate {"string", true, [] { return "MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef=typeof(Efl.Eo.StringPassOwnershipMarshaler))"; }}, - {"string", false, [] { + {"string", false, [this] { + auto is_native_to_managed = false; + if constexpr (efl::eolian::grammar::tag_check::value) + is_native_to_managed = context_find_tag(*context).current_direction == direction_context::native_to_managed; + + if((is_out || is_return) && is_native_to_managed) + return "MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef=typeof(Efl.Eo.StringOutMarshaler))"; return "MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef=typeof(Efl.Eo.StringKeepOwnershipMarshaler))"; }}, {"mstring", true, [] { diff --git a/src/bin/eolian_mono/eolian/mono/property_definition.hh b/src/bin/eolian_mono/eolian/mono/property_definition.hh index 703b70269d..827454fd10 100644 --- a/src/bin/eolian_mono/eolian/mono/property_definition.hh +++ b/src/bin/eolian_mono/eolian/mono/property_definition.hh @@ -37,7 +37,7 @@ struct compare_get_and_set_value_type inline bool operator () (attributes::parameter_def const& get, attributes::parameter_def const& set) const; inline bool operator () (attributes::type_def const& get, attributes::type_def const& set) const; }; - + struct compare_get_and_set_value_type_overload { template @@ -64,7 +64,7 @@ struct compare_get_and_set_value_type_overload typedef bool result_type; }; - + inline bool compare_get_and_set_value_type::operator () (attributes::parameter_def const& get, attributes::parameter_def const& set) const { return efl::eina::visit(compare_get_and_set_value_type_overload{}, get.type.original_type, set.type.original_type); @@ -73,7 +73,7 @@ inline bool compare_get_and_set_value_type::operator () (attributes::type_def co { return efl::eina::visit(compare_get_and_set_value_type_overload{}, get.original_type, set.original_type); } - + template bool property_generate_wrapper_both_check(attributes::property_def const& property, Context const& context) { @@ -87,7 +87,7 @@ bool property_generate_wrapper_both_check(attributes::property_def const& proper if ((is_concrete || is_interface) && is_static) return false; - + if (!property.getter) return false; @@ -102,7 +102,7 @@ bool property_generate_wrapper_both_check(attributes::property_def const& proper else return false; } - + return true; } @@ -144,7 +144,7 @@ bool property_generate_wrapper_setter (attributes::property_def const& property, if (property.setter->explicit_return_type != attributes::void_) return false; - + if (!property.setter->keys.empty()) return false; @@ -192,7 +192,9 @@ struct native_property_function_definition_generator (marshall_annotation << " " << marshall_parameter) ) % ", ") << ");\n\n") - .generate(sink, std::make_tuple(f.return_type, f.return_type, f.c_name, f.parameters), context)) + .generate(sink, + std::make_tuple(f.return_type, f.return_type, f.c_name, f.parameters), + context_add_tag(direction_context{direction_context::native_to_managed}, context))) return false; // API delegate is the wrapper for the Eo methods exported from C that we will use from C#. @@ -279,7 +281,7 @@ struct native_property_function_definition_generator if(!f.keys.empty() && !as_generator(lit("[(") << (native_argument_invocation % ", ") << ")]").generate (sink, f.keys, context)) return false; - + if(!as_generator (" = (" << (native_tuple_argument_invocation % ", ") << ");\n" @@ -340,7 +342,9 @@ struct native_property_function_definition_generator // This is the delegate that will be passed to Eo to be called from C. if(!as_generator( indent << "private static " << f.c_name << "_delegate " << f.c_name << "_static_delegate;\n\n" - ).generate(sink, attributes::unused, context)) + ).generate(sink, + attributes::unused, + context_add_tag(direction_context{direction_context::native_to_managed}, context))) return false; return true; }; @@ -377,10 +381,10 @@ template <> struct is_generator< ::eolian_mono::native_property_function_definition_generator> : std::true_type {}; namespace type_traits { - + template <> struct attributes_needed< ::eolian_mono::native_property_function_definition_generator> : std::integral_constant {}; - + } } } } #endif diff --git a/src/bindings/mono/eina_mono/eina_common.cs b/src/bindings/mono/eina_mono/eina_common.cs index a4f2ff4c5f..014e77d020 100644 --- a/src/bindings/mono/eina_mono/eina_common.cs +++ b/src/bindings/mono/eina_mono/eina_common.cs @@ -55,6 +55,9 @@ internal static partial class NativeCustomExportFunctions efl_mono_native_free_addr_get(); [DllImport(efl.Libs.CustomExports)] public static extern IntPtr efl_mono_native_efl_unref_addr_get(); + + [DllImport(efl.Libs.Eina)] public static extern IntPtr + eina_slstr_copy_new(string str); } /// Wrapper around native memory DllImport'd functions. @@ -94,6 +97,11 @@ public static class MemoryNative return NativeCustomExportFunctions.efl_mono_native_strdup(str); } + public static IntPtr SlstrCopyNew(string str) + { + return NativeCustomExportFunctions.eina_slstr_copy_new(str); + } + /// /// Retrieves an instance of a string for use in program. /// Since EFL 1.23. diff --git a/src/bindings/mono/eo_mono/iwrapper.cs b/src/bindings/mono/eo_mono/iwrapper.cs index ceae250bc9..626777f7d5 100644 --- a/src/bindings/mono/eo_mono/iwrapper.cs +++ b/src/bindings/mono/eo_mono/iwrapper.cs @@ -1508,6 +1508,45 @@ class StringPassOwnershipMarshaler : ICustomMarshaler static private StringPassOwnershipMarshaler marshaler; } + +class StringOutMarshaler: ICustomMarshaler +{ + public object MarshalNativeToManaged(IntPtr pNativeData) + { + return Eina.StringConversion.NativeUtf8ToManagedString(pNativeData); + } + + public IntPtr MarshalManagedToNative(object managedObj) + { + return Eina.MemoryNative.SlstrCopyNew((string)managedObj); + } + + public void CleanUpNativeData(IntPtr pNativeData) + { + } + + public void CleanUpManagedData(object managedObj) + { + } + + public int GetNativeDataSize() + { + return -1; + } + + internal static ICustomMarshaler GetInstance(string cookie) + { + if (marshaler == null) + { + marshaler = new StringOutMarshaler(); + } + + return marshaler; + } + + static private StringOutMarshaler marshaler; +} + class StringKeepOwnershipMarshaler: ICustomMarshaler { public object MarshalNativeToManaged(IntPtr pNativeData)