[clr-interp] Fix access of boxed static fields address #115198
Conversation
Previous code was assuming that the field address obtained from the runtime points to the address where the field value resides. This is false for CORINFO_FLG_FIELD_STATIC_IN_HEAP fields, where at the field address we actually have the boxed instance reference, so we need to access the field value inside this box object instead.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the access of boxed static fields so that the field value is correctly obtained from the boxed instance rather than directly at the static field address.
- Introduces a flag to detect boxed static fields
- Adjusts stack pointer arithmetic to first load the boxed instance reference and then skips over the method table word
- Updates the code flow to correctly manage and push the new interpreted types
| @@ -1861,6 +1862,25 @@ void InterpCompiler::EmitStaticFieldAddress(CORINFO_FIELD_INFO *pFieldInfo, CORI | |||
| assert(0); | |||
| break; | |||
| } | |||
|
|
|||
| if (isBoxedStatic) | |||
There was a problem hiding this comment.
Consider refactoring the pointer arithmetic logic within the isBoxedStatic block. Extracting intermediate values using temporary variables or a helper function could improve clarity and reduce potential errors.
Copilot uses AI. Check for mistakes.
| // Skip method table word | ||
| m_pStackPointer--; | ||
| AddIns(INTOP_ADD_P_IMM); | ||
| m_pLastNewIns->data[0] = sizeof(void*); |
There was a problem hiding this comment.
Using sizeof(void*) as a magic value can be unclear; consider defining a named constant to document its purpose.
| m_pLastNewIns->data[0] = sizeof(void*); | |
| m_pLastNewIns->data[0] = POINTER_SIZE; |
Copilot uses AI. Check for mistakes.
|
@cshung You reported some issues with thread static fields a while ago. Don't remember exactly what the test case was, but found some incorrect behavior when revisiting the code. In case you remember how you reproduced the issue, let me know if this fixes it. |
@kg would know - I was helping her to debug something related to GC reporting - she gave me that repro. |
|
The existing code is unconditionally pushing byrefs, i.e. PushInterpType(InterpTypeByRef, NULL);I think it needs to push InterpTypeO for the case where the static field address is in the heap, right? |
|
@kg Why would it ? Field address is always a byref. It is not an object reference, rather an interior pointer for an object in your case. |
Previous code was assuming that the field address obtained from the runtime points to the address where the field value resides. This is false for CORINFO_FLG_FIELD_STATIC_IN_HEAP fields, where at the field address we actually have the boxed instance reference, so we need to access the field value inside this box object instead.