Conversation
|
Unfortunately the diff is confused as I've factored out some code into |
| builder.PositionAtEnd(throwBlock); | ||
| MetadataType nullRefType = _compilation.NodeFactory.TypeSystemContext.SystemModule.GetType("System", "NullReferenceException"); | ||
|
|
||
| CreateAndThrowException(builder, "NullReferenceException", NullRefFunction); |
There was a problem hiding this comment.
The other codegens use ThrowHelpers for this purpose. E.g. there is ThrowHelpers.ThrowNullReferenceException.
Would it be make sense to do the same for WASM?
There was a problem hiding this comment.
Thanks, I notice there is no ThrowArithmeticException there, but there is ThrowOverflowException. Looking at some of the code that was used for this , https://github.com/dotnet/coreclr/blob/9b0a9fd623/tests/src/JIT/IL_Conformance/Old/Base/ckfinite.il, it is catching an OverflowException but if I look at https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.opcodes.ckfinite?view=netcore-3.1 it says it should throw an ArithmeticException, which is the direction I went, but I'd need to add a helper method for that. Should I do that or use an OverflowException ?
There was a problem hiding this comment.
it says it should throw an ArithmeticException
OverflowException inherits from ArithmeticException so the docs are not wrong. It is ok to throw more specific exception that what the docs say. It may be nice to make the docs more accurate.
The behavior should match what CoreCLR does.
In any case, you can add these helpers as needed.
| { | ||
| if (llvmCheckFunction.Handle == IntPtr.Zero) | ||
| { | ||
| llvmCheckFunction = Module.AddFunction("corert.throwckfinite" + size, LLVMTypeRef.CreateFunction(LLVMTypeRef.Void, new LLVMTypeRef[] { LLVMTypeRef.CreatePointer(LLVMTypeRef.Int8, 0), size == 32 ? LLVMTypeRef.Float : LLVMTypeRef.Double }, false)); |
There was a problem hiding this comment.
Similar here - this can be a helper method in MathHelpers.
| builder.PositionAtEnd(throwBlock); | ||
|
|
||
| HandleDirectCall(ctorDef, ctorDef.Signature, new StackEntry[] { exceptionEntry }, null, default(LLVMValueRef), 0, NullRefFunction.GetParam(0), builder, false, default(LLVMValueRef), ctorDef); | ||
| ThrowException(builder, "Internal.Runtime.CompilerHelpers", "MathHelpers", "ThrowCkFiniteExc", llvmCheckFunction); |
There was a problem hiding this comment.
I would just use the existing ThrowHelpers.ThrowOverflowException
There was a problem hiding this comment.
I hope I'm not sending you round in circles on this. I think I've got it right...
2 participants
This follows #5405 now that we have exception support. Added all the test cases from the cases mentioned there, https://github.com/dotnet/coreclr/blob/9b0a9fd623/tests/src/JIT/IL_Conformance/Old/Base/ckfinite.il
Thanks @am11
WIP as depends on #8096, when I'll squash commits.
Closes #4549