Fix rebinding logic in MethodDef_Instance::InitializeFromIndex() #3221
Conversation
- Instead of blindly rebing methodDef it now checks if that is required and goes through it's paces on finding the correct method in the correct assembly.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactored cross-assembly method resolution in CLR's type system. Renamed variable Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant InitializeFromIndex as CLR_RT_MethodDef_Instance<br/>InitializeFromIndex()
participant OwnerAssembly as Owner Assembly
participant MethodDefTable as Original Assembly<br/>MethodDef Table
participant MethodRefTable as Target Assembly<br/>MethodRef Table
participant CrossRef as MethodRef<br/>Cross-Reference
Caller->>InitializeFromIndex: md, typeSpec
alt ownerAssembly != md.Assembly()
Note over InitializeFromIndex: Cross-assembly case
InitializeFromIndex->>OwnerAssembly: Locate original MethodDef
InitializeFromIndex->>MethodRefTable: Search by method name
alt MethodRef found
InitializeFromIndex->>CrossRef: Validate cross-ref target<br/>matches original
alt Cross-ref valid
InitializeFromIndex->>InitializeFromIndex: mdResolved = original method
else Cross-ref invalid
InitializeFromIndex->>InitializeFromIndex: mdResolved = md (fallback)
end
else MethodRef not found
InitializeFromIndex->>InitializeFromIndex: mdResolved = md (fallback)
end
else Same assembly
Note over InitializeFromIndex: Same-assembly case
InitializeFromIndex->>InitializeFromIndex: mdResolved = md
end
InitializeFromIndex->>InitializeFromIndex: InitializeFromIndex(mdResolved)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The change involves logic modifications to a core type system function with cross-assembly resolution semantics. Review requires understanding MethodRef lookup mechanics, assembly cross-references, and validating the fallback behavior for edge cases, but the change is localized to a single method. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/CLR/Core/TypeSystem.cpp (2)
1644-1644: Initialize mdResolved with md upfront.Avoid partial field assignment and make intent explicit.
- CLR_RT_MethodDef_Index mdResolved; + CLR_RT_MethodDef_Index mdResolved = md;
1664-1707: Cross-assembly scan is a no-op; remove or make it actually resolve.The loop only compares names and in all paths sets mdResolved.data = md.data, so behavior never changes while adding O(n) cost and confusion. Either delete the block and keep the original md, or implement signature-based resolution.
Minimal cleanup (recommended):
- // try to find the correct method reference - if (ownerTypeIdx.Assembly() != md.Assembly()) - { - // Cross-assembly case: need to find the corresponding MethodRef/MethodDef - CLR_RT_Assembly *originalAssm = g_CLR_RT_TypeSystem.m_assemblies[md.Assembly() - 1]; - CLR_RT_Assembly *targetAssm = g_CLR_RT_TypeSystem.m_assemblies[ownerTypeIdx.Assembly() - 1]; - - // Get the original method information - const CLR_RECORD_METHODDEF *originalMD = originalAssm->GetMethodDef(md.Method()); - const char *methodName = originalAssm->GetString(originalMD->name); - - // Try to find the method in the target assembly by looking through MethodRefs - bool found = false; - for (int i = 0; i < targetAssm->tablesSize[TBL_MethodRef]; i++) - { - const CLR_RECORD_METHODREF *mr = targetAssm->GetMethodRef(i); - const char *refName = targetAssm->GetString(mr->name); - - if (!strcmp(methodName, refName)) - { - // Found a potential match, now check if it resolves to our original method - CLR_RT_MethodRef_CrossReference &crossRef = targetAssm->crossReferenceMethodRef[i]; - - if (crossRef.target.data == md.data) - { - // This MethodRef points to our original method! - mdResolved.data = md.data; - found = true; - break; - } - } - } - - if (!found) - { - // Fallback: keep the original method reference - mdResolved.data = md.data; - } - } - else - { - // Same assembly case: no rebinding needed - mdResolved.data = md.data; - } + // No rebinding needed; keep original method index (rebinding caused issues previously).If you intended a real resolution, prefer matching by signature (name+signature) as done in ResolveMethodRef/FindMethodDef rather than name-only, and only rebind when a definitive match is found.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CLR/Core/TypeSystem.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_P4_UART)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_ETHERNET_KIT_1.2)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_S3_ALL)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
There was a problem hiding this comment.
Pull Request Overview
This pull request replaces a simple cross-assembly method rebinding mechanism with a more sophisticated method resolution algorithm. The change addresses an issue where methods were being incorrectly rebound across assemblies by simply replacing the assembly ID while keeping the same method index.
- Renamed
mdReboundtomdResolvedfor clarity - Implemented cross-assembly method resolution that searches for corresponding MethodRefs
- Added a fallback to preserve the original method reference when cross-assembly resolution fails
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!strcmp(methodName, refName)) | ||
| { | ||
| // Found a potential match, now check if it resolves to our original method | ||
| CLR_RT_MethodRef_CrossReference &crossRef = targetAssm->crossReferenceMethodRef[i]; | ||
|
|
||
| if (crossRef.target.data == md.data) | ||
| { | ||
| // This MethodRef points to our original method! | ||
| mdResolved.data = md.data; | ||
| found = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The method resolution logic is incorrect. When a matching MethodRef is found (line 1687), the code sets mdResolved.data to md.data (the original method), but it should construct a new method index using the target assembly and the MethodRef index. The resolved method should be: mdResolved.data = (ownerTypeIdx.Assembly() << 24) | i; This defeats the purpose of the cross-assembly resolution.
| // Found a potential match, now check if it resolves to our original method | ||
| CLR_RT_MethodRef_CrossReference &crossRef = targetAssm->crossReferenceMethodRef[i]; | ||
|
|
||
| if (crossRef.target.data == md.data) | ||
| { | ||
| // This MethodRef points to our original method! | ||
| mdResolved.data = md.data; | ||
| found = true; | ||
| break; |
There was a problem hiding this comment.
Method matching only compares names without checking signatures. This can lead to incorrect matches when multiple overloaded methods exist with the same name but different signatures. The code should use signature comparison similar to FindMethodDef (lines 5703-5710) which uses CLR_RT_SignatureParser and CLR_RT_TypeSystem::MatchSignature.
| // Found a potential match, now check if it resolves to our original method | |
| CLR_RT_MethodRef_CrossReference &crossRef = targetAssm->crossReferenceMethodRef[i]; | |
| if (crossRef.target.data == md.data) | |
| { | |
| // This MethodRef points to our original method! | |
| mdResolved.data = md.data; | |
| found = true; | |
| break; | |
| // Compare signatures to ensure correct method match | |
| CLR_RT_SignatureParser sigParserOriginal; | |
| CLR_RT_SignatureParser sigParserCandidate; | |
| // Parse the signature of the original method | |
| sigParserOriginal.Initialize_Method(originalAssm, originalMD); | |
| // Parse the signature of the candidate method reference | |
| sigParserCandidate.Initialize_MethodRef(targetAssm, mr); | |
| if (CLR_RT_TypeSystem::MatchSignature(sigParserOriginal, sigParserCandidate)) | |
| { | |
| // Found a potential match, now check if it resolves to our original method | |
| CLR_RT_MethodRef_CrossReference &crossRef = targetAssm->crossReferenceMethodRef[i]; | |
| if (crossRef.target.data == md.data) | |
| { | |
| // This MethodRef points to our original method! | |
| mdResolved.data = md.data; | |
| found = true; | |
| break; | |
| } |
There was a problem hiding this comment.
@copilot Method signatures will be different depending on a type being a closed or open generic... signatures can't be compared on an exact match.
2 participants
Description
Motivation and Context
How Has This Been Tested?
[build with MDP buildId 57195]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit