Skip to content

fix(instrument): SelectorExpr in IncDecStmt now instrumented (#30)#31

Merged
kolkov merged 3 commits into
mainfrom
fix/issue-30-selectorexpr
Dec 19, 2025
Merged

fix(instrument): SelectorExpr in IncDecStmt now instrumented (#30)#31
kolkov merged 3 commits into
mainfrom
fix/issue-30-selectorexpr

Conversation

@kolkov

@kolkov kolkov commented Dec 19, 2025

Copy link
Copy Markdown
Owner

Summary

Fixes #30: s.x++ was not being instrumented because shouldInstrument() skips all SelectorExpr to avoid method calls/package functions.

Root cause: In visitSelectorInModifyContext(), we recorded Node: expr (SelectorExpr), but findParentStatement() then returned the outermost containing statement (BlockStmt) instead of the IncDecStmt. This caused the instrumentation lookup to fail during code generation.

Solution: Pass the parent statement (IncDecStmt) to visitSelectorInModifyContext() and use it as Node. This ensures findParentStatement() returns the correct statement for insertion.

Changes

  • Modified visitSelectorInModifyContext(parentStmt, expr) to accept parent statement
  • Use parentStmt as Node so findParentStatement() returns correct statement
  • Added TestInstrumentFile_SelectorExprIncDec test case
  • Added TestInstrumentFile_SelectorExprInGoroutine for full reproduction
  • Updated CHANGELOG.md for v0.8.3

Before/After

Before fix:

func update(s *S) {
    s.x++  // No race detection!
}

After fix:

func update(s *S) {
    race.RaceRead(uintptr(unsafe.Pointer(&s.x)))
    race.RaceWrite(uintptr(unsafe.Pointer(&s.x)))
    s.x++
}

Test plan

  • TestInstrumentFile_SelectorExprIncDec passes
  • TestInstrumentFile_SelectorExprInGoroutine passes
  • All existing tests pass
  • pre-release-check.sh passes with 0 errors
  • Coverage: 85.7%

Acknowledgments

Thanks to @thepudds for finding this critical bug in Issue #27 comments.

Fixed Issue #30: s.x++ was not instrumented because shouldInstrument()
skips all SelectorExpr to avoid method calls/package functions.

Root cause: In visitSelectorInModifyContext(), we recorded Node as
the SelectorExpr itself. But findParentStatement() then returned the
outermost containing statement (BlockStmt), not the IncDecStmt.
This caused stmtToPoints lookup to fail during instrumentation.

Solution: Pass the parent statement (IncDecStmt) to
visitSelectorInModifyContext() and use it as Node. This ensures
findParentStatement() returns the correct statement for insertion.

Technical details:
- visitSelectorInModifyContext(parentStmt, expr) now takes parent stmt
- Node field uses parentStmt instead of SelectorExpr
- findParentStatement() returns statement directly (already a Stmt)

Example (before):
  func update(s *S) { s.x++ }  // No instrumentation!

Example (after):
  func update(s *S) {
      race.RaceRead(uintptr(unsafe.Pointer(&s.x)))
      race.RaceWrite(uintptr(unsafe.Pointer(&s.x)))
      s.x++
  }

Tests: Added TestInstrumentFile_SelectorExprIncDec and
TestInstrumentFile_SelectorExprInGoroutine for Issue #30 coverage.

Reported-by: @thepudds
Fixes: #30
- Add v0.8.3 entry for Issue #30 fix (SelectorExpr in IncDecStmt)
- Document known limitation with compiler directives (Issue #31)
- Credit @thepudds for finding the bug
@codecov

codecov Bot commented Dec 19, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.67%. Comparing base (d8f3aba) to head (76a746e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   69.45%   69.67%   +0.22%     
==========================================
  Files          28       28              
  Lines        2933     2955      +22     
==========================================
+ Hits         2037     2059      +22     
  Misses        777      777              
  Partials      119      119              
Flag Coverage Δ
unittests 69.67% <100.00%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cmd/racedetector/instrument/visitor.go 74.47% <100.00%> (+1.24%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8f3aba...76a746e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Update version to v0.8.3 (STABLE)
- Add v0.8.2 section (Issue #27: *ptr++ fix by @thepudds)
- Add v0.8.3 section (Issue #30: s.x++ fix by @thepudds)
- Document known limitation with compiler directives (Issue #31)
- Update version history with all v0.8.x releases
- Credit @thepudds for community contributions
@kolkov kolkov merged commit 962743f into main Dec 19, 2025
14 checks passed
@kolkov kolkov deleted the fix/issue-30-selectorexpr branch December 19, 2025 19:04
kolkov added a commit that referenced this pull request Dec 19, 2025
PR #31 was the merged PR, Issue #32 is the compiler directives bug.
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.

bug(escape): Struct field access via pointer receiver not instrumented (s.x++)

1 participant