Skip to content

[WIP] Add linked createdump support for NativeAOT executables - #134687

Open
eduardo-vp wants to merge 27 commits into
dotnet:mainfrom
eduardo-vp:linked-createdump
Open

eduardo-vp wants to merge 27 commits into
dotnet:mainfrom
eduardo-vp:linked-createdump

Conversation

@eduardo-vp

@eduardo-vp eduardo-vp commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

The original motivation was to have a statically linked version of createdump embedded into Native AOT executables, allowing an executable to collect a dump on its own. Since Native AOT avoids libstdc++ and links against a shim instead, this embedded version must also avoid components that rely on it (DAC, minipal, etc.). This refactor splits the existing code into a shared core without libstdc++ (used by both the embedded and standalone versions) and an external-only portion for the standalone createdump.

Also added a test to verify the behavior of both versions. The test defaults to using the embedded createdump via an MSBuild property, but later sets an environment variable to force using the standalone one. It verifies that the correct implementation was invoked and checks the dump content.

The diff is large but several parts were pretty much just moved to other files.

Key changes include:

CrashInfo Refactoring and Memory Management

  • Refactored the CrashInfo class to use a shared ProcessInfo object for process state, removing direct process state members and related platform-specific code from CrashInfo.
  • Refactored ThreadInfo class to use a shared ThreadSnapshot object for fields that should be used by both implementations.

  • Add a DumpRegionStore class that allows the code to use some common functions to handle memory regions by abstracting the way to insert and find a memory region in a container. This way external createdump can remain efficient by using std::set.

Shared Code Consolidation:

  • Common dump creation logic and helpers have been moved into new shared source files (e.g., shared/createdumpcore.cpp, shared/dumpname.cpp, shared/crashinfocore.cpp, shared/dumpwriter.cpp, etc.).

Memory Region Management Improvements:

  • Memory region insertion and overlap checks are now delegated to the new ProcessInfo abstraction and shared helpers, simplifying the logic in CrashInfo. The region combination logic is refactored to use a shared utility function. (src/coreclr/debug/createdump/crashinfo.cpp)

Note

This pull request description was generated with GitHub Copilot.

@azure-pipelines

azure-pipelines Bot commented Sep 25, 2026 •

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: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

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

The validation assumes 4-KiB system pages indirectly and does not enforce use of linked createdump.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Adds a NativeAOT smoke test for validating external and future linked createdump output.

Changes:

  • Exercises automatic and forced-external dump generation.
  • Validates ELF notes, memory mappings, and managed exception diagnostics.
File Description
CreatedumpValidation.csproj Configures the Linux NativeAOT smoke test.
CreatedumpValidation.cs Generates and validates crash dumps.

Comment thread src/tests/nativeaot/SmokeTests/CreatedumpValidation/CreatedumpValidation.cs Outdated

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

Linked support is not exercised, build integration is absent, and sanitizer runs risk enormous full dumps.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (2)

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

🔵 Needs a closer look

The central linked-createdump assertion remains commented out, so the intended behavior is not yet enforced.

Review effort: Balanced
Findings: None

Resolved since last review (2)

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 encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

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

🔵 Needs a closer look

The assertion distinguishing linked createdump from the external helper remains disabled, leaving the stated linked behavior untested.

Review effort: Balanced
Findings: None

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

The process-state refactor introduces two confirmed initialization regressions, and the PR description does not represent the implementation scope.

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

Open (3)

Comment thread src/coreclr/debug/createdump/crashinfounix.cpp Outdated
Comment thread src/coreclr/debug/createdump/shared/createdumpunixcore.cpp Outdated

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

Dump-name formatting now rejects previously valid long Unix paths, and the description materially understates the production scope.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (3)

Comment thread src/coreclr/nativeaot/Runtime/createdump/CMakeLists.txt

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

🔵 Needs a closer look

The formatter introduces a dump-path compatibility regression, and the refactor leaves dead global state.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Avoid hard-capping formatted dump paths at 1,023 bytes

src/​coreclr/​debug/​createdump/​shared/​dumpname.cpp:59

This introduces a compatibility regression by hard-capping every formatted dump path at 1,023 bytes. The previous std::string implementation grew as needed, so external createdump accepted valid Unix paths beyond this artificial 1 KB limit (up to the platform's path limit). Please preserve the existing external behavior—e.g., use dynamically owned storage there—while allowing the linked implementation to use bounded storage if required.

Low severity Remove unused linkedCreateDump global

src/​coreclr/​debug/​createdump/​createdumpmain.cpp:9

linkedCreateDump has no references anywhere in the createdump sources, so this new externally linked global is dead state. Remove it to avoid leaving an unexplained symbol behind from the refactor.

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

🔵 Needs a closer look

This WIP substantially changes low-level crash handling, process suspension, dump serialization, and NativeAOT linking across platforms, requiring final maintainer review.

Review effort: Balanced
Findings: None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not much of this seems to be native AOT specific. Should this go somewhere where we can also run it with CoreCLR?

It would be more of a question for diagnostics team - how/where do we test createdump? Is "handwritten test-local parser can still parse this" the right success criteria (do we have parser API that we should be checking instead)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes I think running it with CoreCLR might be good. Also, this test was built considering dumps built by the linked createdump, not sure if there's room to add better validation on dumps created by the original createdump.

@eduardo-vp

Copy link
Copy Markdown
Member Author

cc @dotnet/dotnet-diag-contrib

@agocke

agocke commented Oct 2, 2026

Copy link
Copy Markdown
Member

Yeah, I would split this PR up. My immediate thoughts are:

  1. Add testing, if it's valuable. If we already have testing that covers this, I don't think the testing is worth it.
  2. Add createdump functionality. If the new createdump features are useful, we can start by just putting the code directly into createdump.
  3. Add linking between NAOT and createdump.

Basically, I think the actual extra functionality for NAOT should be pretty limited, if the rest of the stack is properly factored.

@agocke

agocke commented Oct 2, 2026

Copy link
Copy Markdown
Member

One more thing: the actual testing I did when I wrote the original code was load the produced jump into GDB and do some basic validation.

@eduardo-vp

eduardo-vp commented Oct 6, 2026 •

Copy link
Copy Markdown
Member Author

It looks like all the validation we need for createdump is in the runtime-diagnostics pipeline. I think we can update it such that it gets triggered if it detects changes to src/coreclr/debug/createdump/**. Should be fine to drop the test in this PR.

I think the actual extra functionality for NAOT should be pretty limited, if the rest of the stack is properly factored.

Yes I think so. In that case I think a separate PR could be only the refactor of createdump into code that leverage libstdc++ and code that doesn't need to (in a shared folder that Native AOT can use). A subsequent PR with the linking should be quite simple.

@eduardo-vp

Copy link
Copy Markdown
Member Author

It looks like all the validation we need for createdump is in the runtime-diagnostics pipeline. I think we can update it such that it gets triggered if it detects changes to src/coreclr/debug/createdump/**.

Actually I guess running the full pipeline for createdump changes alone might be too expensive (although changes there seem relatively uncommon). In any case I'll run it manually for these PRs since the diagnostics team is in a better position to decide whether to update the pipeline's path filters.

@dotnet/dotnet-diag

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.

4 participants