RyuJIT losing top bits of handles

[MichalStrehovsky]

In #121697 I'm seeing a failure in the interfacestatics test.

The problem is that in this part of the program:

runtime/src/tests/Regressions/coreclr/15647/interfacestatics.il

Lines 52 to 58 in 8c2b75f

	ldsfld class [System.Runtime]System.Type class IFoo<object>::O
	ldtoken object[]
	call class [System.Runtime]System.Type class [System.Runtime]System.Type::GetTypeFromHandle(valuetype [System.Runtime]System.RuntimeTypeHandle)
	ceq
	brtrue IFoo_object_O_Okay
	ldc.i4.1
	ret

we decide that the IFoo_object_O_Okay jump is never taken and the entire Main should compile to return 1.

The reason why we didn't see this failure before #121697 is that with TrimMode=partial, compiler is forced to assume the IFoo<object>::O field could be a target of reflection and therefore we can't optimize with the otherwise constant value of the field. With TrimMode full the optimizations start kicking in, and misfire.

Here's the JitDump:

dumpy.txt

The problematic part is:

Folding operator with constant nodes into a constant:
               [000004] J---G--N---                         *  EQ        int   
               [000000] H----+-----                         +--*  CNS_INT(h) ref     'ILCompiler.DependencyAnalysis.FrozenRuntimeTypeNode'
               [000069] H----+-----                         \--*  CNS_INT(h) ref     'ILCompiler.DependencyAnalysis.FrozenRuntimeTypeNode'
Bashed to int constant:
               [000004] -----------                         *  CNS_INT   int    0

and the reason why it got folded this way is that RyuJIT lost the top bits of the CNS_INT handle in one side of the comparison.

As to why we have top bits set in the handle:

runtime/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

Lines 712 to 738 in 8c2b75f

	#if DEBUG
	private static readonly IntPtr s_handleHighBitSet = (sizeof(IntPtr) == 4) ? new IntPtr(0x40000000) : new IntPtr(0x4000000000000000);
	#endif
	private IntPtr ObjectToHandle(object obj)
	{
	// MethodILScopes need to go through ObjectToHandle(MethodILScope methodIL).
	Debug.Assert(obj is not MethodILScope);
	return ObjectToHandleUnchecked(obj);
	}
	private IntPtr ObjectToHandleUnchecked(object obj)
	{
	// SuperPMI relies on the handle returned from this function being stable for the lifetime of the crossgen2 process
	// If handle deletion is implemented, please update SuperPMI
	IntPtr handle;
	if (!_objectToHandle.TryGetValue(obj, out handle))
	{
	handle = (IntPtr)(handleMultiplier * _handleToObject.Count + handleBase);
	#if DEBUG
	handle = new IntPtr((long)s_handleHighBitSet | (long)handle);
	#endif
	_handleToObject.Add(obj);
	_objectToHandle.Add(obj, handle);
	}
	return handle;
	}

Notice we set top bits, but do not enforce the top bits are present when the handle is used, so handles with stripped top bits are as good as any.

Is the bug that this should look at the pointer size of the target, not at the pointer size of the host?

runtime/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

Line 713 in 8c2b75f

private static readonly IntPtr s_handleHighBitSet = (sizeof(IntPtr) == 4) ? new IntPtr(0x40000000) : new IntPtr(0x4000000000000000);

Or is RyuJIT expected to preserve the top bits like this and the bug is in RyuJIT?

Cc @davidwrighton

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[jakobbotsch]

I recalled discussing this previously, and I dug up a comment chain about it: #77593 (comment)

[MichalStrehovsky]

Can the fix just be deleting s_handleHighBitSet? It doesn't seem to serve any purpose, except breaking things. If we were validating the bit is present it would be a different thing but we don't so the only thing it can do is cause these subtle bugs.

[jkotas]

David added it in https://github.com/dotnet/runtime/pull/41126/files#diff-4039de0dc0aa123aac5c4aadc4155f56e9dda87549b351875447c5ba73444213R401 . This change introduced support for cross-bitness cross-compilation among other things. I assume that s_handleHighBitSet was added to make it easier to catch cross-bitness bugs.

Deleting will fix this bug, but it may allow different bugs to sneak in unnoticed.

validating the bit is present

It would be nice to have that validation.

[jakobbotsch]

Going to give this to Egor since it's CORINFO_OBJECT_HANDLE stuff and it was the intended design when we discussed it previously.