Fix bad codegen for fixed buffer access#26351
Conversation
When generating a pinning temp for fixed statements on fixed buffers, the type for the pinning temp is ref to the type of the fixed field. However, in our lowering the type of a fixed buffer field is a pointer type, while what we actually want for the pinning temp is a ref to the underlying type. This change fixes that scenario. Fixes dotnet#26335
|
cc @jaredpar |
|
cc @dotnet/roslyn-compiler for review |
|
Approved but do get the PR reviewed. |
|
Hold off on merge until we get division approval. |
| fieldAccess.FieldSymbol, | ||
| fieldAccess.ConstantValueOpt, | ||
| fieldAccess.ResultKind, | ||
| initializerType); |
There was a problem hiding this comment.
initializerType [](start = 20, length = 15)
This doesn't look right to me. The node is no longer consistent. #Closed
| initializerExpr = ((BoundAddressOfOperator)initializerExpr).Operand; | ||
|
|
||
| TypeSymbol initializerType = initializerExpr.Type; |
There was a problem hiding this comment.
TypeSymbol initializerType = initializerExpr.Type; [](start = 12, length = 50)
It looks like before the break, we were getting the type from the BoundAddressOfOperator, before digging through it. Can we restore the same logic instead of trying to patch the type later? #Closed
There was a problem hiding this comment.
The type would still be wrong. The problem is that we need to actually change the type from pointer to reference, but the language sees a fixed buffer as being of pointer type. We could change the type earlier in codegen, but I think that would be a riskier change.
There was a problem hiding this comment.
I agree that would be riskier. If we feel it is the better change then my suggestion:
- Keep the fix in the more constrained form here.
- File a bug to fix the type up earlier in the compiler in the 15.8 release.
There was a problem hiding this comment.
but I think that would be a riskier change.
Before we can make this call, we should actually understand what would be the change.
In reply to: 183831907 [](ancestors = 183831907)
AlekseyTs
left a comment
There was a problem hiding this comment.
I think we shouldn't mangle BoundFieldAccess node for a fixed field in the local rewriter. If necessary, we should adjust emit layer to properly emit address of a fixed field when it is a source of a ref assignment.
| Lo = lo; | ||
| Hi = hi; | ||
| unsafe { | ||
| fixed (int *p = Delta) { |
There was a problem hiding this comment.
fixed (int *p = Delta) { [](start = 16, length = 24)
Consider adding a test for a static fixed field and for access to an instance fixed field outside of the declaring type. #Closed
There was a problem hiding this comment.
Static fixed fields are tested in FixedSizeBufferTests.StaticFixedBuffer and instance fixed fields outside the declaring type is tested in FixedSizeBufferTests.FixedStatementOnFixedBuffer.
| { | ||
| public class FixedSizeBufferTests : EmitMetadataTestBase | ||
| { | ||
| [Fact] |
There was a problem hiding this comment.
[Fact] [](start = 8, length = 6)
WorkItem? #Closed
|
Done with review pass (iteration 2) #Closed |
…serts to allow pointer to managed ref assignment for fixed-size buffers
AlekseyTs
left a comment
There was a problem hiding this comment.
LGTM (iteration 4), assuming all tests will pass.
Customer scenario
Very common usages of fixed buffers will cause failures in Mono and potentially other runtimes due to bad IL. The IL is rejected due to type system violations, but is semantically correct. This causes the CLR to accept the IL and run it as expected. Mono is stricter, however, and other runtimes may also reject the program.
Bugs this fixes
#26335
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/605434
Workarounds, if any
For Mono, there are none.
Risk
This is a change localized to fixed buffer field access in lowering.
Performance impact
None, this is a minor change in logic.
Is this a regression from a previous update?
Yes.
Root cause analysis
Unfortunately, the desktop CLR and CoreCLR both accept this IL and run it as expected. Type system violations like this are usually caught with PEVerify, but since this is unsafe code by definition, PEVerify cannot be used to find this violation.
How was the bug found?
Mono reported this bug when they upgraded their baseline compiler.