Fix allocation of empty array in the frozen heap for collectible types

[xoofx]

Fixes #100437

An empty array should not be allocated on a frozen heap if its type is coming from a collectible assembly.

Might be difficult to bring a proper test (e.g is there an API to check if an object is instantiated in a frozen heap?). If it is required, guidance appreciated.

This fix should be backported to net8.0 as well.

[xoofx]

My colleague that has been investigating this is also suggesting adding an assert in FrozenObjectHeapManager::TryAllocateObject to check that a type cannot be collectible.

runtime/src/coreclr/vm/frozenobjectheap.cpp

Lines 48 to 49 in 34d13b2

	_ASSERT(type != nullptr);
	_ASSERT(FOH_COMMIT_SIZE >= MIN_OBJECT_SIZE);

Thoughts?

I can add it as part of this PR.

[jkotas]

adding an assert

Sounds good to me.

We should also add a test that hits it.

[jkotas]

We should also add a test that hits it.

Let me know if you need help with the test.

[xoofx]

Let me know if you need help with the test.

Yes, please, a starting place would be helpful! 😅

[cshung]

is there an API to check if an object is instantiated in a frozen heap

Yes, IGCHeap::IsInFrozenSegment should do.

runtime/src/coreclr/gc/gcinterface.h

Line 974 in d1cc577

virtual bool IsInFrozenSegment(Object *object) PURE_VIRTUAL

[xoofx]

Yes, IGCHeap::IsInFrozenSegment should do.

Thanks! I meant from C# as I'm not familiar how I will be able to make tests only from C++.

[jkotas]

is there an API to check if an object is instantiated in a frozen heap

We prefer to test observable behavior like that the program does not crash. You do not need an API that checks if an object is instantiated in a frozen heap for that.

[jkotas]

Yes, please, a starting place would be helpful!

This will crash with high probability due to this bug:

using System.Runtime.Loader;

public class Program
{
    static void Main()
    {
        WeakReference[] wrs = new WeakReference[10];
        for (int i = 0; i < wrs.Length; i++)
        {
            var alc = new MyAssemblyLoadContext();
            var a = alc.LoadFromAssemblyPath(typeof(Program).Assembly.Location);
            wrs[i] = (WeakReference)a.GetType("Program").GetMethod("Work").Invoke(null, null);
            GC.Collect();
        }

        foreach (var wr in wrs)
        {
            Console.WriteLine(wr.Target);
        }
    }

    public static WeakReference Work()
    {
        return new WeakReference(Array.Empty<Program>());
    }
}

class MyAssemblyLoadContext : AssemblyLoadContext
{
    public MyAssemblyLoadContext()
        : base(isCollectible: true)
    {
    }
}

[xoofx]

We prefer to test observable behavior like that the program does not crash. You do not need an API that checks if an object is instantiated in a frozen heap for that.
This will crash with high probability due to this bug:

Makes sense! Where should I add such a test? In src\tests\GC\Scenarios but I see also that there is a src\tests\GC\API\Frozen?

[jkotas]

As I was writing the test, I have realized that there is an opposite problem too: The frozen object allocated in shared generic static constructor may end up leaking:

class MyG<T>
{
     // This will be allocated on frozen heap, but it is going to leak if T is collectible
    static object s = new object();
}

I am not sure what's the best way to fix this leak. We can either detect and reject these problematic patterns as canidates for frozen heap allocation, or we need to pass the containing generic type to the MayBeFrozen allocation helper. cc @EgorBo

[jkotas]

Where should I add such a test?

I would add it under src\tests\JIT\Regression\JitBlue. This is a codegen related problem, so the regression test for it belongs under JIT tests.

[xoofx]

Added the test with commit 1189eec but I don't know how to run it. I tried .\build.cmd clr+libs+libs.tests -rc checked -lc release -test but doesn't look that it ran it.

[EgorBo]

As I was writing the test, I have realized that there is an opposite problem too: The frozen object allocated in shared generic static constructor may end up leaking:

class MyG<T>
{
     // This will be allocated on frozen heap, but it is going to leak if T is collectible
    static object s = new object();
}

I am not sure what's the best way to fix this leak. We can either detect and reject these problematic patterns as canidates for frozen heap allocation, or we need to pass the containing generic type to the MayBeFrozen allocation helper. cc @EgorBo

Good point! I propose this patch here:

-    if ((fi.fieldFlags & flagsToCheck) == flagsToCheck)
+    if (((fi.fieldFlags & flagsToCheck) == flagsToCheck) &&
+       ((info.compCompHnd->getClassAttribs(info.compClassHnd) & CORINFO_FLG_SHAREDINST) == 0))

the logic to detect candidates for NonGC allocators is quite trivial here, I have a prototype for a more advanced version, but it's not ready yet.

[EgorBo]

Good point! I propose this patch here:

Ah, it's a bit too conservative. Basically, with this patch we'll never optimize Array.Empty<string>() into a nongc handle. I presume a proper way is to pass type of current class as an argument in frozen allocator.. but that is not a trivial change so probably for now we should take the conservative patch?

[jkotas]

Ah, it's a bit too conservative. Basically, with this patch we'll never optimize Array.Empty() into a nongc handle. I presume a proper way is to pass type of current class as an argument in frozen allocator.. but that is not a trivial change so probably for now we should take the conservative patch?

Yes, the conservative fix can be backport candidate. The full fix would be hard to backport.

@xoofx Would you like to include it in this PR? (The test for this case can have similar structure - it should use weak handle to verify that the object is gone after the collectible assembly is unloaded.)

[jkotas]

I have pushed test update that hits all interesting cases.

[EgorBo]

I also usually struggle finding the right syntax for issues.props 😐

[jkotas]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8517805878

[github-actions]

@jkotas backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix allocation of empty array in the frozen heap for collectible types (#100437)
Using index info to reconstruct a base tree...
M	src/coreclr/vm/frozenobjectheap.cpp
M	src/coreclr/vm/gchelpers.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/vm/gchelpers.cpp
CONFLICT (content): Merge conflict in src/coreclr/vm/gchelpers.cpp
Auto-merging src/coreclr/vm/frozenobjectheap.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix allocation of empty array in the frozen heap for collectible types (#100437)
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[github-actions]

@jkotas an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

[jkotas]

I have opened #100510 on re-enabling the optimization in shared generic with proper lifetime checks.

#100509 is the backport to .NET 8.

@xoofx Thank you for your help with getting this bug found and fixed!

[xoofx]

@xoofx Thank you for your help with getting this bug found and fixed!

Most of the hard work investigating the crash from our side came from @alexey-zakharov ☺️

We are super glad that a quick fix was found, as we are heavily relying on collectible ALC and that bug haunted several crashes on our CI. Thanks a lot for helping with it!