JIT/Intrinsics/MemoryMarshalGetArrayDataReference from #72725 fails on Mono JIT

[MichalPetryka]

Description

A test introduced in #72725 shows that Mono JIT in some cases doesn't throw NRE on null array passed to MemoryMarshal.GetArrayDataReference.

Reproduction Steps

_ = MemoryMarshal.GetArrayDataReference<byte>(null);

Expected behavior

NullReferenceException.

Actual behavior

No exceptions.

Regression?

Works on CoreCLR.

Known Workarounds

Indirect calls to GetArrayDataReference via a function pointer do throw NRE.

Configuration

mono minijit Pri0 Runtime Tests Run OSX x64 release job.

Other information

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[lambdageek]

Well if it gets intrinsified it should emit a null check...

runtime/src/mono/mono/mini/intrinsics.c

Line 1011 in 1729ca6

MONO_EMIT_NULL_CHECK (cfg, args [0]->dreg, FALSE);

So maybe we're running the Unsafe.As non-intrinsic version?

Interp has a null check too

runtime/src/mono/mono/mini/interp/interp.c

Line 5507 in 20dd6ea

NULL_CHECK (o);

[MichalPetryka]

Both AOT and interp pass this test, only JIT fails.

And I'd rather assume that an indirect call would run the non-intrinsic version and those calls do correctly NRE.

[MichalPetryka]

For reference, this also fails:

[MethodImpl(MethodImplOptions.NoInlining)]
static T NoInline<T>(T t) => t;
_ = ref MemoryMarshal.GetArrayDataReference(NoInline<byte[]>(null));

[lambdageek]

maybe dead code elimination is eliding the null check

(I should be in a position to run some tests in a couple of hours. just taking some notes in the meantime)

[MichalPetryka]

I've added some more tests in #72745 that'll show if this is dead code elimination.

[lambdageek]

Ok, it's not dce. it's actually the backends

runtime/src/mono/mono/mini/mini-amd64.c

Lines 4914 to 4915 in aa5ff31

	case OP_NOT_NULL:
	break;

(arm64 is the same)

Main in this

        private static void Main(string[] args)
        {
            //bool isMono = typeof(object).Assembly.GetType("Mono.RuntimeStructs") != null;
            //Console.WriteLine($"Hello World {(isMono ? "from Mono!" : "from CoreCLR!")}");
            //Console.WriteLine(typeof(object).Assembly.FullName);
            //Console.WriteLine(System.Reflection.Assembly.GetEntryAssembly ());

	    ref byte b = ref MemoryMarshal.GetArrayDataReference<byte>(null);
	    
            //Console.WriteLine(System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription);
	    //Console.WriteLine (b);
	    Use (ref b);
        }

	[MethodImpl(MethodImplOptions.NoInlining)]
	private static void Use (ref byte b) {
	}

turns into

LOCAL REGALLOC BLOCK 2:
	1  il_seq_point intr il: 0x0
	2  i8const R18 <- [0]
	3  not_null R18
	4  long_add_imm R21 <- R18 [32] clobbers: 1
	5  il_seq_point il: 0x6, nonempty-stack
	6  il_seq_point il: 0x7
	7  voidcall [void HelloWorld.Program:Use (byte&)] [%rdi <- R21] clobbers: c
	8  il_seq_point il: 0xd, nonempty-stack
	9  il_seq_point il: 0xd
liveness: R18 [2 - 2]
liveness: R21 [4 - 4]
processing:	9  il_seq_point il: 0xd
	9  il_seq_point il: 0xd
processing:	8  il_seq_point il: 0xd, nonempty-stack
	8  il_seq_point il: 0xd, nonempty-stack
processing:	7  voidcall [void HelloWorld.Program:Use (byte&)] [%rdi <- R21] clobbers: c
	assigned arg reg %rdi to R21
	7  voidcall [void HelloWorld.Program:Use (byte&)] [%rdi <- R21] clobbers: c
processing:	6  il_seq_point il: 0x7
	6  il_seq_point il: 0x7
processing:	5  il_seq_point il: 0x6, nonempty-stack
	5  il_seq_point il: 0x6, nonempty-stack
processing:	4  long_add_imm R21 <- R18 [32] clobbers: 1
	assigned dreg %rdi to dest R21
	freeable %rdi (R21) (born in 4)
	assigned sreg1 %rdi to R18
	4  long_add_imm %rdi <- %rdi [32] clobbers: 1
processing:	3  not_null R18
	3  not_null %rdi
processing:	2  i8const R18 <- [0]
	assigned dreg %rdi to dest R18
	freeable %rdi (R18) (born in 2)
	2  i8const %rdi <- [0]
processing:	1  il_seq_point intr il: 0x0
	1  il_seq_point intr il: 0x0
CFA: [0] def_cfa: %rsp+0x8
CFA: [0] offset: unknown at cfa-0x8
CFA: [4] def_cfa_offset: 0x10
Basic block 0 starting at offset 0x4
Basic block 2 starting at offset 0x1c
Basic block 1 starting at offset 0x28
CFA: [2c] def_cfa: %rsp+0x8
Method void HelloWorld.Program:Main (string[]) emitted at 0x40180150 to 0x4018017d (code length 45)

*** ASM for HelloWorld.Program:Main (string[]) ***

/tmp/.Ai0lZC:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <loWorld_Program_Main__string___>:
loWorld_Program_Main__string___():
   0:	48 83 ec 08          	sub    $0x8,%rsp
   4:	48 b8 b8 9b fb 8f 2f 	movabs $0x7f2f8ffb9bb8,%rax
   b:	7f 00 00 
   e:	f7 00 01 00 00 00    	testl  $0x1,(%rax)
  14:	74 06                	je     1c <loWorld_Program_Main__string___+0x1c>
  16:	90                   	nop
  17:	e8 c2 65 ff ff       	call   ffffffffffff65de <loWorld_Program_Main__string___+0xffffffffffff65de>
<BB>:2
  1c:	33 ff                	xor    %edi,%edi
  1e:	48 83 c7 20          	add    $0x20,%rdi
  22:	90                   	nop
  23:	e8 05 00 00 00       	call   2d <loWorld_Program_Main__string___+0x2d>
xo<BB>:1
  28:	48 83 c4 08          	add    $0x8,%rsp
  2c:	c3                   	ret
***

So we make it all the way down to register allocation and then the backend mono_arch_output_basic_block drops the OP_NOT_NULL.

cc @vargaz - do we have some other opcode that emits a real load for a null check?

[vargaz]

Call MONO_EMIT_NEW_CHECK_THIS () instead of MONO_EMIT_NULL_CHECK () the latter only seems to work if exlicit null checks are enabled.

[lambdageek]

Call MONO_EMIT_NEW_CHECK_THIS () instead of MONO_EMIT_NULL_CHECK () the latter only seems to work if exlicit null checks are enabled.

Is that ok to call on something that isn't really this?

It's messing with CFG flags...

		cfg->flags |= MONO_CFG_HAS_CHECK_THIS; \

---

Ah interesting. the only side-effect is that we run array bounds check removal on the method

[MichalPetryka]

Could this affect other places if it's not exactly specific to this intrinsic then @lambdageek?

[alexrp]

I took a quick glance over the other intrinsics and it doesn't seem like others would be affected. The atomic intrinsics, for example, perform an actual load/store which would produce a SIGSEGV that gets turned into a NullReferenceException.

It was only a problem here because GetArrayDataReference doesn't actually dereference anything, yet must perform a null check.

[MichalPetryka]

The change seems to have made LLVM AOT fail when compiling the tests now, see #72725.