Skip to content

test(native): fix minidump context layout assertions - #2216

Merged
jpnurmi merged 3 commits into
getsentry:masterfrom
IanHollow:codex/minidump-layout-ad28578f
Oct 19, 2026
Merged

jpnurmi merged 3 commits into
getsentry:masterfrom
IanHollow:codex/minidump-layout-ad28578f

Conversation

@IanHollow

Copy link
Copy Markdown
Contributor

Summary

SENTRY_BACKEND=native fails minidump_structures_packed on Linux x86-64 because the expected-size expression adds up to 1244, although its comment says 1232. The production structure is already 1232 bytes, as independently checked with GCC and Clang and required by the minidump format.

 minidump_structures_packed
-  compare AMD64 size against an incorrect sum
-  accept ARM64 sizes up to 1012 bytes
+  require the format sizes: AMD64 1232, ARM64 912
+  check register-group offsets and the packed memory-list array offset

Also correct the misleading ARM64 size comment in minidump_context_sizes. This changes one test file; production structures and serialized output stay as they are.

The expected layout comes from Microsoft's AMD64 CONTEXT, ARM64_NT_CONTEXT, and MINIDUMP_MEMORY_LIST. The SDK's pinned Crashpad context definitions and explicit 1232-byte AMD64 assertion agree.

Evidence

Tested on current master bbbd78cd714e282c900d1d19aa75da6b93962e98, with recursive submodules initialized and the pinned formatter requirements installed.

  • Before: sentry_test_unit --no-summary minidump_structures_packed fails at the AMD64 expected-size assertion in an unmodified native-backend build. GCC and Clang layout probes both report actual size 1232 versus expected sum 1244.
  • After: the focused layout cases and the complete native unit binary pass on Linux x86-64 with GCC (Debug and RelWithDebInfo), Clang (RelWithDebInfo), and native ARM64 macOS with Apple Clang (RelWithDebInfo).
  • Valgrind passes both layout cases with full leak checking, zero errors, and no leaked blocks. Upstream make format and make style pass.
  • Mutation probes compile and execute the exact updated assertion body with both GCC and Clang: swapping the equal-width context-flags/MxCsr fields fails the new offset check while keeping total context size 1232. Removing PACKED_ATTR fails the memory-list array offset check, again while the AMD64 total stays 1232. The correct header passes both compilers. These are isolated probe mutations, not changes to SDK source in this PR.

A disposable combined-source check with the separate #2215 CTest setup patch passes all 614 Linux native CTest registrations in parallel and with two consecutive repetitions. That CTest patch is not included in this PR.

The independent default-layout/shared-fixture CTest issues are addressed separately in #2215. Windows, 32-bit architectures and the full HTTP integration suite were not run locally.

Merge Danger

Door: two-way; this test-only commit is independently revertible.

Blast Radius: tests. Exact offsets can expose a real compiler/platform format-layout regression that total-size assertions alone would miss. They should fail rather than silently accept a different wire layout. No runtime, dependency or packaging changes.

The AMD64 packed-structure test expects an arithmetic sum of 1244 even
though the wire format and production context are 1232 bytes. Use exact
format sizes and check register-group offsets so same-size field-order
regressions are detected too. Require the exact ARM64 layout instead of
allowing another 100 bytes, and correct its misleading size comment.

Check the memory-list array offset to detect lost structure packing.
No production structures or serialized output change.

The original focused test fails on unmodified master. Focused and complete
native unit runs pass with GCC Debug/RelWithDebInfo and Clang on Linux,
and Apple Clang on ARM64 macOS. Valgrind reports no errors or leaks for
the layout cases. Both compilers reject isolated field-order and packing
mutations under the new assertions. With the separate CTest setup patch,
all 614 Linux native registrations pass repeated parallel CTest runs.
Formatting and style checks pass; Windows/32-bit execution is untested.

Assisted-by: OpenAI Codex
The unit suite was only ever built with `SENTRY_BACKEND=none`, so every
test guarded by `SENTRY_BACKEND_NATIVE` in `test_native_backend.c`
compiled to `SKIP_TEST()` in CI. Add a `test_unit_native` variant that
builds `sentry_test_unit` together with `sentry-crash`, which
`sentry_init` spawns for the native backend.
@jpnurmi

jpnurmi commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the fix! 🙏

I pushed a small follow-up for running native unit tests in the CI so similar issues will be caught in the future: ad5d981

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ad5d981. Configure here.

Comment thread tests/test_unit.py Outdated
@jpnurmi

jpnurmi commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

I pushed a small follow-up for running native unit tests in the CI so similar issues will be caught in the future: ad5d981

Never mind, ASan needs sigaltstack changes, so I'll take it in a separate follow-up.

@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.52%. Comparing base (bbbd78c) to head (b73991f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2216      +/-   ##
==========================================
- Coverage   75.61%   75.52%   -0.10%     
==========================================
  Files         103      103              
  Lines       28261    28261              
  Branches     5179     5179              
==========================================
- Hits        21369    21343      -26     
- Misses       5536     5570      +34     
+ Partials     1356     1348       -8     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jpnurmi
jpnurmi merged commit 6fd55cb into getsentry:master Oct 19, 2026
62 checks passed
jpnurmi added a commit that referenced this pull request Oct 19, 2026
jpnurmi added a commit that referenced this pull request Oct 20, 2026
jpnurmi added a commit that referenced this pull request Oct 20, 2026
* chore: Update CHANGELOG.md

* backfill thank you for #2216

---------

Co-authored-by: jpnurmi <140617+jpnurmi@users.noreply.github.com>
Co-authored-by: J-P Nurmi <jpnurmi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants