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

Fix LoaderAllocator of DynamicMethodTable #111038

Merged

Conversation

janvorli
Copy link
Member

@janvorli janvorli commented Jan 2, 2025

There is one DynamicMethodTable allocated per Module if needed. When its instance is being created, an incorrect LoaderAllocator is passed to the CreateMinimalMethodTable function. Instead of the Module's LoaderAllocator, it passes in the default one.
That fires an assert in non-release builds of the runtime and it results in incorrect marking of the MethodTable as not collectible in case the Module's LoaderAllocator is collectible.

This change fixes it by passing in the LoaderAllocator of the module instead.

Close #110987

@janvorli janvorli requested a review from jkotas January 2, 2025 16:24
@janvorli janvorli self-assigned this Jan 2, 2025
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Do we need a regression test?

@janvorli
Copy link
Member Author

janvorli commented Jan 2, 2025

/azp run runtime-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

There is one `DynamicMethodTable` allocated per `Module` if needed. When its
instance is being created, an incorrect `LoaderAllocator` is passed to the
`CreateMinimalMethodTable` function. Instead of the `Module`'s
`LoaderAllocator`, it passes in the default one.
That fires an assert in non-release builds of the runtime and it results in
incorrect marking of the `MethodTable` as not collectible in case the `Module`'s
LoaderAllocator is collectible.

This change fixes it by passing in the `LoaderAllocator` of the module
instead.

Close dotnet#110987
@janvorli janvorli force-pushed the fix-dynamic-method-table-loader-allocator branch from b7406ce to 07fda5c Compare January 3, 2025 09:40
@janvorli janvorli merged commit 86a7ff9 into dotnet:main Jan 3, 2025
92 of 96 checks passed
@janvorli janvorli deleted the fix-dynamic-method-table-loader-allocator branch January 3, 2025 18:47
@BruceForstall
Copy link
Member

@janvorli Looks like the regression test is breaking test builds with

/__w/1/s/src/tests/Regressions/coreclr/GitHub_110987/test110987.cs(29,45): error CS1002: ; expected [/__w/1/s/src/tests/Regressions/coreclr/GitHub_110987/test110987.csproj] [/__w/1/s/src/tests/build.proj]

https://dev.azure.com/dnceng-public/public/_build/results?buildId=906353&view=logs&j=3725811e-4a8b-5f6a-b03b-a353ee894148&t=d59fccfb-2b13-5841-75bf-bfec705ee16a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assert failure in SetLoaderAllocator() when calling a DynamicMethod with collectible AssemblyLoadContext
3 participants