views:

1748

answers:

10

Hi, guys. Here is the example with comments:

class Program
{
    // first version of structure
    public struct D1
    {
        public double d;
        public int f;
    }

    // during some changes in code then we got D2 from D1
    // Field f type became double while it was int before
    public struct D2 
    {
        public double d;
        public double f;
    }

    static void Main(string[] args)
    {
        // Scenario with the first version
        D1 a = new D1();
        D1 b = new D1();
        a.f = b.f = 1;
        a.d = 0.0;
        b.d = -0.0;
        bool r1 = a.Equals(b); // gives true, all is ok

        // The same scenario with the new one
        D2 c = new D2();
        D2 d = new D2();
        c.f = d.f = 1;
        c.d = 0.0;
        d.d = -0.0;
        bool r2 = c.Equals(d); // false! this is not the expected result        
    }
}

So, what do you think about this?

+5  A: 

It does give true for me, with Mono's gmcs 2.4.2.3.

Matthew Flaschen
Curiouser and curiouser...
Michael La Voie
Yes, I've also tried it in Mono, and it gives me true too. Looks like MS does some magic inside :)
Alexander Efimov
interesting, we all ship to Mono?
WeNeedAnswers
+2  A: 

Simpler test case:

Console.WriteLine("Good: " + new Good().Equals(new Good { d = -.0 }));
Console.WriteLine("Bad: " + new Bad().Equals(new Bad { d = -.0 }));

public struct Good {
    public double d;
    public int f;
}

public struct Bad {
    public double d;
}

EDIT: The bug also happens with floats, but only happens if the fields in the struct add up to a multiple of 8 bytes.

SLaks
Looks like an optimizer rule that goes: if its all doubles than do a bit-compare, else do separate double.Equal calls
Henk Holterman
I don't think this is the same test case as what the issue presented here seems to be is that the default value for Bad.f is not 0, whereas the other case seems to be an Int vs. Double issue.
Driss Zouak
@Driss: The default value for `double` _is_ `0`. You're wrong.
SLaks
A: 

It must be zero related, since changing the line

d.d = -0.0

to:

d.d = 0.0

results in the comparison being true...

+4  A: 

It must be related to a bit by bit comparison, since 0.0 should differ from -0.0 only by the signal bit.

João Angelo
+5  A: 

Half an answer:

Reflector tells us that ValueType.Equals() does something like this:

if (CanCompareBits(this))
    return FastEqualsCheck(this, obj);
else
    // Use reflection to step through each member and call .Equals() on each one.

Unfortunately both CanCompareBits() and FastEquals() (both static methods) are extern ([MethodImpl(MethodImplOptions.InternalCall)]) and have no source available.

Back to guessing why one case can be compared by bits, and the other cannot (alignment issues maybe?)

Vilx-
A: 

if you make D2 like this

    public struct D2
    {
        public double d;
        public double f;
        public string s;
    }

it's true.

if you make it like this

        public struct D2
    {
        public double d;
        public double f;
        public double u;
    }

It's still false.

it seems like it's false if the struct only holds doubles..

Morten Anderson
+84  A: 

The bug is in the following two lines of System.ValueType: (I stepped into the reference source)

if (CanCompareBits(this)) 
    return FastEqualsCheck(thisObj, obj);

(Both methods are [MethodImpl(MethodImplOptions.InternalCall)])

When the all of the fields are 8 bytes wide, CanCompareBits mistakenly returns true, resulting in a bitwise comparison of two different, but semantically identical, values.

When at least one field is not 8 bytes wide, CanCompareBits returns false, and the code proceeds to use reflection to loop over the fields and call Equals for each value, which correctly treats -0.0 as equal to 0.0.

Here is the source for CanCompareBits from SSCLI:

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

    _ASSERTE(obj != NULL);
    MethodTable* mt = obj->GetMethodTable();
    FC_RETURN_BOOL(!mt->ContainsPointers() && !mt->IsNotTightlyPacked());
}
FCIMPLEND
SLaks
This definitely deserved the checkmark. Nice work.
Ben M
Stepping into System.ValueType? That's pretty hardcore bro.
Pierreten
A: 

I can only think it is some quirk in the compiler. -0.0 and 0.0 should still be the same at the bit level, unless for some reason 0.0 is being stored as an unsigned number, which will differ at the bit level from 0.0 as a singed number...

actually... no it shouldn't... which ever way you store 0.0 it is still 0.0

I really don't see any reason for this behaviour.

thecoshman
That is actually by design. Read IEEE 754.
SLaks
Since floating point numbers are inherently inexact, there isn't really a concept of zero in IEEE floating point numbers. Instead there are two values called +0 and -0, which mean "a positive or negative number that is very, very small." This is why division-by-zero is typically not an error in floating point math. It just results in an "infinity", which, of course, doesn't really mean infinite. :)
Jeffrey L Whitledge
+29  A: 

I found the answer at http://blogs.msdn.com/xiangfan/archive/2008/09/01/magic-behind-valuetype-equals.aspx.

The core piece is the source comment on CanCompareBits, which ValueType.Equals uses to determine whether to use memcmp-style comparison:

The comment of CanCompareBits says "Return true if the valuetype does not contain pointer and is tightly packed". And FastEqualsCheck use "memcmp" to speed up the comparison.

The author goes on to state exactly the problem described by the OP:

Imagine you have a structure which only contains a float. What will occur if one contains +0.0, and the other contains -0.0? They should be the same, but the underlying binary representation are different. If you nest other structure which override the Equals method, that optimization will also fail.

Ben M
+1 You beat me to it with a minute. Speed counts :)
Fredrik Mörk
+14  A: 

Vilx's conjecture is correct. What "CanCompareBits" does is checks to see whether the value type in question is "tightly packed" in memory. A tightly packed struct is compared by simply comparing the binary bits that make up the structure; a loosely packed structure is compared by calling Equals on all the members.

This explains SLaks' observation that it repros with structs that are all doubles; such structs are always tightly packed.

Unfortunately as we've seen here, that introduces a semantic difference because bitwise comparison of doubles and Equals comparison of doubles gives different results.

Eric Lippert
Then why it isn't a bug? Even though MS recommends to override Equals on value types always.
Alexander Efimov
For obvious reasons, it also repro's with an `IntPtr` field when running 64-bit. However, it doesn't repro with an 8-byte-wide struct field. Why aren't nested structs tightly-packed?
SLaks
Beats the heck out of me. I'm not an expert on the internals of the CLR.
Eric Lippert
... You aren't? Surely your knowledge of the C# internals would lead to considerable knowledge on how the CLR works.
CaptainCasey
@CaptainCasey: I've spent five years studying the internals of the C# compiler and probably in total a couple of hours studying the internals of the CLR. Remember, I am a *consumer* of the CLR; I understand its public surface area reasonably well, but its internals are a black box to me.
Eric Lippert
My mistake, I thought the CLR and the VB/C# compilers were more tightly coupled... so C#/VB -> CIL -> CLR
CaptainCasey