Add more VB operator tests

[hughbe]

Contributes to https://github.com/dotnet/corefx/issues/31407

[danmoseley]

Great tests but I'm nervous about deleting "dead" code given this is for compat (and I'm not familiar enough with VB to easily persuade myself it is dead)

I'll ask @OmarTawfik to sign off on this one as a VB guy.

[hughbe]

Dead code, as GetTypeCode is implemented as

Private Shared Function GetTypeCode(ByVal o As Object) As TypeCode
    If o Is Nothing Then
        Return TypeCode.Empty
    Else
        Return o.GetType.GetTypeCode
    End If
End Function

and we return Boxed_ZeroInteger in the TypeCode.Empty case directly below this

[hughbe]

Dead code, as GetTypeCode is implemented as

Private Shared Function GetTypeCode(ByVal o As Object) As TypeCode
    If o Is Nothing Then
        Return TypeCode.Empty
    Else
        Return o.GetType.GetTypeCode
    End If
End Function

so tc == TypeCode.Empty if Operand Is Nothing

[hughbe]

We have tests negating decimal.MinValue - this does not cause an overflow exception

yield return new object[] { decimal.MinValue, decimal.Parse("79228162514264337593543950335") };

[OmarTawfik]

Can we add tests with MaxValue as well (applicable to others)?

---

In reply to: 204927252 [](ancestors = 204927252)

[hughbe]

This can't overflow, and the degenerate case, if left == Int16.MinValue and right == -1 does not overflow and returns 0

[OmarTawfik]

Do we have tests for these?

---

In reply to: 204927383 [](ancestors = 204927383)

[hughbe]

This can't overflow, and the degenerate case, if left == Int16.MinValue and right == -1 does not overflow and returns 0

But I have special cased the degenerate case to match ModInt64:

         Private Shared Function ModInt64(ByVal left As Int64, ByVal right As Int64) As Object
 
             If left = Int64.MinValue AndAlso right = -1 Then
                 Return 0L
             Else
                 Return left Mod right
             End If

[OmarTawfik]

Is the first line in the comment applicable here? also, do we have tests for these?

---

In reply to: 204927534 [](ancestors = 204927534)

[OmarTawfik]

Left a few questions.

[hughbe]

Added more tests to cover your questions

[hughbe]

@OmarTawfik do you mind helping me get to the bottom of these test failures? It passes on my mac and on a windows VM but there seem to be test failures on CI

Example

Assert.Equal() Failure
                             ↓ (pos 19)
Expected: -1,58456325028529E+43
Actual:   -1,58456325028529E+29
                             ↑ (pos 19)

[danmoseley]

The Windows.10.Amd64.ClientRS4.ES failures are on Spanish culture machines. I think tests like MultiplyObject_Convertible_ReturnsExpected(left: 15, right: 3,402823E+38, expected: 5,10423519957793E+53) failures relate to a comma decimal point. You can change this on your Windows machine and repro I expect.

ModObject_DivideByZeroObject_ThrowsDivideByZeroException(left: True, right: null) is passing on Windows.10.Amd64.ClientRS4.Open which I think is NETFX suggesting your expected is wrong?

ConcatenateObject_InvalidObjects_ThrowsInvalidCastException(left: 2, right: ) is the reverse, suggesting a discrepancy in the ported implementation

[danmoseley]

As you know you can run tests against NETFX locally after you build with build -framework:netfx and build-tests -framework:netfx -skiptests you can do msbuild /t:buildandtest /p:targetgroup=netfx in your project.

[hughbe]

Yup - thats a bug in the implementation of ConcatenateObject and this also led me to discover bugs in the handling of DBNull.

After this PR is merged I'll put together more test cases for the cases of DBNull and IConvertible etc.

[danmoseley]

ModObject_DivideByZeroObject_ThrowsDivideByZeroException(left: True, right: null) still failing

[hughbe]

Hmm this is weird. I can't seem to reproduce this locally on Windows or Unix. The following throws DVE in my tests, and so does Console.WriteLine(Operators.ModObject(True, Nothing))

Imports System
Imports Microsoft.VisualBasic.CompilerServices
				
Public Module Module1
	Public Sub Main()
		Dim left as Int16 = -1
		Dim right as Int16 = 0
		Dim y as Int16 = left Mod right
	End Sub
End Module

These test failures are only happening in Release mode. I'm baffled!

[hughbe]

@OmarTawfik friendly ping - do you have any ideas?

[hughbe]

@mikedn do you think the test failures here in release mode for Mod by 0 are related to https://github.com/dotnet/coreclr/issues/8648

/cc @AndyAyersMS @briansull

[mikedn]

Is there some simple repro? It's not very clear to me which test fails.

As far as I can tell the failing test involves ModObject(Object, Object). That's a very large method that won't inline and anyway it would require quite a bit of optimization to reduce down to 8648.

[hughbe]

Okay so the failing test is only failing in Release mode which is firstly a bit strange.

It is in the following code

public static IEnumerable<object[]> ModObject_DivideByZeroObject_TestData()
{
    yield return new object[] { (byte)20, false };
    yield return new object[] { (byte)20, null };
    yield return new object[] { (byte)20, byte.MinValue };
    yield return new object[] { byte.MinValue, byte.MinValue };
    yield return new object[] { byte.MaxValue, byte.MinValue };
    yield return new object[] { byte.MaxValue, byte.MinValue };
    yield return new object[] { byte.MaxValue, byte.MinValue };
    yield return new object[] { byte.MinValue, byte.MinValue };
    yield return new object[] { byte.MinValue, byte.MinValue };
    yield return new object[] { byte.MinValue, byte.MinValue };
    yield return new object[] { (sbyte)20, false };
    yield return new object[] { (sbyte)20, null };
    yield return new object[] { (sbyte)20, (sbyte)0 };
    yield return new object[] { (sbyte)0, (sbyte)0 };
    yield return new object[] { (sbyte)(-20), (sbyte)0 };
    yield return new object[] { sbyte.MaxValue, (sbyte)0 };
    yield return new object[] { sbyte.MinValue, (sbyte)0 };
    yield return new object[] { (ushort)20, false };
    yield return new object[] { (ushort)20, null };
    yield return new object[] { (ushort)20, ushort.MinValue };
    yield return new object[] { ushort.MinValue, ushort.MinValue };
    yield return new object[] { ushort.MaxValue, ushort.MinValue };
    yield return new object[] { ushort.MaxValue, ushort.MinValue };
    yield return new object[] { ushort.MaxValue, ushort.MinValue };
    yield return new object[] { ushort.MinValue, ushort.MinValue };
    yield return new object[] { ushort.MinValue, ushort.MinValue };
    yield return new object[] { ushort.MinValue, ushort.MinValue };
    yield return new object[] { (short)20, false };
    yield return new object[] { (short)20, null };
    yield return new object[] { (short)20, (short)0 };
    yield return new object[] { (short)0, (short)0 };
    yield return new object[] { (short)(-20), (short)0 };
    yield return new object[] { short.MaxValue, (short)0 };
    yield return new object[] { short.MinValue, (short)0 };
    yield return new object[] { (uint)20, false };
    yield return new object[] { (uint)20, null };
    yield return new object[] { (uint)20, uint.MinValue };
    yield return new object[] { uint.MinValue, uint.MinValue };
    yield return new object[] { uint.MaxValue, uint.MinValue };
    yield return new object[] { uint.MaxValue, uint.MinValue };
    yield return new object[] { uint.MaxValue, uint.MinValue };
    yield return new object[] { uint.MinValue, uint.MinValue };
    yield return new object[] { uint.MinValue, uint.MinValue };
    yield return new object[] { uint.MinValue, uint.MinValue };
    yield return new object[] { 20, false };
    yield return new object[] { 20, null };
    yield return new object[] { 20, 0 };
    yield return new object[] { 0, 0 };
    yield return new object[] { -20, 0 };
    yield return new object[] { int.MaxValue, 0 };
    yield return new object[] { int.MinValue, 0 };
    yield return new object[] { (ulong)20, false };
    yield return new object[] { (ulong)20, null };
    yield return new object[] { (ulong)20, ulong.MinValue };
    yield return new object[] { ulong.MinValue, ulong.MinValue };
    yield return new object[] { ulong.MaxValue, ulong.MinValue };
    yield return new object[] { ulong.MaxValue, ulong.MinValue };
    yield return new object[] { ulong.MaxValue, ulong.MinValue };
    yield return new object[] { ulong.MinValue, ulong.MinValue };
    yield return new object[] { ulong.MinValue, ulong.MinValue };
    yield return new object[] { ulong.MinValue, ulong.MinValue };
    yield return new object[] { (long)20, false };
    yield return new object[] { (long)20, null };
    yield return new object[] { (long)20, (long)0 };
    yield return new object[] { (long)0, (long)0 };
    yield return new object[] { (long)(-20), (long)0 };
    yield return new object[] { long.MaxValue, (long)0 };
    yield return new object[] { long.MinValue, (long)0 };
    yield return new object[] { (decimal)20, false };
    yield return new object[] { (decimal)20, null };
    yield return new object[] { (decimal)20, (decimal)0 };
    yield return new object[] { (decimal)0, (decimal)0 };
    yield return new object[] { (decimal)(-20), (decimal)0 };
    yield return new object[] { decimal.MaxValue, (decimal)0 };
    yield return new object[] { decimal.MinValue, (decimal)0 };
    yield return new object[] { true, false };
    yield return new object[] { true, null };
    yield return new object[] { null, false };
    yield return new object[] { null, null };
}

[Theory]
[MemberData(nameof(ModObject_DivideByZeroObject_TestData))]
public void ModObject_DivideByZeroObject_ThrowsDivideByZeroException(object left, object right)
{
    Assert.Throws<DivideByZeroException>(() => Operators.ModObject(left, right));
}

The error message is

Mod​Object_Divide​By​Zero​Object_Throws​Divide​By​Zero​Exception(left: True, right: null)
Assert.Throws() Failure
Expected: typeof(System.DivideByZeroException)
Actual:   (No exception was thrown)
Stack Trace :
   at Microsoft.VisualBasic.CompilerServices.Tests.OperatorsTests.ModObject_DivideByZeroObject_ThrowsDivideByZeroException(Object left, Object right) in D:\j\workspace\windows-TGrou---f8ac6754\src\Microsoft.VisualBasic\tests\OperatorsTests.cs:line 3337

[mikedn]

Thanks, this is indeed very likely to be 8648. ModObject is too large to be inlined and expose the bug but inside it there are calls like Return ModSByte(Convert.ToSByte(left), Nothing). I don't know VB but it seems that Nothing is implicitly converted to 0 so this calls ModSByte(SByte, SByte). This is small enough to be inlined so we end up with x % 0.

Minimal VB repro:

Module Program
    Sub Main()
        Dim x As Object = Convert.ToSByte(42)
        Dim y As Object = Nothing
        Console.WriteLine(x Mod y)
    End Sub
End Module

This prints 42 with the released .NET Core 2.1.

The relevant ModObject disassembly is

       488BCE               mov      rcx, rsi
       E8F6C77C59           call     System.Convert:ToSByte(ref):byte
       480FBEF0             movsx    rsi, al
       48B94042EA6CFF7F0000 mov      rcx, 0x7FFF6CEA4240
       E863BD815F           call     CORINFO_HELP_NEWSFAST
       83FE80               cmp      esi, -128
       0F8CBF080000         jl       G_M19509_IG65
       83FE7F               cmp      esi, 127
       0F8FB6080000         jg       G_M19509_IG65
       8BCE                 mov      ecx, esi
       884808               mov      byte  ptr [rax+8], cl
       E9A0080000           jmp      G_M19509_IG63

There's no trace of idiv in it. It simply returns the value returned by ToSByte(left).

A fix for 8648 is in progress in dotnet/coreclr#19284.

Not sure what workarounds are available until that's done. You can try to block inlining of methods such as ModSByte by using the <MethodImpl(MethodImplOptions.NoInlining)> attribute. Or make the 0 invisible to the JIT optimizer by loading it from a static (non-readonly) variable.

[danmoseley]

@dotnet-bot test this please

[karelz]

@danmosemsft ready for merge?

[briansull]

The Fix for VB Mod by zero has now been checked in with

Full support for exception sets in value numbering. #20129