Skip to content

Commit 1c7c7ea

Browse files
kakkoyungvisor-bot
authored andcommitted
fix(checklocks): enhance diagnostics and deduplication in failure reporting
This change aims to improve the clarity and accuracy of failure reporting in the analysis tool, reducing noise in diagnostics. Previous output: ```text -: return with unexpected locks held (locks: &({param:s}.mu) exclusively) -: return with unexpected locks held (locks: &(*(&({*ssa.Call:0x14003d9fe80}.trace)).mu) exclusively) -: return with unexpected locks held (locks: &(*(&({*ssa.Call:0x14003db0a00}.trace)).mu) exclusively) -: return with unexpected locks held (locks: &(*(&({*ssa.Call:0x14003db1600}[0].trace)).mu) exclusively) ``` Current output: ```text # gvisor.dev/gvisor/tools/checklocks/test test/basics.go:130:6: return with unexpected locks held (locks: &({param:tc}.mu) exclusively) test/methods.go:95:6: lock a.mu (&({param:a}.mu)) not held exclusively (locks: no locks held) ```` - Introduced a mechanism to track the current function during checks for improved diagnostics when SSA positions are missing. - Enhanced the `maybeFail` function to build a message string for deduplication, ensuring identical diagnostics are not reported multiple times for the same position key. - Updated the `passContext` struct to include a new `reported` map for tracking reported messages. Signed-off-by: Kemal Akkoyun <[email protected]> FUTURE_COPYBARA_INTEGRATE_REVIEW=#11741 from kakkoyun:existing_issues 3f8b63f PiperOrigin-RevId: 761287214
1 parent b77441b commit 1c7c7ea

File tree

3 files changed

+36
-4
lines changed

3 files changed

+36
-4
lines changed

tools/checklocks/analysis.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -777,6 +777,11 @@ func (pc *passContext) checkBasicBlock(fn *ssa.Function, block *ssa.BasicBlock,
777777

778778
// checkFunction checks a function invocation, typically starting with nil lockState.
779779
func (pc *passContext) checkFunction(call callCommon, fn *ssa.Function, lff *lockFunctionFacts, parent *lockState, force bool) {
780+
// Track current function for diagnostics when SSA positions are missing.
781+
prevFn := pc.curFn
782+
pc.curFn = fn
783+
defer func() { pc.curFn = prevFn }()
784+
780785
defer func() {
781786
// Mark this function as checked. This is used by the top-level
782787
// loop to ensure that all anonymous functions are scanned, if

tools/checklocks/annotations.go

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,17 +84,41 @@ func (pc *passContext) addForce(pos token.Pos) {
8484

8585
// maybeFail checks a potential failure against a specific failure map.
8686
func (pc *passContext) maybeFail(pos token.Pos, fmtStr string, args ...any) {
87-
if fd, ok := pc.failures[pc.positionKey(pos)]; ok {
87+
// Build the message string to allow deduplication.
88+
msg := fmt.Sprintf(fmtStr, args...)
89+
90+
origPos := pos
91+
reportPos := pos
92+
if !pos.IsValid() && pc.curFn != nil {
93+
reportPos = pc.curFn.Pos()
94+
}
95+
96+
key := pc.positionKey(origPos)
97+
98+
// Deduplicate: if we already reported this exact message for the same
99+
// position key, ignore subsequent identical diagnostics so they donʼt
100+
// affect failure-count logic.
101+
if seenMsgs, ok := pc.reported[key]; ok {
102+
if _, dup := seenMsgs[msg]; dup {
103+
return
104+
}
105+
} else {
106+
pc.reported[key] = make(map[string]struct{})
107+
}
108+
109+
pc.reported[key][msg] = struct{}{}
110+
111+
if fd, ok := pc.failures[key]; ok {
88112
fd.seen++
89113
return
90114
}
91-
if _, ok := pc.exemptions[pc.positionKey(pos)]; ok {
115+
if _, ok := pc.exemptions[key]; ok {
92116
return // Ignored, not counted.
93117
}
94-
if !enableWrappers && !pos.IsValid() {
118+
if !enableWrappers && !origPos.IsValid() {
95119
return // Ignored, implicit.
96120
}
97-
pc.pass.Reportf(pos, fmtStr, args...)
121+
pc.pass.Reportf(reportPos, fmtStr, args...)
98122
}
99123

100124
// checkFailure checks for the expected failure counts.

tools/checklocks/checklocks.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,9 @@ type passContext struct {
7373
failures map[positionKey]*failData
7474
exemptions map[positionKey]struct{}
7575
forced map[positionKey]struct{}
76+
reported map[positionKey]map[string]struct{}
7677
functions map[*ssa.Function]struct{}
78+
curFn *ssa.Function
7779
observations map[types.Object]*objectObservations
7880
}
7981

@@ -143,6 +145,7 @@ func run(pass *analysis.Pass) (any, error) {
143145
exemptions: make(map[positionKey]struct{}),
144146
forced: make(map[positionKey]struct{}),
145147
functions: make(map[*ssa.Function]struct{}),
148+
reported: make(map[positionKey]map[string]struct{}),
146149
}
147150

148151
// Find all line failure annotations.

0 commit comments

Comments
 (0)