Avoid Unsafe.As in BitConverter

[xtqqczze]

Doesn't depend upon implementation specific behaviour, i.e. that the array data is properly aligned.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

[stephentoub]

Why is this better?

[xtqqczze]

We should really just use MemoryMarshal.Write, but the codegen is suboptimal, see sharplab

[xtqqczze]

Why is this better?

It doesn't depend upon implementation specific behaviour, i.e. that the array data is properly aligned.

[hughbe]

We should really just use MemoryMarshal.Write, but the codegen is suboptimal, see sharplab

Is this something that can be improved? Might be worth opening an issue :)

[stephentoub]

Why is this better?

It doesn't depend upon implementation specific behaviour, i.e. that the array data is properly aligned.

In the future, please include your reasoning for the proposed changes in the PR description. Thanks.

[EgorBo]

We should really just use MemoryMarshal.Write, but the codegen is suboptimal, see sharplab

Looks like a simple addressing mode was not contained. It's just that this PR replaces one unsafe code with another unsafe (a tiny bit better, but still).
These APIs allocate, it is unlikely an extra lea makes perf difference here

[xtqqczze]

We should really just use MemoryMarshal.Write, but the codegen is suboptimal, see sharplab

Looks like a simple addressing mode was not contained. It's just that this PR replaces one unsafe code with another unsafe (a tiny bit better, but still). These APIs allocate, it is unlikely an extra lea makes perf difference here

@jakobbotsch do we have an issue tracking this?

[jakobbotsch]

@jakobbotsch do we have an issue tracking this?

I don't think we do. Feel free to open one.

[xtqqczze]

@MihuBot

[EgorBo]

Thanks! The diffs look sane considering these APIs allocate (and tracked by #104538). I have a general question to @stephentoub @jkotas

So in this case MemoryMarshal.Write is perfectly safe, but if we mark it as "[CallersRequiresUnsafeContext]", it's likely going to require unsafe for it. We have BinaryPrimitives.* APIs which are 100% memory safe, the problem that they're Endianess agnostic:

BinaryPrimitives.WriteInt32LittleEndian
BinaryPrimitives.WriteInt32BigEndian

Do we want to add new APIs there which check the endianess inside?

+BinaryPrimitives.WriteInt32

So then for any perfectly safe Read/Write for primitives we can use BinaryPrimitives.

[stephentoub]

if we mark it as "[CallersRequiresUnsafeContext]"

What's the rationale for why MemoryMarshal.Write(Span<T>, T) would be marked as unsafe? (This gets to our definition for what exactly we're covering with CallersRequiresUnsafeContext.)

[EgorBo]

What's the rationale for why MemoryMarshal.Write(Span, T) would be marked as unsafe?

I think mainly to protect from cases where structs with paddings/non-blittable fields (like bool) are used as T (probably can be protected by some theoretical where T : bitwise-writeable) resulting in storing garbage (or maybe even secrets?) into a byte stream. If whatever attribute we come up with can relax its behavior for primitives - that'd solve the issue as well

[stephentoub]

What's the rationale for why MemoryMarshal.Write(Span, T) would be marked as unsafe?

I think mainly to protect from cases where structs with paddings/non-blittable fields (like bool) are used as T (probably can be protected by some theoretical where T : bitwise-writeable) resulting in storing garbage (or maybe even secrets?) into a byte stream. If whatever attribute we come up with can relax its behavior for primitives - that'd solve the issue as well

Before we start talking about introducing new APIs, we should agree on the definition.

If we decide that such functionality needs to be considered "unsafe", and if the relevant APIs are considered important enough, and if there isn't an existing counterpart, we could certainly introduce additional methods. But there are several ifs there. I'm also not sure how such a BinaryPrimitives.WriteInt32 would differ from the existing BitConverter.TryWriteBytes, other than the try-ness.

[EgorBo]

Good point!

I'm also not sure how such a BinaryPrimitives.WriteInt32 would differ from the existing BitConverter.TryWriteBytes, other than the try-ness.

TBH, it's a bit confusing we have these primitives spread in BitConverter, BinaryPrimitives, BitOperations, MemoryMarshal 🙂

[jkotas]

resulting in storing garbage (or maybe even secrets?) into a byte stream.

If the bar for CallersRequiresUnsafeContext is memory safety and nothing else, I do not think this justification would meet the bar. This is not a memory safety violation. You can leak secrets into a byte stream in 100% safe code in many different ways.

BinaryPrimitives.WriteInt32 would differ from the existing BitConverter.TryWriteBytes

Can this change use TryWriteBytes and friends instead? It would make it more future proof.

[EgorBo]

Can this change use TryWriteBytes and friends instead? It would make it more future proof.

@xtqqczze could you please try that?

[EgorBo]

@MihuBot

[xtqqczze]

@MihuBot

[xtqqczze]

Can this change use TryWriteBytes and friends instead? It would make it more future proof.

@xtqqczze could you please try that?

Diffs are as expected with this approach

[EgorBo]

@xtqqczze Thanks!