Skip to content

Fix off-by-one in Select instruction ref tracking#2704

Merged
sbc100 merged 2 commits into
WebAssembly:mainfrom
sumleo:fix/select-ref-tracking-off-by-one
Feb 26, 2026
Merged

Fix off-by-one in Select instruction ref tracking#2704
sbc100 merged 2 commits into
WebAssembly:mainfrom
sumleo:fix/select-ref-tracking-off-by-one

Conversation

@sumleo

@sumleo sumleo commented Feb 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The Select instruction's ref tracking compares against wrong stack index after popping the condition value, causing reference operands to lose GC tracking.
  • Fix adjusts both ref checks to use values_.size() - 1 instead of values_.size().

Details

In the Select case (interp.cc), Pop<u32>() removes the condition from the value stack before the ref checks run. At that point, values_.size() is the index one past false_val, not the index of false_val itself. Similarly, after popping false_, values_.size() is one past true_val.

The refs_ vector stores indices of reference-typed values on the stack. Comparing refs_.back() against values_.size() (instead of values_.size() - 1) means neither operand is detected as a ref. The result is that when both select operands are references, the pushed result loses its ref tracking, which can cause premature GC collection.

The fix changes both comparisons from values_.size() to values_.size() - 1 to correctly identify the top-of-stack value.

After Pop<u32>() removes the condition value, values_.size() has
already decremented. The ref check compares refs_.back() against
values_.size(), but at that point values_.size() is one past the
index of the value being checked. This causes neither select operand
to be detected as a ref, losing GC tracking for the result.
@zherczeg

Copy link
Copy Markdown
Collaborator

The patch looks good. Thank you for finding these and fixing them.
I think adding a test would be a good idea since an early collection should have visible side effects.

Exercise typed select with funcref values to verify reference tracking
works correctly. Tests cover selecting between two non-null funcrefs,
selecting between non-null and null refs, and storing the select result
in a global.
@sumleo

sumleo commented Feb 26, 2026

Copy link
Copy Markdown
Contributor Author

Added a test in test/interp/select-ref.txt that exercises typed select with funcref operands. It covers:

  • Selecting between two non-null funcrefs (true and false branches)
  • Selecting between a non-null ref and a null ref
  • Storing the select result into a global

Note that a GC-level test (verifying early collection has visible side effects) would ideally need low-level Thread control to trigger Store::Collect() mid-execution, which the existing test infrastructure doesn't support from WAT — there's a TODO about this at test-interp.cc:734. The WAT-level test verifies correct behavior with reference operands, which is the practical regression test for this fix.

@sbc100 sbc100 merged commit 9d5cf7c into WebAssembly:main Feb 26, 2026
17 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.

3 participants