Fix TypePreinit preinitialization correctness#128973
Conversation
Intern foreign preinit instances so nested static reference identity is preserved, conservatively mark serialized object instances mutable, and fix vtable-like slot clearing. Add NativeAOT smoke coverage for nested preinit identity and vtable slot clearing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
There was a problem hiding this comment.
Pull request overview
This PR updates the NativeAOT type preinitialization interpreter/serializer to preserve reference identity across nested preinit evaluation, fix a vtable-like slot initialization bug, and make frozen object mutability tracking more conservative. It also adds smoke-test coverage for the identity and slot-clearing scenarios.
Changes:
- Intern
ForeignTypeInstanceobjects by allocation site so repeated references to the same nested-preinit allocation preserve identity. - Fix vtable-like slot clearing to null out the correct range of slots.
- Make serialized
ObjectInstanceconservatively reportIsKnownImmutable => false, and add NativeAOT smoke tests for nested identity + slot clearing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.cs | Adds new smoke tests for nested preinit identity and vtable-slot clearing behavior. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypePreinit.cs | Fixes correctness in nested preinit object identity, vtable-like slot clearing, and mutability classification. |
| builder.EmitBytes(_data, pointerSize, _data.Length - pointerSize); | ||
| } | ||
|
|
||
| public bool IsKnownImmutable => !Type.GetFields().GetEnumerator().MoveNext(); |
There was a problem hiding this comment.
@EgorBo how is this value used? are we going to miss out on something important if we just return false? (gpt says coreclr is checking if there are any instance fields, including inherited. this is checking any fields, but only current type). we can fix it, but wondering how necessary it is.
There was a problem hiding this comment.
It is used e.g. to skip memory barriers https://godbolt.org/z/zsdK7Ge7b
this is checking any field
Also, this is checking static fields. It should check instance fields only.
There was a problem hiding this comment.
Yup, noticed that one too, that's why I only qualified "instance fields" on the CoreCLR side. It was both too much and too little.
Even a hello world had several instances where we'd say immutable even though there was instance state in the base. "This is effectively ARM-only" is the key thing I missed. Fixed this to be precise.
I asked GPT-5.5 to find bugs and it found bugs. One is more severe.
Intern foreign preinit instances so nested static reference identity is preserved, conservatively mark serialized object instances mutable, and fix vtable-like slot clearing.
Add NativeAOT smoke coverage for nested preinit identity and vtable slot clearing.