Correctly handle use of cgt and others for object refs (fixes #3465)

[ElektroKill]

Link to issue(s) this covers
#3465

Problem

ILSpy did not handle use of cgt for object reference comparison where neither side was ldnull.

This is allowed as stated in ECMA:
"cgt.un is allowed and verifiable on ObjectRefs (O). This is commonly used when comparing an ObjectRef with null (there is no “compare-not-equal” instruction, which would otherwise be a more obvious solution)" - ECMA III.1.5 - Operand type table, Table III.4: Binary Comparison or Branch Operations, Note 2

Code checks for other comparison types just like it did before for ldnull but now for all object refs.

Solution

• No test currently as I'm yet to see a file using this.

@greenozon Could you verify if this changes fixes your issue on your file?

[greenozon]

Thanks!
will give it a try tomorrow and ping back!

[dgrunwald]

But is this correct? Is the JIT silently switching > to !=? Or is actually comparing the numeric values of the object pointers? (which would only have reliable runtime behavior if at least one if the input pointers is null)

[ElektroKill]

But is this correct? Is the JIT silently switching > to !=? Or is actually comparing the numeric values of the object pointers? (which would only have reliable runtime behavior if at least one if the input pointers is null)

Looks like I rushed this a bit too much and didn't really read into it! ECMA says its only valid for null as implemented in ILSpy already and modern C# JIT appears to treat them as pointers (A quick glance at dotnet/runtime reveals no special handling of cgt.un for object refs) so this change is not valid on IL level. This would probably explain why I couldn't get the C# compiler to emit the pattern seen in the snippet in the issue.

The current way of rewriting these comparisons on the IL level is incorrect. The only other alternative I see to decompile this properly would be to handle this case on the C# AST level or when translating ILAst to C# AST. Would this be appropriate, and if so, which way would be preferred?

[greenozon]

test is positive, but well, comments are not so..

[dgrunwald]

It's probably an obfuscator hiding nulls in static fields?

Am I guessing correctly that Class13.image_0 is always null?

I'm not sure what we should do about this, if anything. Unsafe.IsAddressGreaterThan only works for references, not object pointers. So I'm not sure if there is any semantics-preserving C# expression for this IL.
On the other hand, the IL only has semantics worth preserving if one of the inputs is null (the output of the comparison is nondeterministic otherwise).
Still, someone might use ILSpy to debug a compiler they're writing, in which case we shouldn't silently fix bugs in the generated IL code.
Maybe we could output != but with a warning comment?

[ElektroKill]

It's probably an obfuscator hiding nulls in static fields?

Am I guessing correctly that Class13.image_0 is always null?

This is actually something I have not considered! It may very well be the case here, as it is something decently common in obfuscation. @greenozon, could you confirm? If this is, in fact, the case, we can close the PR and issue, I'd say, since it's not something the C# compiler will/has produced.

Maybe we could output != but with a warning comment?

If this doesn't end up as an obfuscation issue, I think this is the way that makes the most sense.

[ds5678]

Unsafe.IsAddressGreaterThan only works for references, not object pointers. So I'm not sure if there is any semantics-preserving C# expression for this IL.

This is one way to do it. However, it depends on the memory layout of TypedReference.

public static unsafe bool GreaterThan(object left, object right)
{
    TypedReference leftTypedReference = __makeref(left);
    void* leftPointer = **(void***)&leftTypedReference;
    TypedReference rightTypedReference = __makeref(right);
    void* rightPointer = **(void***)&rightTypedReference;
    return leftPointer > rightPointer;
}

https://sharplab.io/#v2:D4AQTAjAsAUAxAOwK4BsUEMBGKCmACAEwEsBnLXWEAZj3DwGE8BvWPN2mkCANjyQTIAzfJgD2olHgDiAJxzoALjhkAVABboEAClGYAVjgDGCvLkEKANHl0HjeGUQDmahQEpW7FjHY+8KgJ4ADjgEAEo4wnIIhvhmCgHBYRHKONH4ALx4APpZALboANYpglpxrgDcHr60ACwAVKYRCgAKokQISjJ4mXV1WiD1va4AZHEJIeGRqTGV3tV+QRPJUTH2Ti7jSVNp3dl5hcVaDs5us/O1Dccure2du739g3UjV/GLWylpZ/MgAOyN5huHWUeAAfGsTkDOt82ABfWCwoA=

[ds5678]

Or maybe just this? It requires C# 11.

public static unsafe bool GreaterThan(object left, object right)
{
    return *(void**)&left > *(void**)&right;
}

https://sharplab.io/#v2:D4AQTAjAsAUAxAOwK4BsUEMBGKCmACAEwEsBnLXWEAZj3DwGE8BvWPN2mkCANjyQTIAzfJgD2olHgDiAJxzoALjhkAVABboEAClGYAVjgDGCvLkEKANHl0HjeGUQDmahQEpW7FjHY/aAdjwAKi0QABZAwNcAMjMTAD4gkPDIqIdnBQBuDzYAX1gcoA==

[dgrunwald]

Yes, *(void**)&left > *(void**)&right should work.
&left could maybe be replaced with Unsafe.AsPointer(ref left) for older C# versions?

Both of these ideas have the problem of requiring an lvalue object pointer, so we'd need to avoid inlining in these cases; or un-inline later (via ILInstruction.Extract, though this isn't guaranteed to succeed for arbitrarily complex constructs).

[dgrunwald]

Actually there's no reason to use an unmanaged pointer for the second level of indirection.
Unsafe.As<object, UIntPtr>(ref left) > Unsafe.As<object, UIntPtr>(ref right) seems like it'd be more reliable, as now we'd support the left expression pointing to the heap (e.g. parameter in function with yield-return) without getting in trouble with the GC.