Skip to content
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.

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

Empty array allocated on the Frozen Heap for a Collectible type? #100437

Closed
xoofx opened this issue Mar 29, 2024 · 5 comments · Fixed by #100444
Closed

Empty array allocated on the Frozen Heap for a Collectible type? #100437

xoofx opened this issue Mar 29, 2024 · 5 comments · Fixed by #100444
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@xoofx
Copy link
Member

xoofx commented Mar 29, 2024

Hello folks,

Apologies, it's not a fully reproducible bug as we haven't tried yet to reproduce it with a simpler sample, but we have a suspicion.

We are investigating an error where we have the GC marking of frozen objects gc_heap::mark_ro_segments()that is crashing.

One place that we are curious about is in TryAllocateFrozenSzArray:

if (pArrayMT->ContainsPointers() && cElements > 0)
{
// For arrays with GC pointers we can only work with empty arrays
return NULL;
}

Where it seems that we could allow to allocate an empty array for collectible MethodTable in the Frozen heap. Shouldn't it be something like this instead?

    if (pArrayMT->Collectible() || (pArrayMT->ContainsPointers() && cElements > 0))
    {
        // For arrays with GC pointers we can only work with empty arrays
        return NULL;
    }

Before we investigate this further, any thoughts?

cc: @EgorBo

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 29, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 29, 2024
@jkotas
Copy link
Member

jkotas commented Mar 29, 2024

Before we investigate this further, any thoughts?

Yes, this looks like a bug. Could you please submit a PR with the fix?

@xoofx
Copy link
Member Author

xoofx commented Mar 29, 2024

Yes, this looks like a bug. Could you please submit a PR with the fix?

Sure!

@jkotas
Copy link
Member

jkotas commented Mar 29, 2024

There is CORJIT_FLAG_FROZEN_ALLOC_ALLOWED that handles most cases, but it does not cover shared generics. The shared generic code may have CORJIT_FLAG_FROZEN_ALLOC_ALLOWED set, but the actual instantiation can be collectible. I think you should be able to reproduce the crash using a pattern like this.

@xoofx
Copy link
Member Author

xoofx commented Mar 29, 2024

Created PR #100444. We have tested a similar fix in our own fork, and it seems to be fixing our issues.

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Mar 29, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Mar 29, 2024
xoofx added a commit to xoofx/dotnet-runtime that referenced this issue Mar 30, 2024
dotnet#100437)

Add assert for collectible in FrozenObjectHeapManager::TryAllocateObject

Fix another potential case

Add test

Fix test

Fix test bis

Fix test bis bis
jkotas added a commit that referenced this issue Apr 2, 2024
#100444)

* Fix allocation of empty array in the frozen heap for collectible types (#100437)

* Remove Optimize from csproj

* Add test for generic with static

* Apply suggestions from code review

* Better test

* Disable tests on Mono

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
jkotas added a commit that referenced this issue Apr 2, 2024
#100444)

* Fix allocation of empty array in the frozen heap for collectible types (#100437)

* Remove Optimize from csproj

* Add test for generic with static

* Apply suggestions from code review

* Better test

* Disable tests on Mono

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
alexey-zakharov pushed a commit to Unity-Technologies/runtime that referenced this issue Apr 2, 2024
dotnet#100444)

* Fix allocation of empty array in the frozen heap for collectible types (dotnet#100437)

* Remove Optimize from csproj

* Add test for generic with static

* Apply suggestions from code review

* Better test

* Disable tests on Mono

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
(cherry picked from commit 78f7707)
jkotas added a commit that referenced this issue Apr 4, 2024
#100444) (#100509)

* Fix allocation of empty array in the frozen heap for collectible types (#100437)

* Remove Optimize from csproj

* Add test for generic with static

* Apply suggestions from code review

* Better test

* Disable tests on Mono

---------

Co-authored-by: Alexandre Mutel <alexandre_mutel@live.com>
matouskozak pushed a commit to matouskozak/runtime that referenced this issue Apr 30, 2024
dotnet#100444)

* Fix allocation of empty array in the frozen heap for collectible types (dotnet#100437)

* Remove Optimize from csproj

* Add test for generic with static

* Apply suggestions from code review

* Better test

* Disable tests on Mono

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants