Avoid crashing in case of alias detection failure

[BrzVlad]

Simply use the conservative may alias value instead.

[BrzVlad]

Should fix crashes in implicit null checks pass dotnet/runtime#105319

cc @radekdoulik

[lambdageek]

@BrzVlad is this a bug upstream? I don't think the implicit null checks pass is heavily used. Can we isolate a test case where the operand doesn't have a value?

[akoeplinger]

We actually added the same check for MMO2 in f1f846f, maybe it is something specific to us?

[lambdageek]

maybe it is something specific to us

I'm not sure. the reason MMO1->getValue() might return null is because MachineMemOperand stores a PointerUnion<Value*, PseudoSourceValue*> and getValue will only extract the Value* if it's stored in there, if there's a PseudoSourceValue stored there, it'll return null.

llvm-project/llvm/include/llvm/CodeGen/MachineMemOperand.h

Lines 203 to 214 in 190606b

	/// Return the base address of the memory access. This may either be a normal
	/// LLVM IR Value, or one of the special values used in CodeGen.
	/// Special values are those obtained via
	/// PseudoSourceValue::getFixedStack(int), PseudoSourceValue::getStack, and
	/// other PseudoSourceValue member functions which return objects which stand
	/// for frame/stack pointer relative references and other special references
	/// which are not representable in the high-level IR.
	const Value *getValue() const { return PtrInfo.V.dyn_cast<const Value*>(); }
	const PseudoSourceValue *getPseudoValue() const {
	return PtrInfo.V.dyn_cast<const PseudoSourceValue*>();
	}

So maybe we're the only ones who do implicit null checks of stack pointer offsets, for example?

[lambdageek]

ImplicitNullChecks only fires on branches tagged with MD_make_implicit which emit_cond_system_exception in mini-llvm.c only does for NullReferenceException - MONO_EMIT_NEW_COND_EXC (cfg,cond,"NullReferenceException") is the macro that emits it. we use it pretty liberally without really checking if the argument is a heap or stack reference. so it's pretty likely we're doing something that the pass doesn't expect.

For example we emit it for the Interlocked.Increment (ref int arg) intrinsic:

https://github.com/dotnet/runtime/blob/1337553ef2f1cb7a78bb1c19a85b8a9c929064b2/src/mono/mono/mini/intrinsics.c#L1424-L1452

which will end up with a stack operand if someone is using atomic increment on a local for some misguided reason.

[BrzVlad]

@lambdageek I agree this is likely not a proper fix. I gave up quickly trying to undertand what exactly is leading to this crash since it didn't seem trivial and avoiding crashes is high priority. Further investigation would be preferable at some point.

[matouskozak]

How long does it take for this change to become effective inside dotnet/runtime? We are still seeing failures on the CI https://dev.azure.com/dnceng-public/public/_build/results?buildId=766538&view=logs&j=bb18f708-6684-5381-2c5c-c995f6debaa3&t=644a1944-383a-5910-653c-070240832801

[BrzVlad]

@matouskozak It has to be bumped manually dotnet/runtime#105867. I just triggered a new run there since it was still crashing before due to some simd intrinsic failures, which might have been caused by one of @tannergooding changes while llvm x64 ci was broken.

[matouskozak]

@matouskozak It has to be bumped manually dotnet/runtime#105867. I just triggered a new run there since it was still crashing before due to some simd intrinsic failures, which might have been caused by one of @tannergooding changes while llvm x64 ci was broken.

The failures were fixed in dotnet/runtime#105538 but we verified it only on arm64. Let's see how the new run goes.

[BrzVlad]

I already had that commit when testing locally. Also I fixed a crash by replacing new_args with args as it can be seen in that PR but there still seems to be some other issue. So I disabled emitting of that intrinsic entirely to see if it fixes the crashes.

[tannergooding]

Is there still a crash or issue that I need to look at here?

Was this missed or masked by CI due to the broader LLVM issue that existed?

[BrzVlad]

@tannergooding The crash was caused by dotnet/runtime@4fbd498cc25c. It is x64 specific. I reproduced it on MacOS with the llvm bump from dotnet/runtime#105867. I suspect it reproduces the same on linux.

The steps to repro should look something like this:
// Build runtime
./build.sh -subset mono+libs+clr.hosts -c Release /p:MonoEnableLLVM=true /p:MonoAOTEnableLLVM=true
// Build core root
src/tests/build.sh -mono Release generatelayoutonly
// Compile System.IO.Hashing.dll
cd artifacts/tests/coreclr/osx.x64.Release/Tests/Core_Root
export MONO_PATH=..../artifacts/tests/coreclr/osx.x64.Release/Tests/Core_Root
export PATH=$PATH:.../artifacts/bin/mono/osx.x64.Release // in order to find opt and llc
export MONO_ENV_OPTIONS="--aot=llvm"
..../artifacts/bin/mono/osx.x64.Release/cross/osx-x64/mono-aot-cross System.IO.Hashing.dll

[BrzVlad]

Also note that I tried fixing the crash (the second commit in the linked PR), which seemed to be caused by accidental unitialized passing of new_args. However it was still crashing so I disabled emitting that intrinsic altogether. Note that I'm not yet convinced that the bug comes from your commit. It is possible there is an issue somewhere else, maybe in our already existing implementation of PSHUFD for example.