Fix closed generic instance corruption and generic type-param cast#3452
Fix closed generic instance corruption and generic type-param cast#3452danielmeza wants to merge 2 commits into
Conversation
…GENERICINST handling A closed generic class instance stores its closed TypeSpec in the object-header slot that overlaps the monitor-lock pointer. Entering a Synchronized instance method (which C# field-like event add/remove accessors are) locked `this` and wrote the lock pointer into that slot, destroying the TypeSpec; subsequent generic field/method resolution then dereferenced a wild pointer and crashed (LoadProhibited). FindLockObject had the inverse defect, reading the TypeSpec back as a lock pointer. Guards the object-lock cache against generic instances (CLR_RT_HeapBlock_Lock::CreateInstance/ChangeOwner and FindLockObject); monitor ownership still works through the per-thread lock lists. Closes the related GC and type-system gaps for the dedicated DATATYPE_GENERICINST heap representation (boxed Nullable<T>): GC reachability marking and heap-compaction relocation walk its fields, type recovery (InitializeFromObject/ExtractTypeIndexFromObject) and initobj/clone handle it, and NewGenericInstanceObject records the closed TypeSpec and the generic-instance flag. Also completes the remaining DATATYPE_GENERICINST handling: GetHashCode, ObjectsEqual and Compare_Values treat it like a value type (inline fields); the compactor ValidateCluster overlapping-block check excludes it (its header slot is reused for object data, like class/value-type); and the unconditional NewGenericInstanceObject debug traces are removed.
A (T)item cast (castclass !0) inside an explicit interface implementation on a closed generic instance threw CLR_E_INVALID_CAST: the dispatch lost the closed TypeSpec, so the VAR branch of InitializeFromSignatureToken had no generic context and failed the cast. The CALLVIRT generic-context recovery pre-pass was guarded to TBL_MethodRef (cross-assembly interface calls). When the interface and the caller live in the SAME assembly (e.g. Repeater calling IDataTemplate.Build, both in nanoFramework.Lvgl.Ui) the call is a TBL_MethodDef, so the pre-pass never ran and genericType stayed null. Two changes in CEE_CALL/CEE_CALLVIRT (Interpreter.cpp): - Pre-pass: prefer the receiver's own stored closed TypeSpec (obj->ObjectGenericType()) over the caller-assembly TypeSpec-table scan (more robust, cross-assembly correct). - else branch (genericType still null after InitializeFromIndex): recover the closed TypeSpec from the receiver (IsAGenericInstance + ObjectGenericType, matched to the callee's declaring type) and reinitialize, mirroring the recovery already done in the genericType branch. This covers the same-assembly MethodDef interface dispatch the pre-pass misses. On device (ESP32-S3): a generic DataTemplate<T> provider shim (IDataTemplate.Build(object) -> (T)item) driving a Repeater over ObservableCollection<T> now builds, rebuilds on collection change, and applies per-item compiled bindings with no InvalidCastException.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
Apart from this, please review the comments added. They are VERY verbose. I understand AI is eager to add them, but it doesn't make sense otherwise we'll end up with more comments than code making it unreadable. Keep only the absolutely necessary ones and make them as concise as possible.
|
|
||
| fieldCursor = reinterpret_cast<CLR_RT_HeapBlock *>(giHeader) + totFields; | ||
|
|
||
| CLR_Debug::Printf( |
There was a problem hiding this comment.
These debug snippets are to be kept.
|
@danielmeza please instruct your agent to respect the PR template. Do not remove the checklist items not checked. 😉 |
|
@danielmeza we need a unit test (or more) to cover this. Please add it to mscorlib solution (develop branch). Maybe in the Types project... |
2 participants
Description
Synchronizedinstance method no longer overwrites the closedTypeSpecstored in the object header.DATATYPE_GENERICINSThandling for the dedicated heap representation (boxedNullable<T>): garbage-collector reachability marking and heap-compaction relocation now walk its fields, and type recovery (InitializeFromObject/ExtractTypeIndexFromObject),initobj/clone,GetHashCode,ObjectsEqualandCompare_Valuesall treat it as an inline value type.TypeSpecduringCEE_CALL/CEE_CALLVIRTso a generic type-parameter cast (castclass !0) resolves correctly when the interface call is dispatched within the same assembly.NewGenericInstanceObjectdebug traces.Motivation and Context
TypeSpecin the object-header slot that overlaps the monitor-lock pointer. Entering aSynchronizedinstance method — which is what a C# field-like eventadd/removeaccessor compiles to — lockedthisand wrote the lock pointer into that slot, destroying theTypeSpec. Any subsequent generic field or method resolution then dereferenced a wild pointer and faulted (LoadProhibited).FindLockObjecthad the inverse defect, reading theTypeSpecback as a lock pointer. Monitor ownership still works through the per-thread lock lists, so guarding the cache against generic instances is sufficient.(T)itemcast (castclass !0) inside an explicit interface implementation on a closed generic instance threwCLR_E_INVALID_CAST: the dispatch lost the closedTypeSpec, so theVARbranch ofInitializeFromSignatureTokenhad no generic context. TheCALLVIRTgeneric-context recovery pre-pass was guarded toTBL_MethodRef(cross-assembly interface calls); when the interface and the caller live in the same assembly the call is aTBL_MethodDef, so the pre-pass never ran and the generic type stayed null. The fix prefers the receiver's own stored closedTypeSpecover the caller-assemblyTypeSpec-table scan, and adds an else-branch recovery from the receiver for the same-assemblyMethodDefinterface dispatch the pre-pass misses.How Has This Been Tested?
Observable<int>.Changed += handler; raise) now subscribes and fires without faulting, where it previously crashed withLoadProhibited.DataTemplate<T>provider whoseIDataTemplate.Build(object)performs a(T)itemcast, driving a repeater over anObservableCollection<T>(both in the same assembly), now builds, rebuilds on collection change, and applies per-item bindings with noInvalidCastException.Types of changes
Checklist: