Fix/cgo line directives - #282
Open
JBacchelli wants to merge 3 commits into
Open
Conversation
scip-go emitted one SCIP Document per *compiled* Go file, keyed by pkg.Fset.File(f.Package).Name(). For cgo packages that physical file is cgo's generated output under the build cache (foo.cgo1.go, _cgo_gotypes.go), so every occurrence in a cgo file -- including each `C.<fn>` call site -- was attributed to an ephemeral GOCACHE path instead of the user's real .go source. Occurrence *ranges* were already computed with Fset.Position(), which honors the //line directives cgo emits, so line/column were correct -- only the document they were attached to was wrong. Fix: key documents (and route each occurrence) by the //line-adjusted origin file, and drop occurrences that don't map to real source: - loader: request NeedFiles so pkg.GoFiles is populated. - visitors.OriginFile(pkg, pos): the cleaned, //line-adjusted path for a position. visitors.RealGoFiles(pkg): the package's on-disk source set. - VisitPackageSyntax / index.Index / ListMissing: key documents by OriginFile instead of the physical compiled path, and skip files whose origin is not real source (e.g. cgo's _cgo_gotypes.go glue). - fileVisitor: drop occurrences whose //line-adjusted position resolves to another file, or to a line/column outside the origin. cgo rewrites such as `defer C.f(x)` and inserted thunks like _cgoCheckPointer otherwise yield out-of-bounds ranges that downstream SCIP consumers reject. Non-cgo packages are unaffected (origin == physical file), so existing snapshots are unchanged. For cgo, `C.<fn>` references now resolve to the real <file>.go at the call site.
The previous commit attributed each document to its file's //line origin and dropped any occurrence that didn't fit that one file. That is safe for cgo and for all //line-free code, but a generated file whose //line points at a *different* real source file would lose those occurrences. Generalize it: accumulate occurrences on the document of each occurrence's own //line-resolved origin (via a shared path->document map), emitting one document per source file at the end. An occurrence is dropped only when its origin is not a real source document (cgo's _cgo_gotypes.go glue, a yacc .y, a build-cache path) or its range does not fit that source. - Paths are compared symlink-resolved (CleanResolve), so a module reached through a symlinked directory no longer drops real files as "not a GoFile". - InBounds now also rejects negative line/column (a "//line file:N" directive with no column collapses to column 0, i.e. scip -1) and reversed ranges, so routing can never emit a malformed occurrence. - Document owns occurrence/symbol accumulation, bounds checking, and ToScip rendering; the file visitor routes via targetDoc and attaches symbols in Finish. Verified: existing snapshots are byte-identical (no regression); protobuf/normal Go output is byte-identical to upstream; cgo Go->C refs still anchor to the real .go; a //line-to-another-real-.go file now routes there; malformed (negative column) occurrences are dropped rather than emitted. InBounds unit-tested.
cgo rewrites `C.puts` to `_Cfunc_puts` before the type checker sees it, and an
occurrence's range is sized from the identifier the checker reports. Once the
`//line` directives map the position back to the original source the start
column is right, but the range is five characters too wide and can run past the
end of the real line. Those occurrences were rejected as out of bounds even
though they were perfectly well located.
Give an out-of-bounds range one repair attempt before dropping it: measure the
identifier actually present at that column in the origin source and re-emit with
that width. The repair is only accepted if the result is in bounds, so it never
invents a location -- a synthesized position such as cgo's `defer C.f(x)`
wrapper, which lands at end-of-line where there is no identifier, finds nothing
to measure and is still dropped.
Measured on gravitational/teleport (2767 Go files, 12 cgo packages), same commit
and same toolchain, counting `_C[2]func_` references and the Go->C call edges
they resolve to:
unpatched scip-go 44 refs / 8 edges -- but every one anchored in
~/.cache/go-build, so unusable
before this commit 25 refs / 6 edges -- all on real source
after this commit 27 refs / 7 edges -- all on real source
The two recovered references are `C.gnu_get_libc_version` and
`C.resetInterruptSignalHandler`; the second adds a bridged edge in a package that
previously contributed none. Multi-line calls are covered too: `C.fill(cs,\n
&out,\n C.int(1))` is recovered with a range covering exactly `C.fill`.
Two known gaps remain, both distinct from what this fixes:
- `C.free` accounts for 16 of the still-missing teleport references. They sit
inside `defer C.free(...)`, whose rewritten wrapper reports end-of-line
positions with no identifier to measure. They are libc calls with no C
definition in the indexed surface, so they never produced edges regardless.
- One call site, teleport's 5-argument `C.mygetpwnam_r(...)` spanning several
lines with pointer arguments, is still absent. It is not rejected by the
bounds check -- no drop is recorded for it -- so it never reaches the
occurrence-emitting path at all, and the cause is elsewhere. Reproducible
with a `getpwnam_r`-style wrapper called with `&pwd` and an
`unsafe.Pointer`-derived cast.
A related wart this deliberately does not touch: a mangled range that happens to
stay in bounds is kept unrepaired and is therefore too wide -- `C.add` is emitted
covering `C.add(C.in`. Re-measuring every range would fix that, but package
references are sized from the import path rather than the source token
(emitImportReference), so a blanket re-measure would truncate `github.com/foo/bar`
to `github`. Fixing it properly needs the range builder to know whether the name
it was handed is the source spelling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Anchor cgo occurrences to real
.gosource via//linedirectivesProblem
scip-go attributes cgo occurrences to files that do not exist in the repository.
For a package that uses cgo, the go command compiles rewritten sources out of the
build cache (
$GOCACHE/.../foo.cgo1.go,_cgo_gotypes.go).Fset.File(pos).Name()returns those cache paths, so documents are emitted keyed by e.g.
../../../home/me/.cache/go-build/6c/6c45706…-d, and the real.gofile gets nodocument at all.
On
gravitational/teleport(2767 Go files, 12 cgo packages) this is total: 44 of44
_Cfunc_/_C2func_references — every Go→C call site in the repo — land inbuild-cache documents. Anything built on the index (cross-references, call graphs,
go-to-definition) points at paths the checkout does not contain.
Occurrence ranges were already
//line-adjusted; only document attribution wasphysical.
Fix
Key each document by its
//line-adjusted origin and route occurrencesper-occurrence rather than per-file:
visitors.OriginFileresolves a position throughFset.Position, so cgo's//linedirectives map it back to the real source;CleanResolveresolvessymlinks so origins stay comparable to
pkg.GoFiles.RealGoFilesgates which origins get a document. Occurrences whose origin is notreal source — compiler glue, a yacc
.y, a bare build-cache path — are droppedinstead of mis-attributed.
so
fileVisitorcarries the whole document map andtargetDocroutes eachoccurrence individually.
Document.InBoundsrejects ranges that escape the origin's source (negativecolumns from a column-less
//line, positions past EOF, reversed ranges) ratherthan emitting a location downstream SCIP consumers reject.
Document.RepairRangehandles the case where the range is only too wide: cgorewrites
C.putsto_Cfunc_putsbefore type checking, and range width comesfrom the identifier the checker reports, so a correctly-positioned occurrence is
emitted 5 characters too wide and can spill past end-of-line. An out-of-bounds
range gets one repair attempt that re-measures the identifier actually present at
that column, accepted only if the result is in bounds — so it never invents a
location.
Supporting changes:
loaderrequestspackages.NeedFilessopkg.GoFilesispopulated, and
RangeFromName/scipRangereturn ascip.Rangeinstead of a bare[]int32so ranges can be bounds-checked and rewritten as a value.Result
teleport, same commit and toolchain, counting
_C[2]func_references and the Go→Ccall edges they resolve to:
The raw reference count drops because occurrences that only ever existed in
generated glue are no longer emitted against fabricated paths. What remains is
anchored in real source:
lib/srv/uacc/uacc_linux.go,lib/shell/shell_unix.go,lib/inventory/metadata/metadata_linux.go,lib/system/signal.go.Known gaps
C.freeaccounts for 16 of the references not emitted on teleport. They sitinside
defer C.free(...), whose rewritten wrapper reports end-of-line positionswith no identifier to measure. All libc, so they resolve to no definition either
way.
C.mygetpwnam_r(...)withpointer arguments — is still absent. It is not rejected by the bounds check, so
the cause is elsewhere and is not addressed here.
too wide (
C.addemitted coveringC.add(C.in). Re-measuring unconditionallywould fix it, but package references are deliberately sized from the import path
rather than the source token (
emitImportReference), so a blanket re-measure wouldtruncate
github.com/foo/bartogithub. Fixing it properly needs the rangebuilder to know whether the name it was handed is the source spelling.
Testing
go test ./...passes; no snapshot goldens changed. New unit tests coverInBounds(well-formed, past-EOF, negative, reversed, unreadable-source) and
RepairRange(recovery, seven rejection cases, unreadable source, non-ASCII byte widths).
Validated end to end on teleport and navidrome by indexing each repo and checking
that every emitted cgo call site resolves inside the checkout.