Skip to content

Fix TypePreinit preinitialization correctness #128973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MichalStrehovsky wants to merge 2 commits into
base: main
Choose a base branch
from
+104 −9
Open

Fix TypePreinit preinitialization correctness #128973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b7f46d
Fix TypePreinit preinitialization correctness
MichalStrehovsky Jun 4, 2026
eababcd
Precise check
MichalStrehovsky Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 52 additions & 9 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypePreinit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public class TypePreinit
private readonly Dictionary<FieldDesc, Value> _fieldValues = new Dictionary<FieldDesc, Value>();
private readonly Dictionary<string, StringInstance> _internedStrings = new Dictionary<string, StringInstance>();
private readonly Dictionary<TypeDesc, RuntimeTypeValue> _internedTypes = new Dictionary<TypeDesc, RuntimeTypeValue>();
private readonly Dictionary<AllocationSite, ForeignTypeInstance> _foreignInstances = new Dictionary<AllocationSite, ForeignTypeInstance>();
private readonly Dictionary<MetadataType, NestedPreinitResult> _nestedPreinitResults = new Dictionary<MetadataType, NestedPreinitResult>();
private readonly Dictionary<EcmaField, byte[]> _rvaFieldDatas = new Dictionary<EcmaField, byte[]>();

Expand Down Expand Up @@ -153,6 +154,16 @@ private byte[] GetFieldRvaData(EcmaField field)
return result;
}

private ForeignTypeInstance GetOrCreateForeignInstance(TypeDesc type, AllocationSite allocationSite, ReferenceTypeValue data)
{
if (!_foreignInstances.TryGetValue(allocationSite, out ForeignTypeInstance result))
{
_foreignInstances.Add(allocationSite, result = new ForeignTypeInstance(type, allocationSite, data));
}

return result;
}

private Status TryScanMethod(MethodDesc method, Value[] parameters, Stack<MethodDesc> recursionProtect, ref int instructionCounter, out Value returnValue)
{
MethodIL methodIL = _ilProvider.GetMethodIL(method);
Expand Down Expand Up @@ -2791,7 +2802,7 @@ public override bool TryInitialize(int size)
if (_index + numSlots > _parent._methods.Length)
return false;

for (int i = _index; i < numSlots; i++)
for (int i = _index; i < _index + numSlots; i++)
_parent._methods[i] = null;

return true;
Expand Down Expand Up @@ -3176,7 +3187,7 @@ public override bool TryCompareEquality(Value value, out bool result)
public abstract ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext);
}

private struct AllocationSite
private readonly struct AllocationSite : IEquatable<AllocationSite>
{
public MetadataType OwningType { get; }
public int InstructionCounter { get; }
Expand All @@ -3186,6 +3197,14 @@ public AllocationSite(MetadataType type, int instructionCounter)
OwningType = type;
InstructionCounter = instructionCounter;
}

public bool Equals(AllocationSite other) =>
OwningType == other.OwningType
&& InstructionCounter == other.InstructionCounter;

public override bool Equals(object obj) => obj is AllocationSite other && Equals(other);

public override int GetHashCode() => HashCode.Combine(OwningType, InstructionCounter);
}

/// <summary>
Expand All @@ -3201,11 +3220,13 @@ public AllocatedReferenceTypeValue(TypeDesc type, AllocationSite allocationSite)
AllocationSite = allocationSite;
}

public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext) =>
new ForeignTypeInstance(
Type,
new AllocationSite(AllocationSite.OwningType, AllocationSite.InstructionCounter - baseInstructionCounter),
this);
public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext)
{
AllocationSite foreignAllocationSite = new AllocationSite(
AllocationSite.OwningType,
AllocationSite.InstructionCounter - baseInstructionCounter);
return preinitContext.GetOrCreateForeignInstance(Type, foreignAllocationSite, this);
}

public override bool GetRawData(NodeFactory factory, out object data)
{
Expand Down Expand Up @@ -3432,7 +3453,10 @@ public override void WriteFieldData(ref ObjectDataBuilder builder, NodeFactory f
}
}

public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext) => this;
public override ReferenceTypeValue ToForeignInstance(int baseInstructionCounter, TypePreinit preinitContext)
{
return preinitContext.GetOrCreateForeignInstance(Type, AllocationSite, Data);
}
}

private sealed class StringInstance : ReferenceTypeValue, IHasInstanceFields
Expand Down Expand Up @@ -3572,7 +3596,26 @@ public void WriteContent(ref ObjectDataBuilder builder, ISymbolNode thisNode, No
builder.EmitBytes(_data, pointerSize, _data.Length - pointerSize);
}

public bool IsKnownImmutable => !Type.GetFields().GetEnumerator().MoveNext();
Copy link
Copy Markdown
Member Author

@MichalStrehovsky MichalStrehovsky Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member

@jkotas jkotas Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@MichalStrehovsky MichalStrehovsky Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public bool IsKnownImmutable
{
get
{
// Are there any instance fields?
if (Type.IsValueType)
{
// For value types, look at the actual fields.
foreach (FieldDesc f in Type.GetFields())
if (!f.IsStatic)
return false;

return true;
}

// For reference types, check if the unaligned size == MethodTable pointer
// since we always have at least that field in the hierarchy.
return ((DefType)Type).InstanceByteCountUnaligned == Type.Context.Target.LayoutPointerSize;
}
}

public int ArrayLength => throw new NotSupportedException();
}
Expand Down
52 changes: 52 additions & 0 deletions src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ private static int Main()
TestDelegateReflectionVisible.Run();
TestInitFromOtherClass.Run();
TestInitFromOtherClassDouble.Run();
TestNestedPreinitIdentity.Run();
TestDelegateToOtherClass.Run();
TestLotsOfBackwardsBranches.Run();
TestSwitch.Run();
Expand Down Expand Up @@ -727,6 +728,38 @@ public static void Run()
}
}

class TestNestedPreinitIdentity
{
class OtherClass
{
public static readonly object ObjectValue = new object();
}

class YetAnotherClass
{
public static readonly object ObjectValue = OtherClass.ObjectValue;
}

static bool s_areSame;
static bool s_areSameIndirect;

static TestNestedPreinitIdentity()
{
object first = OtherClass.ObjectValue;
object second = OtherClass.ObjectValue;
object third = YetAnotherClass.ObjectValue;

s_areSame = first == second;
s_areSameIndirect = first == third;
}

public static void Run()
{
Assert.IsPreinitialized(typeof(TestNestedPreinitIdentity));
Assert.True(s_areSame);
Assert.True(s_areSameIndirect);
}
}

class TestDelegateToOtherClass
{
Expand Down Expand Up @@ -1904,6 +1937,20 @@ static TinyVtableCImpl()
}
}

public unsafe class TinyVtableClearSlotImpl
{
[FixedAddressValueType]
public static readonly ITinyVtableB Vtbl;

static TinyVtableClearSlotImpl()
{
Vtbl.First = &First;
Vtbl.Second = &Second;
Vtbl.Third = &Third;
Vtbl.Second = default;
}
}

public unsafe struct ITinyVtableA
{
public delegate*<void> First;
Expand Down Expand Up @@ -1946,6 +1993,11 @@ public static unsafe void Run()
Assert.AreEqual((nuint)(delegate*<void>)&Second, (nuint)TinyVtableCImpl.Vtbl.Second);
Assert.AreEqual((nuint)(delegate*<void>)&Third, (nuint)TinyVtableCImpl.Vtbl.Third);
Assert.AreEqual((nuint)(delegate*<void>)&Fourth, (nuint)TinyVtableCImpl.Vtbl.Fourth);

Assert.IsPreinitialized(typeof(TinyVtableClearSlotImpl));
Assert.AreEqual((void*)(delegate*<void>)&First, TinyVtableClearSlotImpl.Vtbl.First);
Assert.AreEqual((void*)null, TinyVtableClearSlotImpl.Vtbl.Second);
Assert.AreEqual((void*)(delegate*<void>)&Third, TinyVtableClearSlotImpl.Vtbl.Third);
Comment thread
MichalStrehovsky marked this conversation as resolved.
}
}

Expand Down
Loading