10 years old bug that migrated from .net to .net core. (System.Value.Equal)

[Licoze]

coreclr/src/vm/comutilnative.cpp

FCIMPL1(FC_BOOL_RET, ValueTypeHelper::CanCompareBits, Object* obj)
{
    FCALL_CONTRACT;

    _ASSERTE(obj != NULL);
    MethodTable* mt = obj->GetMethodTable();
    FC_RETURN_BOOL(!mt->ContainsPointers() && !mt->IsNotTightlyPacked());//!!!
}
FCIMPLEND

You can see more information here:
https://blogs.msdn.microsoft.com/xiangfan/2008/09/01/magic-behind-valuetype-equals/
http://stackoverflow.com/questions/2508945/can-anyone-explain-this-strange-behavior-with-signed-floats-in-c/2509152

[mazong1123]

@karelz Anyone working on this? I'd like to sweep out these kind of old bugs :)

[tannergooding]

This would also end up impacting NaN comparisons.

[karelz]

It's yours if you want it :) @mazong1123
We do not have CoreCLR issues cleaned up the same way as CoreFX. Something for me to focus on in next few months ;)

[mazong1123]

@tannergooding How many special cases of value types can fall into this category? I mean those have different bit representation but considered as the "same" (e.g, +0.0 and - 0.0). I'm asking because I see we already specialized NaN in Double.cs so we probably should specialize all these cases here instead of modify ValueTypeHelper::CanCompareBits: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Double.cs#L184

        public override bool Equals(Object obj)
        {
            if (!(obj is Double))
            {
                return false;
            }
            double temp = ((Double)obj).m_value;
            // This code below is written this way for performance reasons i.e the != and == check is intentional.
            if (temp == m_value)
            {
                return true;
            }
            return IsNaN(temp) && IsNaN(m_value);
        }

We may need to add some methods like IsNegativeZero/IsPositiveZero.

[mazong1123]

I found GetHashCode has specialized +0.0 and -0.0 : https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Double.cs#L238

I don't know how the == operator works against double in CSharp, seems like we can simply use == operator to opt out 0 in double.Equals as well, if the performance is greater than or equal to using bit operations.

Also here is a related issue #6307 .

[tannergooding]

@mazong1123, for Single/Double It should just be -0.0/0.0 and all representations of NaN.

However, I am also not familiar with the checks that ValueType.Equals is doing to determine whether bit comparison is allowed. If there are other cases, like Single/Double, that are impacted (such as user-defined structs), then the fix needs to be one way, but if it only impacts types where the user never overrides Equals anywhere in the field hierarchy, then the proposed fix is "probably" fine.

[mazong1123]

However, I am also not familiar with the checks that ValueType.Equals is doing to determine whether bit comparison is allowed

I found IsNaN is actually doing bit comparison : https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Double.cs#L75
So I think we can use bit comparison for zeros if its performance is greater than using ==. Anyway, I'll measure it.

If there are other cases, like Single/Double, that are impacted (such as user-defined structs), then the fix needs to be one way

I think who defined the structs should be responsible for handling its own special cases - Like IsNaN for double/single we did here.

[tannergooding]

@mazong1123, yes, IsNaN checks against all NaN values simultaneously with a single compare.

I would, however, continue just comparing == 0.0 for zero. The underlying instructions of the CPU (at least for Intel) know how to handle the comparisons according to IEEE rules (0.0 == -0.0).

[tannergooding]

I think who defined the structs should be responsible for handling its own special cases

So the code today is broken for:

struct MyStruct
{
    public double value1;
    public double value2;
}

and presumably also for:

struct MyStruct1
{
    public double value1;

    public MyStruct2 value2;
}

struct MyStruct2
{
    public double value;
}

Both of these, should be fixed by special handling double.

I was calling out a scenario such as the following:

struct MyStruct1
{
    public double value1;

    public MyStruct2 value2;
}

struct MyStruct2
{
    public double value;

    public override bool Equals(object obj)
    {
        // custom equality
    }
}

I am hoping that a bit comparison is not done for this scenario, but I don't know enough about the checks being done to know that for certain (it would probably depend on what triggers IsTightlyPacked, since ContainsPointers would presumably return false in this case).