Skip to content

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Mar 25, 2025

Same file content as in dotnet/diagnostics#5369

@Copilot Copilot AI review requested due to automatic review settings March 25, 2025 19:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (5)
  • src/coreclr/gcdump/gcdumpnonx86.cpp: Language not supported
  • src/coreclr/inc/gcinfo.h: Language not supported
  • src/coreclr/inc/gcinfodecoder.h: Language not supported
  • src/coreclr/inc/gcinfotypes.h: Language not supported
  • src/coreclr/vm/gcinfodecoder.cpp: Language not supported

Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented Mar 25, 2025

decoder does not need to worry about parsing GCInfo V1

I assume that we want SOS to work for .NET Framework too. @mikem8361 Is that correct?

If it is the case, we want it to be able to parse GCInfo v1.

@mikem8361
Copy link
Contributor

mikem8361 commented Mar 25, 2025 via email

@VSadov
Copy link
Member Author

VSadov commented Mar 26, 2025

Yes, we want SOS to work for .NET Framework.

How does it get v1? It looks like v3 or v4 is the only option. Same sources used in another repo maybe?

(just want to be sure I am not breaking something I do not notice)

@jkotas
Copy link
Member

jkotas commented Mar 26, 2025

How does it get v1? It looks like v3 or v4 is the only option. Same sources used in another repo maybe?

It is possible that this part of SOS is broken on .NET Framework.

@VSadov
Copy link
Member Author

VSadov commented Mar 26, 2025

I could set version to v1 and shim the parsing for that, assuming the flags bit size is the only difference.

If there is a way to detect .Net Fx via bool IsRuntimeVersion(DWORD major) and bool IsRuntimeVersionAtLeast(DWORD major),

I guess I can just debug and see what happens with .Net Fx

@mikem8361
Copy link
Contributor

This code can tell you if it is .NET Framework:

if (g_pRuntime->GetRuntimeConfiguration() == IRuntime::WindowsDesktop)

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.

Thanks!

// Since in SOS we only care about ability to parse the GC Info,
// we can assume that everything before net10.0 uses GCInfo v3
// v2 and v3 had the same format, so for parsing/dumping purposes they are the same.
// Also, since runtime cannot parse GC info in nondefault format,
Copy link
Member

Choose a reason for hiding this comment

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

since runtime cannot parse GC info in nondefault format,

It is the case at the moment. We had a situation for a while where the runtime parsed multiple version (the older version can come from R2R file). It may be the case again in future.

Copy link
Member Author

@VSadov VSadov Mar 27, 2025

Choose a reason for hiding this comment

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

Anything is possible, I guess. I think the main invariant here, which should hold, is that the default GC Info format can be inferred from runtime`s version. It would be highly unusual for the default format to change without changing at least minor version of the runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will make a change to the comment, just to state as a fact that the default GC Info version can be inferred from the runtime/EE version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, maybe that does not even need mentioning.

Only the part that we treat v2 and v3 the same needs to be commented as it is a simplification that we can allow because the file format was the same.

@VSadov
Copy link
Member Author

VSadov commented Mar 31, 2025

Since we do not have anything to add on the runtime side, I think this can be merged.

@VSadov
Copy link
Member Author

VSadov commented Apr 1, 2025

I see the same failures in a NOOP comment only change - #114048

System.Net.Quic.Functional.Tests and other System.Net.. tests

@VSadov
Copy link
Member Author

VSadov commented Apr 1, 2025

/ba-g System.Net.XXX tests fail in other runs too.

@VSadov VSadov merged commit 3dbeb51 into dotnet:main Apr 1, 2025
96 of 98 checks passed
@VSadov VSadov deleted the gcDecR branch April 1, 2025 23:29
@VSadov
Copy link
Member Author

VSadov commented Apr 1, 2025

Thanks for reviewing!!

@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants