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

[release/9.0-staging] Fix reporting GC fields from base types #111040

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 2, 2025

Backport of #111011 to release/9.0-staging

/cc @MichalStrehovsky

Customer Impact

  • Customer reported
  • Found internally

We've been seeing intermittent crashes when running native AOT-compiled ILC in the CI (~5 crashes out of 2000 invocations). This was root caused to a GC hole. The GC hole could be anywhere in user code as well, whenever stack object allocation kicks in. Would affect both native AOT and ReadyToRun (the GC-hole causing miscompilation is in both).

Regression

  • Yes
  • No

Introduced in #104411 - we missed updating a spot that was only handling valuetypes but started to be fed reference types as of that PR.

Testing

Verified the fix on another 2000 invocations of ILC with the fix. No more crashes observed. Since the crash is intermittent, we only see it sometimes.

Risk

The risk should be low. The fix was obvious once the bug was root caused (we missed reporting GC fields from base types so the fix is to recurse into bases).

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

Package authoring no longer needed in .NET 9

IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.

Sorry, something went wrong.

Fixes #110836.

When we extended managed CorInfoImpl to support object stack allocation in #104411, there was one more spot that assumed valuetypes only in `GatherClassGCLayout` that we missed. This resulted in not reporting any GC pointers in base types.
@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jan 6, 2025
@jeffschwMSFT jeffschwMSFT added this to the 9.0.x milestone Jan 6, 2025
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. we will take for consideration in 9.0.x

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 7, 2025
@jeffschwMSFT jeffschwMSFT modified the milestones: 9.0.x, 9.0.2 Jan 7, 2025
@jeffschwMSFT
Copy link
Member

@MichalStrehovsky please review the PR failures and merge when ready

@jkotas
Copy link
Member

jkotas commented Jan 8, 2025

/ba-g Known infrastructure issues with OSX machines

@jkotas jkotas merged commit 726bb80 into release/9.0-staging Jan 8, 2025
88 of 95 checks passed
@jkotas jkotas deleted the backport/pr-111011-to-release/9.0-staging branch January 8, 2025 19:26
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-crossgen2-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants