Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/cover: cgo produces incorrect coverage profiles #9479

Closed
jonlawlor opened this issue Dec 31, 2014 · 8 comments
Closed

cmd/cover: cgo produces incorrect coverage profiles #9479

jonlawlor opened this issue Dec 31, 2014 · 8 comments
Milestone

Comments

@jonlawlor
Copy link

What version of Go are you using (go version)?
go1.4 darwin/amd64

What operating system and processor architecture are you using?
OSX Yosemite (10.10.1) x86_64

What did you do?
On OSX:

export CGO_LDFLAGS="-framework Accelerate"
go get -u -t github.com/gonum/blas
go test -coverprofile=coverage.out github.com/gonum/blas/cblas/
go tool cover -html=coverage.out

These commands would be different on other OSes, because the Accelerate framework is not available on them. Alternate installation instructions are available on the github.com/gonum/blas README.md. The important thing is that I'm running the cover tool's html option with a package that uses cgo.

What did you expect to see?
I expected 16% of the lines of code to show green, consistent with the test tool's coverage report.

What did you see instead?
Only a 3 lines were highlighted green, and there were hundreds of lines colored red.
screen shot 2014-12-30 at 10 29 04 pm

The issue #6333 seems to be related. That one appears to be focused on the command line tool, which is working correctly in this case (I think). The html report is not.

@mikioh mikioh changed the title go tool cover -html=... has incorrect highlighting cmd/cover: -html=... has incorrect highlighting Dec 31, 2014
@adg
Copy link
Contributor

adg commented Jan 5, 2015

Are you sure you're running the latest version of the cover binary?

go get -u golang.org/x/tools/cmd/cover

@jonlawlor
Copy link
Author

I double checked by removing everything and go get-ing everything again. This produces the same result:

jonlawlor: ~/go/src/github.com/gonum/blas $ go get -u golang.org/x/tools/cmd/cover
jonlawlor: ~/go/src/github.com/gonum/blas $ go version
go version go1.4 darwin/amd64
jonlawlor: ~/go/src/github.com/gonum/blas $ export CGO_LDFLAGS="-framework Accelerate"
jonlawlor: ~/go/src/github.com/gonum/blas $ go get -u -t github.com/gonum/blas
jonlawlor: ~/go/src/github.com/gonum/blas $ go test -coverprofile=coverage.out github.com/gonum/blas/cblas/
--- FAIL: TestIdamax (0.00s)
    level1double.go:1155: idamax: mismatch NaN: expected 0, found 1
    level1double.go:1155: idamax: mismatch NaNInc: expected 0, found 1
FAIL
coverage: 16.0% of statements
FAIL    github.com/gonum/blas/cblas 0.207s
jonlawlor: ~/go/src/github.com/gonum/blas $ go tool cover -html=coverage.out

(except that there was some work in gonum/blas which makes it fail a test)

I really have tried to eliminate any PBCAK, but if I've missed something I apologize.

@robpike
Copy link
Contributor

robpike commented Jan 7, 2015

The problem here is that the line and column numbers in the cover profile refer to the cgo-rewritten file, but the file named is the original Go source file. For this particular case, a profile line refers to column 157 of line 91 of blas.go, whereas the actual data for that counter comes from 157 of line 91 of blas.cgo1.go. Line 91 of blas.go has only 122 characters; the cgo rewrite lengthens the line.

When the HTML processor for the cover tool reads, as instructed, blas.go, line 91 has only 122 characters but the block termination is written to happen at column 157. The tool decides the block never terminates, and the HTML output is compromised.

We are fixing the cover tool to terminate the block at the end of the line so the rest of the file will work. This is a simple fix, but not fully correct: the column numbers for blocks that start or stop on all rewritten lines will be incorrect. They always were, for cgo files.

A real fix will require much more complex computation to refer the column numbers back to the original Go source file, which is the one that the HMTL annotation refers to. This is not impossible, but it's close to infeasible.

We will leave this bug open to record this possibility, but don't expect it to be fixed. The updated cover tool will at least not fail so badly. Only the column numbers will sometimes be wrong. Lines will always be right.

adg added a commit to golang/tools that referenced this issue Jan 7, 2015
When cover is used with cgo packages, the coverage profile is produced
from the cgo-generated Go source files, so the profile will reference
line and column numbers from that generated source, even though it names
the original file.
This is okay in general because the cgo-generated Go source files are
very similar to the original, with one significant exception:
the original source may refer to C identifiers such as C.foo(), but the
cgo tool generates source that rewrites these mentions to something like
_Cfunc_foo.
This means that column numbers in coverage profiles might be higher than
they should be, so be lenient when interpreting them.

Update golang/go#9479

Change-Id: Ic3abef07471614101ce0c686d35b85e7e5e6a777
Reviewed-on: https://go-review.googlesource.com/2410
Reviewed-by: Rob Pike <r@golang.org>
@adg adg changed the title cmd/cover: -html=... has incorrect highlighting cmd/cover: cgo produces incorrect coverage profiles Jan 7, 2015
@robpike
Copy link
Contributor

robpike commented Jan 7, 2015

It occurs to me that a feasible solution is to run the cover analysis on two files, the original and the cgo output. Rewrite the cgo output file as usual, but append the position data from the scan of the original. To work without bringing the diff algorithm in, which I would like to avoid, it would require that all cgo rewriting is within a line, that is, that it does not add or delete any lines. It does this today (otherwise coverage wouldn't work at all), but I am a little nervous to add this requirement.

@mattn
Copy link
Member

mattn commented Jan 7, 2015

It seems working good to me.

http://bl.ocks.org/mattn/raw/7d41ecb1c25c09d9a198/

@mattn
Copy link
Member

mattn commented Jan 7, 2015

BTW, OT, the file path should be ToSlash-ed?

@adg
Copy link
Contributor

adg commented Jan 7, 2015

@mattn if it works well for you then you're just lucky. It only fails under certain circumstances.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/cover: cgo produces incorrect coverage profiles x/tools/cmd/cover: cgo produces incorrect coverage profiles Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the repo-tools label Apr 14, 2015
@rsc rsc changed the title x/tools/cmd/cover: cgo produces incorrect coverage profiles cmd/cover: cgo produces incorrect coverage profiles Nov 10, 2017
@rsc rsc modified the milestones: Unreleased, Go1.11 Nov 10, 2017
@gopherbot
Copy link

Change https://golang.org/cl/77155 mentions this issue: cmd/go: run cover before cgo

@golang golang locked and limited conversation to collaborators Nov 16, 2018
@rsc rsc unassigned adg Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants