[arm32/Linux] System.Runtime.Tests failures on arm32 linux

[joperezr]

Running the arm32 tests on linux for System.Runtime.Tests produced the following results: passed 31235/31245 tests.

All 10 failing tests have the same message which is:

Assert.Throws() Failure\nExpected: typeof(System.ArgumentOutOfRangeException)\nActual:   (No exception was thrown)
   at System.AssertExtensions.Throws[T](String paramName, Func`1 testCode) in /root/corefx-2261125/src/CoreFx.Private.TestUtilities/src/System/AssertExtensions.cs:line 88
   at System.Tests.DateTimeTests.AddHours_NewDateOutOfRange_ThrowsArgumentOutOfRangeException(DateTime date, Double hours) in /root/corefx-2261125/src/System.Runtime/tests/System/DateTimeTests.cs:line 493

[joperezr]

cc @tarekgh

[tarekgh]

This will be good to be looked by the jit guys.

[danmoseley]

Right the math we are doing here is very simple, it is most likely in code gen.

[danmoseley]

@RussKeldorph we suspect codegen here as we are doing simple math which is expected to overflow. The same issue occurs on Linux and Windows - but only on ARM.

[danmoseley]

Or possibly runtime, if they have an ARM specific codepath (it can't be the C library)

[RussKeldorph]

@BruceForstall Could you investigate?

[RussKeldorph]

@dotnet/jit-contrib

[BruceForstall]

Yes. Note that we have a pretty fundamental corefx test run regression now on ARM that may block investigation: https://github.com/dotnet/coreclr/issues/21464.

[wfurt]

The issue seems to be caused by handling floating points on ARM. One of the failing tests is trying to add double.MaxValue to DateTime.Now.

In coreclr repo src/System.Private.CoreLib/shared/System/DateTime.cs

       private DateTime Add(double value, int scale)
        {
            long millis = (long)(value * scale + (value >= 0 ? 0.5 : -0.5));
            if (millis <= -MaxMillis || millis >= MaxMillis)
                throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_AddValue);
            return AddTicks(millis * TicksPerMillisecond);
        }

On both x64 and ARM3 passed in values are value=1.79769313486232E+308 scale=86400000

However on x64 long millis become -9223372036854775808 but on arm32 this will be -1. That is than passed down as -10000 to AddTicks() and that will not trigger OutOfRange when added to current time.

Note that I have seen similar issues in the past with floats causing HTTP test failures. I was running HTTP tests in QEMU emulator and issues with daytime caused troubles.

[BruceForstall]

@wfurt It looks to me like either System.DateTime.Add() is incorrectly handling overflow (namely, not handling it in cases where it may occur), or it is allowed to do "unspecified" things on overflow, and the test is invalid.

In particular, this line:

long millis = (long)(value * scale + (value >= 0 ? 0.5 : -0.5));

where value = double.MaxValue and scale = MillisPerHour (86400000), produces an overflow in the multiplication. This is not checked for. It's possible that floating-point overflow is well-defined in IL; I'm not sure. However, the cast to long is done with a conv.i8 instruction, and the CLI spec (Partition III, 3.27) states, "If overflow occurs converting a floating-point type to an integer, or if the floating-point value being converted to an integer is a NaN, the value returned is unspecified.". The fact that x64 produces -9223372036854775808 seems pretty random and not useful.

[danmoseley]

Question is whether to simply remove the test, as it's undefined behavior, or make the product math checked, which perhaps has some performance penalty.

[tarekgh]

I think the test is correct as it should throw argument out of range exception.

[BruceForstall]

Perhaps the ArgumentOutOfRangeException checking should be pulled into the callers of this Add, like AddHours, which can be more careful about the range before passing on to this Add. Though perhaps this multiply should somehow be checked also.

[wfurt]

It somewhat surprise me that the floating arithmetic at c# level would be still platform dependent. I agree with @tarekgh: This test is valid and it should throw.

Maybe we should be smarter about +-0.5 to avoid overflow.
cc: @janvorli @jkotas

[jkotas]

The behavior of long to double conversions is unspecified on overflow as @BruceForstall said.

The fix should be to do the range check on double before converting it to long.

[CarolEidt]

Floating point arithmetic in which intermediate values are not explicitly cast to float or double is platform-dependent. This is to ensure best performance across platforms. Platform-independent behavior can be obtained by adding checks and/or casts.

I agree with @jkotas that the range checking should be done before the conversion.

[BruceForstall]

@jkotas @CarolEidt So with:

long millis = (long)(value * scale + (value >= 0 ? 0.5 : -0.5));

where value = double.MaxValue and scale = MillisPerHour (86400000) will the expression (before cast) resolve to +Infinity on all platforms? So the issue is the platform-dependent conversion of that (or something less than Infinity but greater than Int64.MaxValue)?

So maybe Add() should be:

   private DateTime Add(double value, int scale)
        {
            double millis_double = value * (double)scale + (value >= 0 ? 0.5 : -0.5);
            if (millis_double <= (double)-MaxMillis || millis_double >= (double)MaxMillis)
                throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_AddValue);
            long millis = (long)millis_double;
            return AddTicks(millis * TicksPerMillisecond);
        }

[BruceForstall]

And maybe there should be an

Assert(MaxMillis <= Int64.MaxValue)

[BruceForstall]

Although I guess that's unnecessary if MaxMillis is typed long

[tarekgh]

@BruceForstall I believe this implementation is reasonable.

[tarekgh]

can we try it on ARM to ensure this will work?

[wfurt]

do you want me to run some tests or do you plan to put up PR @BruceForstall ? You seems to be faster than me...

[tarekgh]

at least we need to test the failed case to ensure it fix the issue. we already have other tests in the CI which exercise the functionality that should catch regressions

[BruceForstall]

I'll let you make the change; I don't have any experience working in the framework code.

[wfurt]

ok. I have build and test machines ready (thanks to @RussKeldorph ) so it is easy for me to try.

[tarekgh]

thanks @wfurt

[wfurt]

Proposed change fixed all DayTime tests. I'll do more testing on X64 tomorrow and Ill create PR.
What remains failing are following three tests:

      <test name="System.Tests.ExceptionDataTests.ICollection_NonGeneric_SyncRoot(count: 0)" type="System.Tests.ExceptionDataTests" method="ICollection_NonGeneric_SyncRoot" time="0.0298165" result="Fail">
      <test name="System.Tests.ExceptionDataTests.ICollection_NonGeneric_SyncRoot(count: 1)" type="System.Tests.ExceptionDataTests" method="ICollection_NonGeneric_SyncRoot" time="0.01636" result="Fail">
      <test name="System.Tests.ExceptionDataTests.ICollection_NonGeneric_SyncRoot(count: 75)" type="System.Tests.ExceptionDataTests" method="ICollection_NonGeneric_SyncRoot" time="0.0073205" result="Fail">

I'll take close look tomorrow as well.

[danmoseley]

The sync root changes are probably fallout of my changes a few days ago to remove syncroot field on our collections. I fixed all the tests though, I thought. Is it possible corefx/coreclr/tests are not all in a matching state here?

[wfurt]

it is certainly possible. I reused my build trees from last week. I can give it fresh or let CI do for me.

      System.Tests.ExceptionDataTests.ICollection_NonGeneric_SyncRoot(count: 75) [STARTING]
      System.Tests.ExceptionDataTests.ICollection_NonGeneric_SyncRoot(count: 75) [FAIL]
        Assert.IsType() Failure
        Expected: System.Object
        Actual:   System.Collections.ListDictionaryInternal
        Stack Trace:
          /home/toweinfu/github/wfurt-corefx-arm/src/Common/tests/System/Collections/ICollection.NonGeneric.Tests.cs(153,0): at System.Collections.Tests.ICollection_NonGeneric_Tests.ICollection_NonGeneric_SyncRoot(Int32 count)
      System.Tests.ExceptionDataTests.ICollection_NonGeneric_SyncRoot(count: 75) [FINISHED] Time: 0.0073205s