Skip to content

Refactor createdump - #135257

Open
eduardo-vp wants to merge 6 commits into
dotnet:mainfrom
eduardo-vp:createdump-refactor
Open

eduardo-vp wants to merge 6 commits into
dotnet:mainfrom
eduardo-vp:createdump-refactor

Conversation

@eduardo-vp

@eduardo-vp eduardo-vp commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Summary

Refactor createdump so the process-inspection and dump-writing code that can be shared with a linked NativeAOT createdump lives under src/coreclr/debug/createdump/shared.

This PR is preparatory refactoring only. It does not enable linked createdump for NativeAOT.

Changes

  • Split process initialization, thread snapshots, memory-map enumeration, dump-region selection, option parsing, and dump naming into shared implementations.
  • Keep external createdump's DAC integration and std::set-based region storage in CrashInfo while exposing only the operations required by the shared algorithms.
  • Add lightweight shared DynamicArray and owned-string utilities that do not require standard-library containers.
  • Make the ELF writer generic over the module-mapping and dump-region containers, allowing external createdump to use std::set and a future linked implementation to use DynamicArray without copying regions.
  • Separate opening a dump from writing it and retain the existing ELF and Mach-O writer paths.
  • Preserve existing external createdump behavior, including dump naming, diagnostics, crash metadata, thread contexts, and partial-file cleanup.

The runtime-diagnostics pipeline was run for this commit and the dump tests pass.

Note

This PR description was generated with GitHub Copilot.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

@eduardo-vp
eduardo-vp force-pushed the createdump-refactor branch from fa20ffc to c249b2e Compare October 6, 2026 05:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Failed dump writes now leave truncated files behind instead of deleting them.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Refactors createdump by separating process/thread snapshots, shared dump-writing logic, and platform-specific orchestration.

Changes:

  • Introduces shared process, thread, memory-region, and utility abstractions.
  • Reworks ELF/Mach-O writers around captured snapshots.
  • Moves common parsing, naming, and lifecycle logic into shared sources.
File Description
threadinfounix.cpp Uses captured thread snapshots.
threadinfomac.cpp Moves Mach thread capture into snapshots.
threadinfo.h Delegates thread state to snapshots.
threadinfo.cpp Updates snapshot-backed access.
shared/​threadsnapshot.h Defines portable thread snapshots.
shared/​threadsnapshot.cpp Captures Unix registers.
shared/​specialdiaginfo.h Shares diagnostic dump metadata.
shared/​processinfo.h Defines captured process state.
shared/​memoryregion.h Adds movable region storage.
shared/​dumpwritermacho.h Shares Mach-O writer declarations.
shared/​dumpwritermacho.cpp Updates Mach-O thread serialization.
shared/​dumpwriterelf.inl Generalizes ELF dump writing.
shared/​dumpwriterelf.h Defines snapshot-based ELF writer.
shared/​dumpwriterelf.cpp Writes ELF process and thread notes.
shared/​dumpwriter.h Selects the platform writer.
shared/​dumpwriter.cpp Shares writer lifecycle and metadata.
shared/​dumpname.cpp Adds bounded dump-name formatting.
shared/​createdumpunixcore.cpp Implements Unix process initialization.
shared/​createdumpcore.h Defines shared createdump contracts.
shared/​createdumpcore.cpp Shares parsing and diagnostics.
shared/​crashinfounixcore.cpp Captures Linux process mappings.
shared/​crashinfocore.cpp Shares dump-region selection.
shared/​coreutils.h Adds no-STL strings and arrays.
dumpwritermacho.h Removes relocated declarations.
createdumpwindows.cpp Uses bounded name formatting.
createdumpunix.cpp Orchestrates the new snapshot flow.
createdumpmain.cpp Uses shared parsing and defaults.
createdump.h Includes shared core definitions.
crashinfounix.cpp Delegates process capture.
crashinfomac.cpp Integrates macOS snapshots.
crashinfo.h References shared process state.
crashinfo.cpp Populates from process snapshots.
CMakeLists.txt Builds the relocated shared sources.

Comment thread src/coreclr/debug/createdump/createdumpunix.cpp Outdated
Comment thread src/coreclr/debug/createdump/shared/threadsnapshot.h Outdated
@eduardo-vp

Copy link
Copy Markdown
Member Author

/azp run runtime-diagnostics

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Allocation failures can silently produce incomplete module mappings and apparently successful dumps.

0 open findings

2 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Propagate module mapping allocation failure

src/​coreclr/​debug/​createdump/​crashinfo.cpp:435

AddOrReplaceModuleMapping now returns false when allocating the owned filename fails, but this caller only traces the failure and continues. That allows dump generation to succeed with a missing or stale module mapping. Propagate the failure so the existing GatherCrashInfo error path aborts the dump.

Medium severity Propagate segment filename allocation failure

src/​coreclr/​debug/​createdump/​crashinfomac.cpp:341

SetFileName can fail on OOM, but returning from this void visitor merely skips the segment and lets dump creation report success with an incomplete module map. Propagate the failure through segment enumeration or fail fast instead of silently continuing.

This issue also appears on line 368 of the same file.

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Value-taking command-line options can dereference a missing argument and crash createdump.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Validate arguments before parsing value-taking options

src/​coreclr/​debug/​createdump/​shared/​createdumpcore.cpp:116

Validate that each value-taking option has a following argument before incrementing argv. For example, invoking createdump --signal currently calls atoi(nullptr) and can crash instead of reporting invalid usage. The same check is needed for -f/--name, --crashthread, --code, --errno, --address, --exception-record, and -l/--logtofile.

🧠 Review effort: Balanced

@eduardo-vp
eduardo-vp marked this pull request as ready for review October 8, 2026 21:05

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants