-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Discard lastIndexMarshalled variable if it is not otherwise used #88060
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
Conversation
| } | ||
|
|
||
| [Fact] | ||
| [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] |
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.
Before we merge this, can we determine why it fails? I do want to confirm why this is failing, even if we don't fix it.
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.
It looks like we got another repro on Windows Server 2022, so this is an intermittent failure that likely isn't Win7 specific.
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.
We also have one on x86 again. I would try running this on x86 Windows again. I can give it a try on my end and attempt a repro too.
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.
@jtschuster This repros locally for me. I built the runtime (Debug) and libraries (Release) x86 and then ran the test as follows:
dotnet.cmd test src\libraries\System.Runtime.InteropServices\tests\ComInterfaceGenerator.Tests\ComInterfaceGenerator.Tests.csproj /p:Configuration=Release /p:TargetArchitecture=x86
The result was:
Test run for ...\runtime\artifacts\bin\ComInterfaceGenerator.Tests\Release\net8.0\ComInterfaceGenerator.Tests.dll (.NETCoreApp,Version=v8.0)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:32.60] ComInterfaceGenerator.Tests.RcwAroundCcwTests.IJaggedArrayMarshallingFails [FAIL]
Failed ComInterfaceGenerator.Tests.RcwAroundCcwTests.IJaggedArrayMarshallingFails [831 ms]
Error Message:
Assert.Throws() Failure
Expected: typeof(System.ArgumentException)
Actual: typeof(System.NullReferenceException): Object reference not set to an instance of an object.
---- System.NullReferenceException : Object reference not set to an instance of an object.
There were no asserts in the runtime so I assume the issue is at a higher layer.
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.
Below is the generated code for the Get(). Look for "Failure --->" below.
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.ComInterfaceGenerator", "42.42.42.42")]
[global::System.Runtime.CompilerServices.SkipLocalsInitAttribute]
int[][] global::ComInterfaceGenerator.Tests.ComInterfaces.IJaggedIntArrayMarshallingFails.Get(out int[] widths, out int length)
{
var(__this, __vtable_native) = ((System.Runtime.InteropServices.Marshalling.IUnmanagedVirtualMethodTableProvider)this).GetVirtualMethodTableInfoForKey(typeof(global::ComInterfaceGenerator.Tests.ComInterfaces.IJaggedIntArrayMarshallingFails));
System.Runtime.CompilerServices.Unsafe.SkipInit(out widths);
System.Runtime.CompilerServices.Unsafe.SkipInit(out length);
int* __widths_native = default;
int[][] __retVal = default;
System.IntPtr* __retVal_native = default;
int __invokeRetVal = default;
// Setup - Perform required setup.
int __widths_native__numElements;
System.Runtime.CompilerServices.Unsafe.SkipInit(out __widths_native__numElements);
int __retVal_native__numElements;
System.Runtime.CompilerServices.Unsafe.SkipInit(out __retVal_native__numElements);
try
{
// Pin - Pin data in preparation for calling the P/Invoke.
fixed (int* __length_native = &length)
{
__invokeRetVal = ((delegate* unmanaged[MemberFunction]<void*, int**, int*, System.IntPtr**, int> )__vtable_native[3])(__this, &__widths_native, __length_native, &__retVal_native);
}
System.GC.KeepAlive(this);
// Unmarshal - Convert native data to managed data.
System.Runtime.InteropServices.Marshal.ThrowExceptionForHR(__invokeRetVal);
__widths_native__numElements = length;
widths = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, int>.AllocateContainerForManagedElements(__widths_native, __widths_native__numElements);
global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, int>.GetUnmanagedValuesSource(__widths_native, __widths_native__numElements).CopyTo(global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, int>.GetManagedValuesDestination(widths));
__retVal_native__numElements = length;
__retVal = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int[], System.IntPtr>.AllocateContainerForManagedElements(__retVal_native, __retVal_native__numElements);
{
System.ReadOnlySpan<System.IntPtr> __retVal_native__nativeSpan = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int[], System.IntPtr>.GetUnmanagedValuesSource(__retVal_native, __retVal_native__numElements);
System.Span<int[]> __retVal_native__managedSpan = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int[], System.IntPtr>.GetManagedValuesDestination(__retVal);
for (int __i0 = 0; __i0 < __retVal_native__numElements; ++__i0)
{
int __retVal_native__nativeSpan____i0__numElements;
System.Runtime.CompilerServices.Unsafe.SkipInit(out __retVal_native__nativeSpan____i0__numElements);
__retVal_native__nativeSpan____i0__numElements = widths[__i0];
__retVal_native__managedSpan[__i0] = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, nint>.AllocateContainerForManagedElements((nint*)__retVal_native__nativeSpan[__i0], __retVal_native__nativeSpan____i0__numElements);
{
System.ReadOnlySpan<nint> __retVal_native__nativeSpan____i0__nativeSpan = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, nint>.GetUnmanagedValuesSource((nint*)__retVal_native__nativeSpan[__i0], __retVal_native__nativeSpan____i0__numElements);
System.Span<int> __retVal_native__nativeSpan____i0__managedSpan = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, nint>.GetManagedValuesDestination(__retVal_native__managedSpan[__i0]);
for (int __i1 = 0; __i1 < __retVal_native__nativeSpan____i0__numElements; ++__i1)
{
__retVal_native__nativeSpan____i0__managedSpan[__i1] = global::ComInterfaceGenerator.Tests.ComInterfaces.ThrowOn4thElementMarshalled.ConvertToManaged(__retVal_native__nativeSpan____i0__nativeSpan[__i1]);
}
}
}
}
}
finally
{
// Cleanup - Perform required cleanup.
__widths_native__numElements = length;
global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, int>.Free(__widths_native);
{
System.ReadOnlySpan<System.IntPtr> __retVal_native__nativeSpan = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int[], System.IntPtr>.GetUnmanagedValuesDestination(__retVal_native, __retVal_native__numElements);
for (int __i0 = 0; __i0 < __retVal_native__nativeSpan.Length; ++__i0)
{
int __retVal_native__nativeSpan____i0__numElements;
System.Runtime.CompilerServices.Unsafe.SkipInit(out __retVal_native__nativeSpan____i0__numElements);
Failure ---> __retVal_native__nativeSpan____i0__numElements = widths[__i0]; // widths is null.
global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int, nint>.Free((nint*)__retVal_native__nativeSpan[__i0]);
}
}
__retVal_native__numElements = length;
global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<int[], System.IntPtr>.Free(__retVal_native);
}
return __retVal;
}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, thank you for looking into this! I'll see if I can try to repro again on my machine and see why it's throwing before widths is assigned.
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.
Could this be a case where an exception is thrown (or failing HResult returned) from the inner call, causing a throw before we unmarshal widths? Then we end up hitting this other exception on all platforms, but due to differences in the exception handling model (and the fact that you shouldn't throw from a finally block anyway), we get different behavior between x86 and x64?
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.
Oh, yeah, that's probably it, the CCW stub for Get would fail marshalling the array and return a non-zero HResult, which would throw before we set widths.
The generated code is the exact same though, and it should skip the cleanup stage if __retVal_native__numElements is 0. That variable is never set along this code path. Does SkipInit have different behavior on x86 and x64?
Either way, we end up using a SkipInit'ed int as the length of a span during cleanup, which doesn't seem right.
|
Looks the failure is related to #88111. Should we block this until that gets sorted out or skip the test and merge? |
AaronRobinsonMSFT
left a comment
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.
Thank you!
|
Failures are known, and issues are filed for follow ups. |
Alternative to #87846 which doesn't need to run the Cleanup generation phase to determine if lastIndexMarshalled is used.