Failing test - FromLowHighUnsigned_x32_Result

[willibrandon]

I would like to know if this test is failing for anyone else. This test fails for me on all build platforms.

System.Windows.Forms.Primitives.Tests.Interop.PARAMTests.FromLowHighUnsigned_x32_Result

PARAMTests.FromLowHighUnsigned_x32_Result

System.Windows.Forms.Primitives.Tests.Interop.PARAMTests.FromLowHighUnsigned_x32_Result
   Source: PARAMTests.cs line 27
   Duration: < 1 ms

  Message: 
System.OverflowException : Arithmetic operation resulted in an overflow.

  Stack Trace: 
IntPtr.op_Explicit(IntPtr value)
PARAMTests.FromLowHighUnsigned_x32_Result() line 30

I believe the overflow is due to an issue in Interop.PARAM.FromLowHighUnsigned

public static IntPtr FromLowHighUnsigned(int low, int high)
     // Convert the int to an uint before converting it to a pointer type,
    // which ensures the high dword being zero for 64-bit pointers.
    // This corresponds to the logic of the MAKELPARAM/MAKEWPARAM/MAKELRESULT
    // macros.
-   => unchecked((nint)(nuint)(uint)ToInt(low, high));
+   => unchecked((nint)(nuint)ToInt(low, high));

Since we are now casting to nuint, we no longer need to cast to uint. That was the whole point of using native-sized integers in #3603. I believe something was missed there and the original author did not completely understand the change they were making.

Making the suggested change will fix FromLowHighUnsigned_x32_Result but will break another PARAM test FromLowHighUnsigned_x64_Result because that test is currently expecting an incorrect result for one of its assertions.

PARAMTests.FromLowHighUnsigned_x64_Result

[ConditionalFact(typeof(ArchitectureDetection), nameof(ArchitectureDetection.Is64bit))]
public unsafe void FromLowHighUnsigned_x64_Result()
{
    Assert.Equal(unchecked((long)0x0000000003040102), (long)PARAM.FromLowHighUnsigned(0x0102, 0x0304));
-    Assert.Equal(unchecked((long)0x00000000F3F4F1F2), (long)PARAM.FromLowHighUnsigned(0xF1F2, 0xF3F4));
+    Assert.Equal(unchecked((long)0xFFFFFFFFF3F4F1F2), (long)PARAM.FromLowHighUnsigned(0xF1F2, 0xF3F4));
}

The only thing using Interop.PARAM.FromLowHighUnsigned() currently is the TaskDialog to set its progress bar range. Its possible the overflow could be triggered from there somehow.

I would appreciate to hear if the test System.Windows.Forms.Primitives.Tests.Interop.PARAMTests.FromLowHighUnsigned_x32_Result has been failing for anyone else and what your thoughts are.

Thank you.

[weltkante]

Since we are now casting to nuint, we no longer need to cast to uint.

This is not correct, did you read the discussion we had in the linked PR?

Changing both sign and size of an integer in one operation is ambigous (which operation happens first - sign change or widening? - it produces different results!) and has previously lead to bugs. Obviously the compiler picks one meaning, but even if it happens to match what you intend, being explicit is always better because the reader knows what is meant without looking it up.

Casting first to uint, then widening to nuint, then changing sign again, is explicit in the order of operations and doesn't allow for mistakes.

Now its possible that there is some bug hidden somewhere, normally C# doesn't throw overflow exceptions for arithmetic, but in your stacktrace I'm seeing it being thrown from IntPtr code and not from arithmetic, so maybe that exception is hardcoded. Makes me wonder why nobody noticed it though, especially if you claim seeing it happening as test failures. I'll have a look if I can repro.

Making the suggested change will fix FromLowHighUnsigned_x32_Result but will break another PARAM test FromLowHighUnsigned_x64_Result because that test is currently expecting an incorrect result for one of its assertions.

I'm not sure if its incorrect, I'd have to double check the C/C++ definition, but given what I said above about ambiguity my first guess would be that you seem to have broken the order of operations and now produce incorrect results with your proposed fix?

I would like to know if this test is failing for anyone else. This test fails for me on all build platforms.

I haven't seen it failing, no. I'll double check though.

[RussKie]

and neither work - this is a VS 2022 bug (or two, depending on why ConditionalFact is broken).

Would you mind raising a VS Feedback for this please?

[weltkante]

Would you mind raising a VS Feedback for this please?

Can do for the bitness selection, but do we know where the ConditionalFact attribute is defined? Its pulled in from some Microsoft XUnit extensions via arcade. (When building the repro I just referenced the assembly directly instead of the nuget package because I couldn't figure out where its defined.)

[RussKie]

I believe we get it from https://github.com/dotnet/arcade/blob/e7ede87875f41a9b3df898ae08da5ebc96e24f56/src/Microsoft.DotNet.XUnitExtensions/src/Attributes/ConditionalFactAttribute.cs

[weltkante]

reported:

• https://developercommunity.visualstudio.com/t/vs-2022-always-runs-tests-as-x64-even-when-selecti/1525842
• ConditionalFact does not work from within VS arcade#7862

[RussKie]

Thank you