Skip to content

[release/8.0-staging] Fix generation of minidump #115739

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

Merged
merged 2 commits into from
Jun 9, 2025
Merged

[release/8.0-staging] Fix generation of minidump #115739

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c49f93
Fix generation of minidump
thaystg May 14, 2025
588f339
Merge branch 'release/8.0-staging' into backport/pr-115562-to-release…
tommcdon Jun 9, 2025
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
12 changes: 8 additions & 4 deletions src/coreclr/vm/method.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3798,10 +3798,14 @@ MethodDesc::EnumMemoryRegions(CLRDataEnumMemoryFlags flags)
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(dac_cast<PTR_MethodDesc>(this));
if (!ilVersion.IsNull())
{
ilVersion.GetActiveNativeCodeVersion(dac_cast<PTR_MethodDesc>(this));
ilVersion.GetVersionId();
ilVersion.GetRejitState();
ilVersion.GetIL();
EX_TRY
{
ilVersion.GetActiveNativeCodeVersion(dac_cast<PTR_MethodDesc>(this));
ilVersion.GetVersionId();
ilVersion.GetRejitState();
ilVersion.GetIL();
}
EX_CATCH_RETHROW_ONLY_COR_E_OPERATIONCANCELLED
Copy link
Member

@jkotas jkotas May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why is the exception that this is catching thrown in the first place?

Is this a proper fix or just a quick workaround?

Copy link
Member

Choose a reason for hiding this comment

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

It's a quick/low-risk workaround. I agree we should do a more full version of the fix in main addressing the reason for the crash.

Copy link
Member

Choose a reason for hiding this comment

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

It's a quick/low-risk workaround. I agree we should do a more full version of the fix in main addressing the reason for the crash.

Copy link
Member

Choose a reason for hiding this comment

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

Have we figured out why it is crashing? If we have not figured out why it is crashing, what's our confidence level that this workaround is going to solve the issue reliably and that it won't just break again later when the crash dump is opened in the debugger?

(I am fine with this workaround if we understand the root cause of the crash and determined that this workaround is the right way to solve it in servicing.)

Copy link
Member

Choose a reason for hiding this comment

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

@jkotas I apologize for the delayed response! The crash was triggered by attempting to read the IL off an InlinedCallFrame pointing to an NDirectMethodDesc. At dump collection time, we enumerate all of the threads, and all of the stacks. While walking the frames, we encounted an InlinedCallFrame and called MethodDesc::EnumMemoryRegions on it. This in turn called ILCodeVersion::GetIL, which called MethodDesc::GetILHeader which attempted to get the IL from the module, causing an access violation due to an invalid pointer. @thaystg verified that avoiding reading the IL on NDirect methoddescs avoids the exception - candidate .net 10 change here - #116391. We decided to leave the try/catch in place but also guard the call to retrieve the IL. If the commit in main looks good, should we back port that to this servicing PR or keep the current PR as-is? The updated fix was verified locally.

Copy link
Member

Choose a reason for hiding this comment

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

candidate .net 10 change here - #116391.

I think the change is not quite right: #116391 (comment)

should we back port that to this servicing PR or keep the current PR as-is?

The current PR is fine as-is. Thanks!

}
#endif

Expand Down
Loading