[cDAC] Implement GCCover portion of SOSDacImpl::GetMethodDescData #110322

max-charlamb · 2024-12-02T19:00:30Z

Contributes to #109426

Changes

Adds APIs needed to access the GCStressCodeCopy.
Modifies ICodeVersions contract

/* adds */ public virtual TargetPointer GetGCStressCodeCopy(NativeCodeVersionHandle codeVersionHandle);

Modifies IRuntimeTypeSystem contract

/* adds */ public virtual TargetPointer GetGCStressCodeCopy(MethodDescHandle methodDesc);

Worklog

Add GCCoverageInfo fields
Main Implementation
Unit tests
Update docs
Manual Testing
- To verify I attached WinDBG to a C# program with $env:DOTNET_GCStress=4. This seems to make normal breakpoints not work, but throws an exception on every inserted GC breakpoint. I configured WinDBG to break on these exceptions (sxe c0000096) and used !ip2md <ip> to query the md which contains gccoverage info.

max-charlamb · 2024-12-05T16:11:59Z

src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs

+                        // In certain minidumps, we won't save the gccover information.
+                        // (it would be unwise to do so, it is heavy and not a customer scenario).
+                        Target.TypeInfo gcCoverageInfo = _target.GetTypeInfo(DataType.GCCoverageInfo);
+                        data->GCStressCodeCopy = gcCoverAddr.Value.Value + (ulong)gcCoverageInfo.Fields["SavedCode"].Offset;


I don't like having the inline string here, but I wasn't sure where to store this field.

ivdiazsa · 2024-12-05T17:41:29Z

...ed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCCoverFactory.cs

+{
+    IGCCover IContractFactory<IGCCover>.CreateContract(Target target, int version)
+    {
+        return version switch


I like how you went for the switch despite only having an if/else condition to have it ready for the likely new versions in the future 🙂

I can't really take credit for this. Aleksey used this factory pattern in the other contracts a few months ago. I do agree its a good approach. 😄

src/native/managed/cdacreader/tests/GCCoverTests.cs

docs/design/datacontracts/RuntimeTypeSystem.md

docs/design/datacontracts/GCCover.md

...managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCCover_1.cs

src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs

elinor-fung

Adds IGCCover contract to get access to gc coverage field from native code version.

While this isn't strictly necessary, the API could live on the CodeVersions contract, I thought it made more sense to keep them logically separated.

This seems to be in an in-between state now, since there is a GCCover contract that gives the separation from the CodeVersions contract, but the RuntimeTypeSystem now also deals with GC coverage info.

I think we should either:

Add a GCCover contract that handles getting GC coverage info from a native code version and from a method desc
- I think this would be the logical separation that you mentioned
Add methods to the CodeVersions and RuntimeTypeSystem contracts for getting GC coverage info (no new GCCover contract)
- Since we don't expect the question of 'is GC cover enabled' to be a scenario for a consumer, so it may make sense to have GC coverage info at a lower level instead of elevated to its own contract

src/coreclr/vm/gccover.h

src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs

max-charlamb · 2024-12-10T16:49:09Z

src/native/managed/cdacreader/tests/CodeVersionsTests.cs

+    internal static Target CreateTarget(
+        MockTarget.Architecture arch,
+        MockCodeVersions builder,
+        Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem)
+    {
+        TestPlaceholderTarget target = new TestPlaceholderTarget(
+            arch,
+            builder.Builder.GetReadContext().ReadFromTarget,
+            builder.Types);
+
+        IContractFactory<ICodeVersions> cvfactory = new CodeVersionsFactory();
+        ContractRegistry reg = Mock.Of<ContractRegistry>(
+            c => c.CodeVersions == cvfactory.CreateContract(target, 1)
+                && c.RuntimeTypeSystem == mockRuntimeTypeSystem.Object);
+        target.SetContracts(reg);
+        return target;
+    }


Previous CodeVersion tests use custom "mock" classes. Now that we have Moq integrated, I used it to create the new GCStressCodeCopy tests.

Not for this change, but might be nice to switch the previous/existing tests to this as well.

max-charlamb · 2024-12-10T16:54:17Z

Adds IGCCover contract to get access to gc coverage field from native code version.

While this isn't strictly necessary, the API could live on the CodeVersions contract, I thought it made more sense to keep them logically separated.

This seems to be in an in-between state now, since there is a GCCover contract that gives the separation from the CodeVersions contract, but the RuntimeTypeSystem now also deals with GC coverage info.

I think we should either:

Add a GCCover contract that handles getting GC coverage info from a native code version and from a method desc

I think this would be the logical separation that you mentioned

Add methods to the CodeVersions and RuntimeTypeSystem contracts for getting GC coverage info (no new GCCover contract)

Since we don't expect the question of 'is GC cover enabled' to be a scenario for a consumer, so it may make sense to have GC coverage info at a lower level instead of elevated to its own contract

I agree with this assessment. I ended up going with the second option, moving the API to ICodeVersions. I think this follows existing patterns which read either from a code version node or the method description depending on if the code version is synthetic or explicit.

I think having a separate GCCover contract which handles the method desc and code version node fetching would be confusing as it directly depends on two underlying areas which should be accessed through their respective contracts.

elinor-fung · 2024-12-11T21:50:05Z

docs/design/datacontracts/CodeVersions.md

+```
+
+1. If `codeVersionHandle` is synthetic, use the `IRuntimeTypeSystem` to find the GCStressCodeCopy.
+2. If `codeVersionHandle` is explicit, read the `NativeCodeVersionNode` for the `GCCoverageInfo` pointer. This value is nullable as it only exists in some builds. If the value doesn't exist or is a nullptr, return `TargetPointer.Null`. Otherwise return the `SavedCode` pointer from the `GCCoverageInfo` struct.


Suggested change

2. If `codeVersionHandle` is explicit, read the `NativeCodeVersionNode` for the `GCCoverageInfo` pointer. This value is nullable as it only exists in some builds. If the value doesn't exist or is a nullptr, return `TargetPointer.Null`. Otherwise return the `SavedCode` pointer from the `GCCoverageInfo` struct.

2. If `codeVersionHandle` is explicit, read the `NativeCodeVersionNode` for the `GCCoverageInfo` pointer. This value only exists in some builds. If the value doesn't exist or is a nullptr, return `TargetPointer.Null`. Otherwise return the `SavedCode` pointer from the `GCCoverageInfo` struct.

elinor-fung · 2024-12-11T21:55:32Z

src/native/managed/cdacreader/tests/CodeVersionsTests.cs

+    internal static Target CreateTarget(
+        MockTarget.Architecture arch,
+        MockCodeVersions builder,
+        Mock<IRuntimeTypeSystem> mockRuntimeTypeSystem)
+    {
+        TestPlaceholderTarget target = new TestPlaceholderTarget(
+            arch,
+            builder.Builder.GetReadContext().ReadFromTarget,
+            builder.Types);
+
+        IContractFactory<ICodeVersions> cvfactory = new CodeVersionsFactory();
+        ContractRegistry reg = Mock.Of<ContractRegistry>(
+            c => c.CodeVersions == cvfactory.CreateContract(target, 1)
+                && c.RuntimeTypeSystem == mockRuntimeTypeSystem.Object);
+        target.SetContracts(reg);
+        return target;
+    }


Not for this change, but might be nice to switch the previous/existing tests to this as well.

elinor-fung · 2024-12-11T22:13:10Z

docs/design/datacontracts/CodeVersions.md

@@ -40,6 +40,9 @@ public virtual bool CodeVersionManagerSupportsMethod(TargetPointer methodDesc);

 // Return the instruction pointer corresponding to the start of the given native code version
 public virtual TargetCodePointer GetNativeCode(NativeCodeVersionHandle codeVersionHandle);
+
+// Gets the GCStressCodeCopy pointer if available, otherwise returns TargetPointer.Null
+public virtual TargetPointer GetGCStressCodeCopy(NativeCodeVersionHandle codeVersionHandle);


Do we want to refer to it as 'GCCoverage' or 'GCStress'? I kind of think of GC coverage as the more basic feature/info that GC stress uses. I don't feel too strongly either way.

dotnet-issue-labeler bot added the area-Diagnostics-coreclr label Dec 2, 2024

dotnet-policy-service bot assigned max-charlamb Dec 2, 2024

max-charlamb force-pushed the md-gccover branch from 7a104d0 to 3cf533a Compare December 3, 2024 17:58

max-charlamb mentioned this pull request Dec 3, 2024

[cdac] Finish unimplemented parts of GetMethodDescData #109426

Open

11 tasks

max-charlamb force-pushed the md-gccover branch from e5799e7 to 5e9c139 Compare December 4, 2024 16:14

max-charlamb marked this pull request as ready for review December 4, 2024 16:59

max-charlamb requested review from elinor-fung, davidwrighton, AaronRobinsonMSFT and a team December 4, 2024 17:03

max-charlamb commented Dec 5, 2024

View reviewed changes

ivdiazsa reviewed Dec 5, 2024

View reviewed changes

src/native/managed/cdacreader/tests/GCCoverTests.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Dec 5, 2024

View reviewed changes

docs/design/datacontracts/RuntimeTypeSystem.md Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Dec 5, 2024

View reviewed changes

docs/design/datacontracts/GCCover.md Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Dec 5, 2024

View reviewed changes

...managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GCCover_1.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Dec 5, 2024

View reviewed changes

src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated Show resolved Hide resolved

max-charlamb requested a review from AaronRobinsonMSFT December 5, 2024 19:16

This was referenced Dec 5, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

elinor-fung reviewed Dec 7, 2024

View reviewed changes

src/coreclr/vm/gccover.h Outdated Show resolved Hide resolved

src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated Show resolved Hide resolved

src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated Show resolved Hide resolved

Max Charlamb added 3 commits December 10, 2024 11:33

initial support for HAVE_GCCOVER

41767c9

fix typos

6e6461d

add tests

0129e85

Max Charlamb added 8 commits December 10, 2024 11:35

add GCCover contract docs

8f629fd

fix bugs

f1e9011

fix merge issues

8526183

remove unused fields

a0194d2

address comments

e2ecb75

address simple comments

15ec95e

remove GCCover contract and move API to CodeVersions

771949d

fix merge issue

635a966

max-charlamb force-pushed the md-gccover branch from 4fc1b6f to 635a966 Compare December 10, 2024 16:41

Max Charlamb added 3 commits December 10, 2024 11:41

delete gccover.md

2396d06

improve doc

61dde80

improve comments

90584bf

max-charlamb commented Dec 10, 2024

View reviewed changes

max-charlamb requested a review from elinor-fung December 10, 2024 16:54

fix datadescriptor

090adca

This was referenced Dec 10, 2024

[wasm] Out of memory exception in System.Xml.XmlWriterApiTests.TCFullEndElement tests #110090

Open

Test failures in JSImportGenerator.Unit.Tests.Compiles.ValidateGeneratedSourceOutput #110575

Closed

Max Charlamb added 3 commits December 10, 2024 17:02

fix gcstresscode

991b217

fix null check

cf7a5f1

improve test to differentiate if SavedCode offset is added

a6eefcb

elinor-fung approved these changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cDAC] Implement GCCover portion of SOSDacImpl::GetMethodDescData #110322

[cDAC] Implement GCCover portion of SOSDacImpl::GetMethodDescData #110322

max-charlamb commented Dec 2, 2024 •

edited

Loading

max-charlamb Dec 5, 2024

ivdiazsa Dec 5, 2024

max-charlamb Dec 5, 2024

elinor-fung left a comment

max-charlamb Dec 10, 2024

elinor-fung Dec 11, 2024

max-charlamb commented Dec 10, 2024

elinor-fung Dec 11, 2024

elinor-fung Dec 11, 2024

elinor-fung Dec 11, 2024

	2. If `codeVersionHandle` is explicit, read the `NativeCodeVersionNode` for the `GCCoverageInfo` pointer. This value is nullable as it only exists in some builds. If the value doesn't exist or is a nullptr, return `TargetPointer.Null`. Otherwise return the `SavedCode` pointer from the `GCCoverageInfo` struct.
	2. If `codeVersionHandle` is explicit, read the `NativeCodeVersionNode` for the `GCCoverageInfo` pointer. This value only exists in some builds. If the value doesn't exist or is a nullptr, return `TargetPointer.Null`. Otherwise return the `SavedCode` pointer from the `GCCoverageInfo` struct.

[cDAC] Implement GCCover portion of SOSDacImpl::GetMethodDescData #110322

Are you sure you want to change the base?

[cDAC] Implement GCCover portion of SOSDacImpl::GetMethodDescData #110322

Conversation

max-charlamb commented Dec 2, 2024 • edited Loading

Changes

Worklog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-charlamb commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-charlamb commented Dec 2, 2024 •

edited

Loading