[c#] Fix GC hole in return areas (#945)

* uncomment to force crash

* fix GC hole with return areas

* remove vscode files

* add comment

remove sln from git

* format

* Close all fixed blocks

* correct tmp variable name use

* correct comment.

* use nint for wit ptr

Author: yowl

crates/csharp/src/RepTable.cs

@@ -30,17 +30,17 @@ internal int Add(T v) {
         return rep;
     }
 
-    internal T Get(int rep) {
-        if (list[rep] is Vacant) {
+    internal T Get(nint rep) {
+        if (list[(int)rep] is Vacant) {
             throw new ArgumentException("invalid rep");
         }
-        return (T) list[rep];
+        return (T) list[(int)rep];
     }
 
-    internal T Remove(int rep) {
+    internal T Remove(nint rep) {
         var val = Get(rep);
-        list[rep] = new Vacant(firstVacant);
-        firstVacant = rep;
+        list[(int)rep] = new Vacant(firstVacant);
+        firstVacant = (int)rep;
         return (T) val;
     }
 }

crates/csharp/src/csproj.rs

@@ -80,7 +80,6 @@ impl CSProjectLLVMBuilder {
         <ItemGroup>
           <NativeLibrary Include=\"{camel}_component_type.o\" />
           <NativeLibrary Include=\"$(MSBuildProjectDirectory)/{camel}_cabi_realloc.o\" />
-   
         </ItemGroup>
 
         <ItemGroup>

crates/csharp/src/lib.rs

@@ -519,6 +519,8 @@ impl WorldGenerator for CSharp {
         if self.needs_export_return_area {
             let mut ret_area_str = String::new();
 
+            let (array_size, element_type) =
+                dotnet_aligned_array(self.return_area_size, self.return_area_align);
             uwrite!(
                 ret_area_str,
                 "
@@ -528,23 +530,22 @@ impl WorldGenerator for CSharp {
                     [StructLayout(LayoutKind.Sequential, Pack = {1})]
                     internal struct ReturnArea
                     {{
-                        private byte buffer;
+                        private {2} buffer;
 
-                        internal unsafe int AddressOfReturnArea()
+                        internal unsafe nint AddressOfReturnArea()
                         {{
-                            fixed(byte* ptr = &buffer)
-                            {{
-                                return (int)ptr;
-                            }}
+                            return (nint)Unsafe.AsPointer(ref buffer);
                         }}
                     }}
 
                     [ThreadStatic]
+                    [FixedAddressValueType]
                     internal static ReturnArea returnArea = default;
                 }}
                 ",
-                self.return_area_size,
+                array_size,
                 self.return_area_align,
+                element_type
             );
 
             src.push_str(&ret_area_str);
@@ -984,8 +985,6 @@ impl InterfaceGenerator<'_> {
         );
 
         let src = bindgen.src;
-        let import_return_pointer_area_size = bindgen.import_return_pointer_area_size;
-        let import_return_pointer_area_align = bindgen.import_return_pointer_area_align;
 
         let params = func
             .params
@@ -1022,28 +1021,6 @@ impl InterfaceGenerator<'_> {
             "#
         );
 
-        if import_return_pointer_area_size > 0 {
-            uwrite!(
-                target,
-                r#"
-                [InlineArray({import_return_pointer_area_size})]
-                [StructLayout(LayoutKind.Sequential, Pack = {import_return_pointer_area_align})]
-                internal struct ImportReturnArea
-                {{
-                    private byte buffer;
-
-                    internal unsafe int AddressOfReturnArea()
-                    {{
-                        fixed(byte* ptr = &buffer)
-                        {{
-                            return (int)ptr;
-                        }}
-                    }}
-                }}
-                "#,
-            )
-        }
-
         uwrite!(
             target,
             r#"
@@ -1862,6 +1839,7 @@ struct FunctionBindgen<'a, 'b> {
     cleanup: Vec<Cleanup>,
     import_return_pointer_area_size: usize,
     import_return_pointer_area_align: usize,
+    fixed: usize, // Number of `fixed` blocks that need to be closed.
     resource_drops: Vec<(String, String)>,
 }
 
@@ -1892,6 +1870,7 @@ impl<'a, 'b> FunctionBindgen<'a, 'b> {
             cleanup: Vec::new(),
             import_return_pointer_area_size: 0,
             import_return_pointer_area_align: 0,
+            fixed: 0,
             resource_drops: Vec::new(),
         }
     }
@@ -2468,7 +2447,6 @@ impl Bindgen for FunctionBindgen<'_, '_> {
 
                 let list = &operands[0];
                 let size = self.gen.gen.sizes.size(element);
-                let _align = self.gen.gen.sizes.align(element);
                 let ty = self.gen.type_name_with_qualifier(element, true);
                 let index = self.locals.tmp("index");
 
@@ -2513,7 +2491,6 @@ impl Bindgen for FunctionBindgen<'_, '_> {
                 let array = self.locals.tmp("array");
                 let ty = self.gen.type_name_with_qualifier(element, true);
                 let size = self.gen.gen.sizes.size(element);
-                let _align = self.gen.gen.sizes.align(element);
                 let index = self.locals.tmp("index");
 
                 let result = match &block_results[..] {
@@ -2526,7 +2503,7 @@ impl Bindgen for FunctionBindgen<'_, '_> {
                     "
                     var {array} = new List<{ty}>({length});
                     for (int {index} = 0; {index} < {length}; ++{index}) {{
-                        int {base} = {address} + ({index} * {size});
+                        nint {base} = {address} + ({index} * {size});
                         {body}
                         {array}.Add({result});
                     }}
@@ -2651,6 +2628,11 @@ impl Bindgen for FunctionBindgen<'_, '_> {
                             uwriteln!(self.src, "return ({results});")
                         }
                     }
+
+                // Close all the fixed blocks.
+                for _ in 0..self.fixed {
+                    uwriteln!(self.src, "}}");
+                }
                 }
             }
 
@@ -2780,19 +2762,30 @@ impl Bindgen for FunctionBindgen<'_, '_> {
         // their return area to be live until the post-return call.
         match self.gen.direction {
             Direction::Import => {
-                let ret_area = self.locals.tmp("retArea");
-                let name = self.func_name.to_upper_camel_case();
                 self.import_return_pointer_area_size =
                     self.import_return_pointer_area_size.max(size);
                 self.import_return_pointer_area_align =
                     self.import_return_pointer_area_align.max(align);
+                let (array_size, element_type) = dotnet_aligned_array(
+                    self.import_return_pointer_area_size,
+                    self.import_return_pointer_area_align,
+                );
+                let ret_area = self.locals.tmp("retArea");
+                let ret_area_byte0 = self.locals.tmp("retAreaByte0");
                 uwrite!(
                     self.src,
                     "
-                    var {ret_area} = new {name}WasmInterop.ImportReturnArea();
-                    var {ptr} = {ret_area}.AddressOfReturnArea();
-                    "
+                    var {2} = new {0}[{1}];
+                    fixed ({0}* {3} = &{2}[0])
+                    {{
+                        var {ptr} = (nint){3};
+                    ",
+                    element_type,
+                    array_size,
+                    ret_area,
+                    ret_area_byte0
                 );
+                self.fixed = self.fixed + 1;
 
                 return format!("{ptr}");
             }
@@ -2857,6 +2850,26 @@ impl Bindgen for FunctionBindgen<'_, '_> {
     }
 }
 
+// We cant use "StructLayout.Pack" as dotnet will use the minimum of the type and the "Pack" field,
+// so for byte it would always use 1 regardless of the "Pack".
+fn dotnet_aligned_array(array_size: usize, required_alignment: usize) -> (usize, String) {
+    match required_alignment {
+        1 => {
+            return (array_size, "byte".to_owned());
+        }
+        2 => {
+            return ((array_size + 1) / 2, "ushort".to_owned());
+        }
+        4 => {
+            return ((array_size + 3) / 4, "uint".to_owned());
+        }
+        8 => {
+            return ((array_size + 7) / 8, "ulong".to_owned());
+        }
+        _ => todo!("unsupported return_area_align {}", required_alignment),
+    }
+}
+
 fn perform_cast(op: &String, cast: &Bitcast) -> String {
     match cast {
         Bitcast::I32ToF32 => format!("BitConverter.Int32BitsToSingle({op})"),
@@ -2900,7 +2913,7 @@ fn wasm_type(ty: WasmType) -> &'static str {
         WasmType::I64 => "long",
         WasmType::F32 => "float",
         WasmType::F64 => "double",
-        WasmType::Pointer => "int",
+        WasmType::Pointer => "nint",
         WasmType::PointerOrI64 => "long",
         WasmType::Length => "int",
     }

tests/runtime/variants/wasm.cs

@@ -0,0 +1,118 @@
+using System;
+using System.Runtime.InteropServices;
+using System.Diagnostics;
+using VariantsWorld.wit.imports.test.variants;
+
+namespace VariantsWorld
+{
+
+    public class VariantsWorldImpl : IVariantsWorld
+    {
+        public static void TestImports()
+        {
+            Debug.Assert(TestInterop.RoundtripOption(new Option<float>(1.0f)).Value == 1);
+            Debug.Assert(TestInterop.RoundtripOption(Option<float>.None).HasValue == false);
+            Debug.Assert(TestInterop.RoundtripOption(new Option<float>(2.0f)).Value == 2);
+
+            Debug.Assert(TestInterop.RoundtripResult(Result<uint, float>.ok(2)).AsOk == 2.0);
+            Debug.Assert(TestInterop.RoundtripResult(Result<uint, float>.ok(4)).AsOk == 4.0);
+            Debug.Assert(TestInterop.RoundtripResult(Result<uint, float>.err(5.3f)).AsErr == 5);
+
+            Debug.Assert(TestInterop.RoundtripEnum(ITest.E1.A) == ITest.E1.A);
+            Debug.Assert(TestInterop.RoundtripEnum(ITest.E1.B) == ITest.E1.B);
+
+            Debug.Assert(TestInterop.InvertBool(true) == false);
+            Debug.Assert(TestInterop.InvertBool(false) == true);
+
+            var (a1, a2, a3, a4, a5, a6) =
+            TestInterop.VariantCasts((ITest.C1.a(1), ITest.C2.a(2), ITest.C3.a(3), ITest.C4.a(4), ITest.C5.a(5), ITest.C6.a(6.0f)));
+            Debug.Assert(a1.AsA == 1);
+            Debug.Assert(a2.AsA == 2);
+            Debug.Assert(a3.AsA == 3);
+            Debug.Assert(a4.AsA == 4);
+            Debug.Assert(a5.AsA == 5);
+            Debug.Assert(a6.AsA == 6.0f);
+
+            var (b1, b2, b3, b4, b5, b6) =
+TestInterop.VariantCasts((ITest.C1.b(1), ITest.C2.b(2), ITest.C3.b(3), ITest.C4.b(4), ITest.C5.b(5), ITest.C6.b(6.0)));
+            Debug.Assert(b1.AsB == 1);
+            Debug.Assert(b2.AsB == 2.0f);
+            Debug.Assert(b3.AsB == 3.0f);
+            Debug.Assert(b4.AsB == 4.0f);
+            Debug.Assert(b5.AsB == 5.0f);
+            Debug.Assert(b6.AsB == 6.0);
+
+            var (za1, za2, za3, za4) =
+TestInterop.VariantZeros((ITest.Z1.a(1), ITest.Z2.a(2), ITest.Z3.a(3.0f), ITest.Z4.a(4.0f)));
+            Debug.Assert(za1.AsA == 1);
+            Debug.Assert(za2.AsA == 2);
+            Debug.Assert(za3.AsA == 3.0f);
+            Debug.Assert(za4.AsA == 4.0f);
+
+            var (zb1, zb2, zb3, zb4) =
+TestInterop.VariantZeros((ITest.Z1.b(), ITest.Z2.b(), ITest.Z3.b(), ITest.Z4.b()));
+            //TODO: Add comparison operator to variants and None
+            //Debug.Assert(zb1.AsB == ITest.Z1.b());
+            //Debug.Assert(zb2.AsB == ITest.Z2.b());
+            //Debug.Assert(zb3.AsB == ITest.Z3.b());
+            //Debug.Assert(zb4.AsB == ITest.Z4.b());
+
+            TestInterop.VariantTypedefs(Option<uint>.None, false, Result<uint, None>.err(new None()));
+
+            var (a, b, c) = TestInterop.VariantEnums(true, Result<None, None>.ok(new None()), ITest.MyErrno.SUCCESS);
+            Debug.Assert(a == false);
+            var test = b.AsErr;
+            Debug.Assert(c == ITest.MyErrno.A);
+        }
+    }
+}
+
+namespace VariantsWorld.wit.exports.test.variants
+{
+    public class TestImpl : ITest
+    {
+        public static Option<byte> RoundtripOption(Option<float> a)
+        {
+            return a.HasValue ? new Option<byte>((byte)a.Value) : Option<byte>.None;
+        }
+
+        public static Result<double, byte> RoundtripResult(Result<uint, float> a)
+        {
+            switch (a.Tag)
+            {
+                case Result<double, byte>.OK: return Result<double, byte>.ok((double)a.AsOk);
+                case Result<double, byte>.ERR: return Result<double, byte>.err((byte)a.AsErr);
+                default: throw new ArgumentException();
+            }
+        }
+
+        public static ITest.E1 RoundtripEnum(ITest.E1 a)
+        {
+            return a;
+        }
+
+        public static bool InvertBool(bool a)
+        {
+            return !a;
+        }
+
+        public static (ITest.C1, ITest.C2, ITest.C3, ITest.C4, ITest.C5, ITest.C6)
+            VariantCasts((ITest.C1, ITest.C2, ITest.C3, ITest.C4, ITest.C5, ITest.C6) a)
+        {
+            return a;
+        }
+
+        public static (bool, Result<None, None>, ITest.MyErrno)
+            VariantEnums(bool a, Result<None, None> b, ITest.MyErrno c)
+        {
+            return new(a, b, c);
+        }
+
+        public static void VariantTypedefs(Option<uint> a, bool b, Result<uint, None> c) { }
+
+        public static (ITest.Z1, ITest.Z2, ITest.Z3, ITest.Z4) VariantZeros((ITest.Z1, ITest.Z2, ITest.Z3, ITest.Z4) a)
+        {
+            return a;
+        }
+    }
+}