Changed internal value to readonly for primitive types

[tgiphil]

See PR #18138 for more information.

[jkotas]

This will make the GetHashCode method significantly slower, I think.

[tgiphil]

Really? The BitConverter.Int64BitsToDouble() method is simply:

coreclr/src/mscorlib/shared/System/BitConverter.cs

Line 450 in dd74fca

return *((long*)&value);

Which seems more straightforward than Unsafe.As(). Any way to review the generated assembly?

[mikedn]

Which seems more straightforward than Unsafe.As().

It's not as Int64BitsToDouble will result in the double value being loaded as double, stored to a temporary and then loaded again as long. The Unsafe.As variant simply converts this (which is basically a ref in the case of structs) from ref double to ref long, that is a no-op.

[tgiphil]

@mikedn Can you suggest how to get Unsafe.AsRef to work? I couldn't figure it out.

[tgiphil]

Side questions: Can the compiler optimize away the cast by promoting the temporary into a register? And is there a X86/X64 instruction that can bit-copy from a floating point XMM register to a general purpose register?

[tgiphil]

@jkotas I made the change to Unsafe.cs; however, I'm not exactly sure what to do with jitinterface.cpp and mscorlib.h.

[jkotas]

Here you go

[mikedn]

We'll need to check if this approach doesn't impact code gen as well. Sometimes the JIT has problems with multiple calls - even if they are inlined it may still spill trees and introduce new local variables.

I'll try to take a look later today.

[mikedn]

Works fine, the JIT generates identical IR for both old and new implementations.

And it turns out that the original As call does result in a temporary variable being introduced. Fortunately it has no impact on the generated code.

[jkotas]

@mikedn Thanks for taking a look!

[mikedn]

@jkotas Should perhaps someone from Roslyn take a look as well? I vaguely remember a comment indicating that Roslyn treats primitive types as readonly anyway.

[jkotas]

It does in some cases, but not all cases. #18138 (comment) is an example where this makes a difference.

[jkotas]

@jaredpar Do you see any Roslyn issues with marking primitive types as readonly?

[jaredpar]

@jkotas

Do you see any Roslyn issues with marking primitive types as readonly?

No. We already consider such types to be readonly.

CC @OmarTawfik

[jaredpar]

I understand this is in the Unsafe class hence it's unsafe. I want to emphasize though that this operation is not just about violating readonly semantics. It actually can cause some pretty severe safety issues. Consider the following code as an example:

void Example(in object o) { 
  Unsafe.AsRef(in o) = new object(); // really dangerous
}

void M(string[] array) { 
  object[] objArray = array;
  Example(in objArray[0]);
}

Hence this operator allows writing objects that violate our basic type safety into locations. It's not just about violating readonly.

[jkotas]

Right. S.R.CS.Unsafe is a very sharp knife that lets introduce pretty severe safety issue if used incorrectly. This particular API shipped already. This change is just adding internal clone of the public API for internal CoreLib consumption.

[jkotas]

@tgiphil Could you please create a matching PR in corefx repo to update the reference assembly https://github.com/dotnet/corefx/tree/master/src/System.Runtime/ref ?

(It will not build successfully until this change propagated to CoreFX - it is fine for the PR to just sit there until that happens.)

[tgiphil]

PR at CoreFX: dotnet/corefx#30717

Please review.