Skip to content

Commit 77081c0

Browse files
noahfalkCopilot
andcommitted
Address PR feedback: use typed pointers, move validation inside try/catch, use heapData.GenerationTable.Count, and use local buffers for debug cross-validation
- Change void* to DacpGenerationData* in ISOSDacInterface8 methods - Move null/validation checks inside try/catch blocks per PR #124814 pattern - Replace ReadGlobal<uint>(TotalGenerationCount) with heapData.GenerationTable.Count - Use local buffers for legacy DAC cross-validation to avoid overwriting cDAC data - Use 'is null'/'is not null' instead of '== null'/'!= null' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 9da7539 commit 77081c0

8 files changed

Lines changed: 275 additions & 126 deletions

File tree

eng/pipelines/runtime-diagnostics.yml

Lines changed: 110 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,13 @@ parameters:
55
displayName: Diagnostics Branch
66
type: string
77
default: main
8+
- name: cdacDumpPlatforms
9+
displayName: cDAC Dump Platforms
10+
type: object
11+
default:
12+
- windows_x64
13+
- linux_x64
14+
# - osx_x64 # Temporarily due to CI capacity constraints. Will re-enable once osx queues are more available.
815

916
resources:
1017
repositories:
@@ -103,8 +110,15 @@ extends:
103110
- task: PublishPipelineArtifact@1
104111
inputs:
105112
targetPath: '$(Build.SourcesDirectory)/artifacts/tmp/$(_BuildConfig)/dumps'
106-
artifactName: Dumps_$(_PhaseName)_Attempt$(System.JobAttempt)
107-
displayName: 'Publish Dumps on Failure'
113+
artifactName: 'Dumps_cDAC_$(osGroup)$(osSubgroup)_$(archType)_$(_BuildConfig)_Attempt$(System.JobAttempt)'
114+
displayName: 'Publish Crash Dumps'
115+
continueOnError: true
116+
condition: failed()
117+
- task: PublishPipelineArtifact@1
118+
inputs:
119+
targetPath: '$(Build.SourcesDirectory)/artifacts/TestResults'
120+
artifactName: 'TestResults_cDAC_$(osGroup)$(osSubgroup)_$(archType)_$(_BuildConfig)_Attempt$(System.JobAttempt)'
121+
displayName: 'Publish Test Results and SOS Logs'
108122
continueOnError: true
109123
condition: failed()
110124
- template: /eng/pipelines/common/platform-matrix.yml
@@ -144,7 +158,99 @@ extends:
144158
- task: PublishPipelineArtifact@1
145159
inputs:
146160
targetPath: '$(Build.SourcesDirectory)/artifacts/tmp/$(_BuildConfig)/dumps'
147-
artifactName: Dumps_$(_PhaseName)_Attempt$(System.JobAttempt)
148-
displayName: 'Publish Dumps on Failure'
161+
artifactName: 'Dumps_DAC_$(osGroup)$(osSubgroup)_$(archType)_$(_BuildConfig)_Attempt$(System.JobAttempt)'
162+
displayName: 'Publish Crash Dumps'
163+
continueOnError: true
164+
condition: failed()
165+
- task: PublishPipelineArtifact@1
166+
inputs:
167+
targetPath: '$(Build.SourcesDirectory)/artifacts/TestResults'
168+
artifactName: 'TestResults_DAC_$(osGroup)$(osSubgroup)_$(archType)_$(_BuildConfig)_Attempt$(System.JobAttempt)'
169+
displayName: 'Publish Test Results and SOS Logs'
149170
continueOnError: true
150171
condition: failed()
172+
173+
#
174+
# cDAC Dump Creation — Build runtime, create crash dumps, publish dump artifacts
175+
#
176+
- stage: DumpCreation
177+
dependsOn: []
178+
jobs:
179+
- template: /eng/pipelines/common/platform-matrix.yml
180+
parameters:
181+
jobTemplate: /eng/pipelines/common/global-build-job.yml
182+
buildConfig: release
183+
platforms: ${{ parameters.cdacDumpPlatforms }}
184+
jobParameters:
185+
buildArgs: -s clr+libs+tools.cdac -c $(_BuildConfig) -rc $(_BuildConfig) -lc $(_BuildConfig)
186+
nameSuffix: CdacDumpGeneration
187+
timeoutInMinutes: 120
188+
postBuildSteps:
189+
- script: $(Build.SourcesDirectory)$(dir).dotnet$(dir)dotnet$(exeExt) msbuild
190+
$(Build.SourcesDirectory)/src/native/managed/cdac/tests/DumpTests/Microsoft.Diagnostics.DataContractReader.DumpTests.csproj
191+
/t:GenerateAllDumps
192+
/p:CIDumpVersionsOnly=true
193+
/p:SetDisableAuxProviderSignatureCheck=true
194+
/p:TargetArchitecture=$(archType)
195+
-bl:$(Build.SourcesDirectory)/artifacts/log/DumpGeneration.binlog
196+
displayName: 'Generate cDAC Dumps'
197+
- template: /eng/pipelines/common/upload-artifact-step.yml
198+
parameters:
199+
rootFolder: $(Build.SourcesDirectory)/artifacts/dumps/cdac
200+
includeRootFolder: false
201+
archiveType: tar
202+
archiveExtension: .tar.gz
203+
tarCompression: gz
204+
artifactName: CdacDumps_$(osGroup)_$(archType)
205+
displayName: cDAC Dump Artifacts
206+
207+
#
208+
# cDAC Dump Tests — Download dumps from all platforms, run tests cross-platform
209+
#
210+
- stage: DumpTest
211+
dependsOn:
212+
- DumpCreation
213+
jobs:
214+
- template: /eng/pipelines/common/platform-matrix.yml
215+
parameters:
216+
jobTemplate: /eng/pipelines/common/global-build-job.yml
217+
buildConfig: release
218+
platforms: ${{ parameters.cdacDumpPlatforms }}
219+
jobParameters:
220+
buildArgs: -s tools.cdacdumptests /p:SkipDumpVersions=net10.0
221+
nameSuffix: CdacDumpTests
222+
timeoutInMinutes: 60
223+
postBuildSteps:
224+
# Download and test against dumps from each platform
225+
- ${{ each dumpPlatform in parameters.cdacDumpPlatforms }}:
226+
- template: /eng/pipelines/common/download-artifact-step.yml
227+
parameters:
228+
artifactName: CdacDumps_${{ dumpPlatform }}
229+
artifactFileName: CdacDumps_${{ dumpPlatform }}.tar.gz
230+
unpackFolder: $(Build.SourcesDirectory)/artifacts/dumps/${{ dumpPlatform }}
231+
displayName: '${{ dumpPlatform }} Dumps'
232+
- script: $(Build.SourcesDirectory)$(dir).dotnet$(dir)dotnet$(exeExt) test
233+
$(Build.SourcesDirectory)/src/native/managed/cdac/tests/DumpTests/Microsoft.Diagnostics.DataContractReader.DumpTests.csproj
234+
--no-build
235+
--logger "trx;LogFileName=CdacDumpTests_${{ dumpPlatform }}.trx"
236+
--results-directory $(Build.SourcesDirectory)/artifacts/TestResults/$(_BuildConfig)/${{ dumpPlatform }}
237+
displayName: 'Run cDAC Dump Tests (${{ dumpPlatform }} dumps)'
238+
continueOnError: true
239+
env:
240+
CDAC_DUMP_ROOT: $(Build.SourcesDirectory)/artifacts/dumps/${{ dumpPlatform }}
241+
- task: PublishTestResults@2
242+
displayName: 'Publish Results ($(osGroup)-$(archType) → ${{ dumpPlatform }})'
243+
inputs:
244+
testResultsFormat: VSTest
245+
testResultsFiles: '**/*.trx'
246+
searchFolder: '$(Build.SourcesDirectory)/artifacts/TestResults/$(_BuildConfig)/${{ dumpPlatform }}'
247+
testRunTitle: 'cDAC Dump Tests $(osGroup)-$(archType) → ${{ dumpPlatform }}'
248+
failTaskOnFailedTests: true
249+
publishRunAttachments: true
250+
buildConfiguration: $(_BuildConfig)
251+
continueOnError: true
252+
condition: always()
253+
# Fail the job if any test or publish step above reported issues.
254+
- script: echo "One or more dump test steps failed." && exit 1
255+
displayName: 'Fail if tests failed'
256+
condition: eq(variables['Agent.JobStatus'], 'SucceededWithIssues')

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ISOSDacInterface.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -830,13 +830,13 @@ public unsafe partial interface ISOSDacInterface8
830830

831831
// WKS
832832
[PreserveSig]
833-
int GetGenerationTable(uint cGenerations, /*struct DacpGenerationData*/ void* pGenerationData, uint* pNeeded);
833+
int GetGenerationTable(uint cGenerations, DacpGenerationData* pGenerationData, uint* pNeeded);
834834
[PreserveSig]
835835
int GetFinalizationFillPointers(uint cFillPointers, ClrDataAddress* pFinalizationFillPointers, uint* pNeeded);
836836

837837
// SVR
838838
[PreserveSig]
839-
int GetGenerationTableSvr(ClrDataAddress heapAddr, uint cGenerations, /*struct DacpGenerationData*/ void* pGenerationData, uint* pNeeded);
839+
int GetGenerationTableSvr(ClrDataAddress heapAddr, uint cGenerations, DacpGenerationData* pGenerationData, uint* pNeeded);
840840
[PreserveSig]
841841
int GetFinalizationFillPointersSvr(ClrDataAddress heapAddr, uint cFillPointers, ClrDataAddress* pFinalizationFillPointers, uint* pNeeded);
842842

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/README.md

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,13 @@ replace it with a cDAC implementation following this pattern:
1111
```csharp
1212
int ISOSDacInterface8.ExampleMethod(uint* pResult)
1313
{
14-
// 1. Validate pointer arguments before the try block
15-
if (pResult == null)
16-
return HResults.E_INVALIDARG;
17-
1814
int hr = HResults.S_OK;
1915
try
2016
{
17+
// 1. Validate pointer arguments inside the try block
18+
if (pResult is null)
19+
throw new ArgumentException();
20+
2121
// 2. Get the relevant contract and call it
2222
IGC gc = _target.Contracts.GC;
2323
*pResult = gc.SomeMethod();
@@ -48,8 +48,8 @@ int ISOSDacInterface8.ExampleMethod(uint* pResult)
4848

4949
- **HResult returns**: Methods return `int` HResult codes, not exceptions.
5050
Use `HResults.S_OK`, `HResults.S_FALSE`, `HResults.E_INVALIDARG`, etc.
51-
- **Null pointer checks**: Validate output pointer arguments *before* the try block
52-
and return `E_INVALIDARG`. This matches the native DAC behavior.
51+
- **Null pointer checks**: Validate output pointer arguments *inside* the try block
52+
and throw `ArgumentException`. The catch block converts this to an HResult code.
5353
- **Exception handling**: Wrap all contract calls in try/catch. The catch converts
5454
exceptions to HResult codes via `ex.HResult`. When the native DAC has an explicit
5555
readability check (e.g., `ptr.IsValid()` or `DACGetMethodTableFromObjectPointer`
@@ -88,7 +88,7 @@ int GetSomeTable(uint count, Data* buffer, uint* pNeeded)
8888

8989
The protocol is:
9090
1. Always set `*pNeeded` to the required count (if `pNeeded` is not null).
91-
2. If `count > 0 && buffer == null`: return `E_INVALIDARG`.
91+
2. If `count > 0 && buffer is null`: throw `ArgumentException`.
9292
3. If `count < needed`: return `S_FALSE` (buffer too small, but `*pNeeded` is set).
9393
4. If `count >= needed`: populate `buffer` and return `S_OK`.
9494

0 commit comments

Comments
 (0)