Use System.Numerics.IEqualityOperators.op_Equality in SpanHelper.T.cs where possible. #74567
Conversation
Workaround crash hit by dotnet#74179 making sure we avoid hitting codepath emitting this null pointer checks. The full fix includes codegen fixes as well, but will be performed in separate PR. There are still locations in SpanHelper.T.cs that uses Equal virtual call on value types that could be managed pointers to value types, but that code has remained the same for the last 4 years to 15 months and have not hit this issue in the past.
ghost
added
the
|
Tagging subscribers to this area: @dotnet/area-system-globalization Issue DetailsWorkaround crash hit by #74179 making sure we avoid hitting codepath emitting this null pointer checks. The full fix includes codegen fixes as well, but will be performed in separate PR. There are still locations in SpanHelper.T.cs that uses Equal virtual call on value types that could be managed pointers to value types, but that code has remained the same for the last 4 years to 15 months and have not hit this issue in the past. Fixes #74179.
|
There was a problem hiding this comment.
LGTM, thank you very much for investigating this complex issue @lateralusX !
|
Tagging subscribers to this area: @dotnet/area-system-memory Issue DetailsWorkaround crash hit by #74179 making sure we avoid hitting codepath emitting this null pointer checks. The full fix includes codegen fixes as well, but will be performed in separate PR. There are still locations in SpanHelper.T.cs that uses Equal virtual call on value types that could be managed pointers to value types, but that code has remained the same for the last 4 years to 15 months and have not hit this issue in the past. Fixes #74179.
|
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@lateralusX just to make sure I understand: Mono does emit correct code for these calls, right? except that we use a 32-bit read for the null check? So once we change the null check to stay in-bounds there are no other codegen issues? the workaround here is actually some kind of micro-optimization to avoid boxing that also happens to avoid the null-check? |
|
@lambdageek, since this specific code has a constraint on This will mitigate the issue introduced by changes to SpanHelpers.T.cs, but could of course happen in other areas, so the full codegen fix is needed to fully mitigate the issue, but due to potential risk, it was split into two PR's, meaning we could just backport this to 7.0 and would be back to the state we had before recent optimizations to SpanHelpers.T.cs. or we could backport both and then this fix will mainly just be eliminating the virtual call that gets devirtualized and inlined, so mainly just eliminate the addition OP_CHECK_THIS. So our virtual calls are fine, just that our lowering of OP_CHECK_THIS reads to much memory, that is fixed by #74638. |
|
Failures in runtime-extra-platforms unrelated. |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2946871738 |
Workaround crash hit by #74179 making sure we avoid hitting codepath emitting this null pointer checks. The full fix includes codegen fixes as well, but will be performed in separate PR. There are still locations in SpanHelper.T.cs that uses Equal virtual call on value types that could be managed pointers to value types, but that code has remained the same for the last 4 years to 15 months and have not hit this issue in the past.
Fixes #74179.
Full fix for underlying codegen issue, #74638.