Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ComWrappers/CsWinRT code produces different (wrong) results on NativeAOT #1321

Closed
Sergio0694 opened this issue Apr 16, 2023 · 2 comments
Closed
Labels
AOT bug Something isn't working
Milestone

Comments

@Sergio0694
Copy link
Member

Describe the bug

Opening this as part of an investigation into a NativeAOT crash I noticed in ComputeSharp, which we've been trying to narrow down (as a collaborative effort over in the C# Discord). While trying to put together a minimal repro, I couldn't quite reproduce the same crash (which seems to be something going wrong in ComWrappers on NAOT), but I still noticed what seems to be incorrect behavior on NAOT, so here's a separate issue to track that. Not entirely sure whether this is more an issue with ComWrappers or CsWinRT, so opening it here for now. Will try to create a ComWrappers-only repro too later on to see if it also repros with just that.

To Reproduce

  1. Create a new project with new7-windows10.0.22621 as TFM
  2. Paste the following code:
Code (click to expand):
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using WinRT.Interop;
using WinRT;

unsafe
{
    DummyUnknown unknown = new();

    for (int i = 0; i < 10; i++)
    {
        var value2 = MarshalInspectable<object>.CreateMarshaler2(unknown);

        try
        {
            var abi = (IUnknownVftbl*)value2.GetAbi();
            var guid = new Guid("33D8B971-2ABF-4A2B-8071-1FFCBCBC8124");
            DummyUnknownAbi* ppv = null;

            ExceptionHelpers.ThrowExceptionForHR(Marshal.QueryInterface((IntPtr)abi, ref guid, out *(IntPtr*)&ppv));

            Console.WriteLine($"{i}: {ppv->GetNumber()}");

            ppv->Release();
        }
        finally
        {
            MarshalInspectable<object>.DisposeMarshaler(value2);
        }
    }
}

Console.WriteLine("Done!");


public unsafe class DummyUnknown : IDummyUnknown
{
    private int number = 42;

    public int GetNumber()
    {
        return number++;
    }
}

public unsafe struct DummyUnknownAbi
{
    public void** lpVtbl;

    public int QueryInterface(Guid* riid, void** ppvObject)
    {
        return ((delegate* unmanaged[Stdcall]<DummyUnknownAbi*, Guid*, void**, int>)lpVtbl[0])((DummyUnknownAbi*)Unsafe.AsPointer(ref this), riid, ppvObject);
    }

    public uint AddRef()
    {
        return ((delegate* unmanaged[Stdcall]<DummyUnknownAbi*, uint>)lpVtbl[1])((DummyUnknownAbi*)Unsafe.AsPointer(ref this));
    }

    public uint Release()
    {
        return ((delegate* unmanaged[Stdcall]<DummyUnknownAbi*, uint>)lpVtbl[2])((DummyUnknownAbi*)Unsafe.AsPointer(ref this));
    }

    public int GetNumber()
    {
        return ((delegate* unmanaged[Stdcall]<DummyUnknownAbi*, int>)lpVtbl[3])((DummyUnknownAbi*)Unsafe.AsPointer(ref this));
    }
}

[Guid("33D8B971-2ABF-4A2B-8071-1FFCBCBC8124")]
[WindowsRuntimeType]
[WindowsRuntimeHelperType(typeof(IDummyUnknown))]
public interface IDummyUnknown
{
    int GetNumber();

    [Guid("33D8B971-2ABF-4A2B-8071-1FFCBCBC8124")]
    public unsafe struct Vftbl
    {
        public static readonly IntPtr AbiToProjectionVftablePtr = InitVtbl();

        private static IntPtr InitVtbl()
        {
            Vftbl* lpVtbl = (Vftbl*)ComWrappersSupport.AllocateVtableMemory(typeof(Vftbl), sizeof(Vftbl));

            lpVtbl->IUnknownVftbl = IUnknownVftbl.AbiToProjectionVftbl;
            lpVtbl->GetNumber = &GetNumberFromAbi;

            return (IntPtr)lpVtbl;
        }

        internal IUnknownVftbl IUnknownVftbl;
        internal delegate* unmanaged[Stdcall]<IntPtr, int> GetNumber;

        [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
        private static int GetNumberFromAbi(IntPtr thisPtr)
        {
            try
            {
                return ComWrappersSupport.FindObject<IDummyUnknown>(thisPtr).GetNumber();
            }
            catch (Exception e)
            {
                ExceptionHelpers.SetErrorInfo(e);

                return Marshal.GetHRForException(e);
            }
        }
    }
}
  1. Add <PublishAot>true</PublishAot> to the .csproj file
  2. Publish with NAOT:
msbuild ComWrappersRepro.csproj -t:restore,publish /p:Configuration=Release /p:Platform=x64 /p:RuntimeIdentifier=win10-x64 /p:TreatWarningsAsErrors=False

Expected behavior

0: 42
1: 43
2: 44
3: 45
4: 46
5: 47
6: 48
7: 49
8: 50
9: 51
Done!

This is what you also get with a normal F5 deploy.

Actual behavior

0: 42
1: -2147467261
2: -2147467261
3: -2147467261
4: -2147467261
5: -2147467261
6: -2147467261
7: -2147467261
8: -2147467261
9: -2147467261
Done!

...?!?!?? wha-

@Sergio0694 Sergio0694 added the bug Something isn't working label Apr 16, 2023
@Sergio0694
Copy link
Member Author

cc. @AaronRobinsonMSFT @jkoritzinsky as this might potentially be related to ComWrappers.
cc. @MichalStrehovsky as this is NAOT-related, maybe the output there gives you any ideas?

@manodasanW manodasanW added the AOT label Apr 17, 2023
kant2002 added a commit to kant2002/runtime that referenced this issue Apr 17, 2023
Fixes: microsoft/CsWinRT#1321
Make ComWrappers in C# work closely with how C++ part works
- `RefCount` => `curr` is plain typo
- Do not destroy wrappers if ref count == 0. That's mimic how CoreCLR work
- Also discover 2 tests which are plainly wrong, since they do not work under CoreCLR
AustinWise pushed a commit to AustinWise/runtime that referenced this issue Apr 20, 2023
Fixes: microsoft/CsWinRT#1321
Make ComWrappers in C# work closely with how C++ part works
- `RefCount` => `curr` is plain typo
- Do not destroy wrappers if ref count == 0. That's mimic how CoreCLR work
- Also discover 2 tests which are plainly wrong, since they do not work under CoreCLR
@Sergio0694
Copy link
Member Author

This was the same issue (just with a slightly different observable result) as dotnet/runtime#84908.
Fixed by dotnet/runtime#85087 🎉

@manodasanW manodasanW added this to the Release 2.1 milestone Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AOT bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants