[NativeAOT] Add null checks into memcpy/memset helpers

[filipnavara]

Fixes #95517

[ghost]

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

Issue Details

---

Fixes #95517

Author:	filipnavara
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[jkotas]

What is this extra wrapper going to do to the perf? It may be better to make the JIT to inline the null checks instead.

[SingleAccretion]

What is this extra wrapper going to do to the perf? It may be better to make the JIT to inline the null checks instead.

We discussed implement this in the Jit. It is possible, but mildly involved due to the need to do the zero-length check. Additionally, these helpers are not used in scenarios with small sizes that would be most sensitive to the overhead (unless the user uses Unsafe.CopyBlock/InitBlock with small dynamic sizes).

This can call SpanHelpers.Fill / Buffer.Memmove instead of the C runtime APIs.

I will note that this would make the implementation fragile in the sense of being prone to self-recursion depending on Jit implementation details (or even optimization choices). Perhaps that's not a big concern; it would be an obvious failure mode.

[EgorBo]

We discussed implement this in the Jit. It is possible, but mildly involved due to the need to do the zero-length check.

Right, we have two options:

1. Expand block copies/inits early into a complex workflow (esp if we need if-else for nullchecks if we don't want to have implicit ones) and potentially ruin quite a few opts (although, the size/nullchecks could be then folded)
2. Emit these checks during codegen emit (similar to GT_STORE_BLK - do not call memset for blocks containg gc pointers on heap #95530). We won't be able to drop the null/size checks and will have to duplicate a complex emit logic for all 6 targets.

[EgorBo]

Additionally, these helpers are not used in scenarios with small sizes that would be most sensitive to the overhead

And only for structs without gc references. The unroll rules are listed here:

runtime/src/coreclr/jit/compiler.h

Lines 9215 to 9222 in d972a19

	// | arch | memset | memcpy |
	// |-------------|--------|--------|
	// | x86 avx512 | 512 | 256 |
	// | x86 avx | 256 | 128 |
	// | x86 sse | 128 | 64 |
	// | arm64 | 256 | 128 | ldp/stp (2x128bit)
	// | arm | 32 | 16 | no SIMD support
	// | loongarch64 | 64 | 32 | no SIMD support

[jkotas]

The unroll rules are listed here:

This kicks in for constant sizes only. Are there any important cases where these helpers are used for non-constant sizes?

[SingleAccretion]

Are there any important cases where these helpers are used for non-constant sizes?

cpblk/initblk are the only cases where they are used with a dynamic size.

[jkotas]

LGTM. Thanks!

[jkotas]

cc @VSadov We may want to do something similar for CoreCLR too. The current implementation of these helpers for CoreCLR is not GC suspend friendly.

[MichalPetryka]

The current implementation of these helpers for CoreCLR is not GC suspend friendly.

FYI Buffer.MemoryCopy throws ExecutionEngineException for null ptrs on both runtimes.
The null check should probably be moved into Buffer.Memmove instead.

[jkotas]

Buffer.MemoryCopy throws ExecutionEngineException for null ptrs on both runtimes. The null check should probably be moved into Buffer.Memmove instead.

Yes, it is by design. Passing invalid unmanaged buffers into BCL APIs is undefined behavior. Invalid unmanaged buffers typically signal process state corruption, buffer overrun, etc. We would not want to intentionally convert any situation where we fail fast for these cases today into something that can be caught and handled.

[MichalPetryka]

Buffer.MemoryCopy throws ExecutionEngineException for null ptrs on both runtimes. The null check should probably be moved into Buffer.Memmove instead.

Yes, it is by design. Passing invalid unmanaged buffers into BCL APIs is undefined behavior. Invalid unmanaged buffers typically signal process state corruption, buffer overrun, etc. We would not want to intentionally convert any situation where we fail fast for these cases today into something that can be caught and handled.

I'd expect null to not be considered "corrupted process state" kind of invalid in such API? This also only reproduces with a big size, a small one throws a NullReferenceException.

[VSadov]

We may want to do something similar for CoreCLR too. The current implementation of these helpers for CoreCLR is not GC suspend friendly.

Are these generally used for small sizes like boxing/unboxing or for potentially big buffers as well?

[jkotas]

I'd expect null to not be considered "corrupted process state" kind of invalid in such API? This also only reproduces with a big size, a small one throws a NullReferenceException.

Right, passing invalid unmanaged buffer into BCL APIs is undefined behavior. We are not trying to detect and convert all cases into fail fasts. If we were to make the behavior more defined, it would be to detect and fail fast in more cases. It tends to come with performance overhead and it is hard to tell when to stop. For example, how far we should go with detecting Span<T> with invalid unmanaged buffers?

This approach matches how many C/C++ libraries deal with this issue, including standard C library. For example, try calling fwrite with null pointer and non-zero buffer size - you are most likely going to see a fatal crash, the details will vary from platform to platform.

[VSadov]

Are these generally used for small sizes like boxing/unboxing or for potentially big buffers as well?

I think the following answers that:

Additionally, these helpers are not used in scenarios with small sizes that would be most sensitive to the overhead

[jkotas]

Are there any important cases where these helpers are used for non-constant sizes?

cpblk/initblk are the only cases where they are used with a dynamic size.

It turns out that we use initblk in two places in CoreLib: #98623 (comment) that are slower after this change than they need to be. I expect this to be cleaned up as this gets unified with full CoreCLR CoreLib.