Fix incorrect behavior noted via asserts

[davidwrighton]

• Multi-thread handling of UnboxingMethodDesc was incorrect. It would result in incorrect caching of fixups to unboxing methods. This would cause asserts and determinism failures.
• ldtoken of a static method on a valuetype should not refer to an unboxing stub, This is unlikely to cause a failure, but will cause asserts to fire

[trylek]

Looks great, thanks for fixing!

[MichalStrehovsky]

Per the comment for the UnboxingMethodDesc:

    /// This class is for internal purposes within the JitInterface. It's not expected
    /// for it to escape the JitInterface.

Is this MethodDesc escaping the local CorInfoImpl class?

I don't think the rest of the system is prepared to handle a MethodDesc to an unboxing method. We explicitly designed the type system not to have a unboxing MethodDesc (and @davidwrighton was one of the proponents of that approach). I don't think the rest of the system is prepared to handle seeing it and we really don't want it to escape. We only have unboxing MethodDescs as a local wart so that we can hand something to RyuJIT while compiling a method and it's supposed to go away after.

CorInfoImpl is expected to unwrap it into things the rest of the system can handle (a bool specifying whether this is an unboxing thunk - like most of crossgen2 is doing). The fact that this is not happening sounds like the bug we should fix instead of introducing a static holding onto type system object. I really wish there was a Roslyn analyzer to block static fields - we should never have statics holding onto type system objects.

Can you please CC crossgen-contrib on crossgen2 changes in the future?

[davidwrighton]

It IS escaping the CorInfoImpl class. I had forgotten about that portion of the design. We should open a bug and fix the problem.

[MichalStrehovsky]

We should open a bug and fix the problem.

Filed #35531.