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: incomplete coverage when using goto #16624

Closed
cbandy opened this issue Aug 6, 2016 · 4 comments
Closed

cmd/cover: incomplete coverage when using goto #16624

cbandy opened this issue Aug 6, 2016 · 4 comments
Assignees
Milestone

Comments

@cbandy
Copy link

@cbandy cbandy commented Aug 6, 2016

In go version go1.6.3 linux/amd64, the following code produces an incomplete cover profile.

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/cbandy/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"
$ cat -n main.go 
     1  package main
     2
     3  func test(s string) (result int) {
     4          if s == "" {
     5                  goto One
     6          }
     7          goto Two
     8
     9  One:
    10          result = 1
    11          return
    12
    13  Two:
    14          result = 2
    15          return
    16  }
$ cat main_test.go 
package main

import "testing"

func Test(t *testing.T) {
        if x := test(""); x != 1 {
                t.Errorf("Expected 1, got %v", x)
        }
        if x := test("a"); x != 2 {
                t.Errorf("Expected 2, got %v", x)
        }
}

Both 1 and 2 are successfully returned, so I expected to see 100% coverage. However, neither the labels nor return statements are accounted for:

$ go test -coverprofile coverage.out
PASS
coverage: 42.9% of statements
ok      cover   0.003s
$ cat coverage.out
mode: set
cover/main.go:3.34,4.13 1 1
cover/main.go:7.2,7.10 1 1
cover/main.go:9.1,15.8 4 0
cover/main.go:4.13,5.11 1 1
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 6, 2016

/cc @robpike

@bradfitz bradfitz changed the title Incomplete coverage when using goto cmd/cover: incomplete coverage when using goto Aug 6, 2016
@robpike robpike self-assigned this Aug 6, 2016
@robpike
Copy link
Contributor

@robpike robpike commented Aug 6, 2016

Nice. I forgot about goto statements. Should be easy to fix.

@quentinmit quentinmit added this to the Go1.8 milestone Aug 8, 2016
@jscrockett01
Copy link

@jscrockett01 jscrockett01 commented Sep 8, 2016

I looked into this for a while because it was biting me as well. Not being familiar with the code I wasn't able to create a quick fix but here's what I learned.

The shortcoming is in how labeled statements are handled. The logic is all there for statements that can be departure points for control flow (i.e., goto statements are not the problem), but the logic is missing for statements that can be arrival points for control flow (i.e., labeled statements are the problem).

The problem can be understood by considering a labeled statement that can itself affect control flow:

L:
    flowChangingStmt

The instrumentation must insert a counter between the label and the statement, but this is delicate surgery because in the ast representation the label contains the statement. The label could be removed from the flowChangingStmt and attached to an immediately preceding new counter statement. However, labeled 'for','switch',or 'select' statements must not have their label removed. It may be that the 'for', 'switch', and 'select' statements are not harmed by the lack of a counter between the label and the statement, so inserting the counter and re-attaching the label may only be necessary for non-'for','switch','select' statements.

I hope this helps. Unfortunately I don't have time to dig deeper. An example file and its as-presently-created cover rewrite are shown below.

package main

func f(x int) int {
    x++
    x++
    x++
    x++
L0:
    x++
    x++
    x++
    x++
    goto L1
    x++
    x++
    x++
    x++
L1:
    x++
    x++
    x++
    x++
    return x
    x++
    x++
    x++
    x++
}
package main

func f(x int) int {
    GoCover.Count[0] = 1
    x++
    x++
    x++
    x++
L0:
    x++
    x++
    x++
    x++
    goto L1
    GoCover.Count[1] = 1
    x++
    x++
    x++
    x++
L1:
    x++
    x++
    x++
    x++
    return x
    x++
    x++
    x++
    x++
}

var GoCover = struct {
    Count     [2]uint32
    Pos       [3 * 2]uint32
    NumStmt   [2]uint16
} {
    Pos: [3 * 2]uint32{
        3, 13, 0x90013, // [0]
        14, 27, 0x50002, // [1]
    },
    NumStmt: [2]uint16{
        9, // 0
        13, // 1
    },
}
@quentinmit quentinmit added the NeedsFix label Oct 6, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 13, 2016

CL https://golang.org/cl/30977 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.