Skip to content

test(graduation): give the width tests' tag ownership of its mesh - #499

Merged
gouarin merged 1 commit into
hpc-maths:mainfrom
gouarin:fix-graduation-test-dangling-mesh
Sep 2, 2026
Merged

gouarin merged 1 commit into
hpc-maths:mainfrom
gouarin:fix-graduation-test-dangling-mesh

Conversation

@gouarin

@gouarin gouarin commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

make_graduation_width_test_tag, added in #498, built the mesh as a local variable and
returned a tag on it. A field only points to its mesh (inner_mesh_type stores a raw pointer),
so graduation.width_zero_is_a_noop and graduation.width_above_dispatch_range_throws run
samurai::graduation on a destroyed mesh.

On main the two tests pass by luck: the stack slot is still intact when the tag is read. On
the branches rebased on main today (#492, #493, #494) the full test run fails with

Expected: samurai::graduation(tag, stencil) doesn't throw an exception.
  Actual: it throws std::out_of_range with description
  "LevelCellArray::get_interval: interval not found at level 4098, i = [0,8[@0:2, index = ".

while the same test passes when run alone. AddressSanitizer confirms the cause on main
itself:

ERROR: AddressSanitizer: stack-use-after-return
    #0 samurai::graduation<...>  include/samurai/algorithm/graduation.hpp:141
    #1 graduation_width_zero_is_a_noop_Test::TestBody()  tests/test_graduation.cpp:284
Address is located in stack of thread T0 at offset ... in frame
    #0 make_graduation_width_test_tag  tests/test_graduation.cpp:267

The tag is now built on holder(mesh): the field owns a copy of the mesh, so the helper is safe
by construction, the way reconstruction() already returns a field that outlives its local mesh.

Returning the mesh and building the tag in each test was considered and rejected: it keeps the
lifetime rule at every call site, and a one-liner passing a temporary mesh would recreate the bug
without any warning.

How has this been tested?

  • tests/test_graduation.cpp, built with -fsanitize=address: the report above before the
    change, clean after it, graduation.* all passing.
  • pre-commit run --files tests/test_graduation.cpp.

Code of Conduct

By submitting this PR, you agree to follow our Code of Conduct

  • I agree to follow this project's Code of Conduct

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

make_graduation_width_test_tag built the mesh as a local and returned a
tag pointing at it. A plain field only points to its mesh, so the two
width tests ran graduation() on a destroyed mesh: AddressSanitizer reports
a stack-use-after-return on main, and in a Release build the outcome
depends on what the stack held before. It surfaced on the PRs rebased on
main today, where width_zero_is_a_noop threw "interval not found at level
4098".

The tag is now built on holder(mesh), so the field owns a copy of the
mesh and the helper is safe by construction, the way reconstruction()
already returns a field that outlives its local mesh. Returning the mesh
and building the tag in each test was considered and rejected: it keeps
the lifetime rule at every call site, and a one-liner passing a temporary
mesh would recreate the bug silently.
@gouarin
gouarin force-pushed the fix-graduation-test-dangling-mesh branch from 3057da4 to df5c9ac Compare September 2, 2026 12:06
@gouarin gouarin changed the title test(graduation): keep the mesh alive for the width tests' tag test(graduation): give the width tests' tag ownership of its mesh Sep 2, 2026
@gouarin
gouarin merged commit 7fa7237 into hpc-maths:main Sep 2, 2026
20 checks passed
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.

1 participant