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/covdata: short read on string table when merging coverage counters #58411

Closed
yihuang opened this issue Feb 8, 2023 · 8 comments
Closed
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@yihuang
Copy link

yihuang commented Feb 8, 2023

What version of Go are you using (go version)?

$ go version
go version go1.20 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/yihuang/Library/Caches/go-build"
GOENV="/Users/yihuang/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/yihuang/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/yihuang/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/nix/store/0m3v5yn7s5ipvzp7p462pvjknfcn8c6s-go-1.20/share/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/nix/store/0m3v5yn7s5ipvzp7p462pvjknfcn8c6s-go-1.20/share/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/yihuang/src/cronos/go.mod"
GOWORK=""
CGO_CFLAGS="-I/nix/store/qg6n4gdcccfd93lnih1yh8a894ya6akr-rocksdb-7.9.2/include"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-L/nix/store/qg6n4gdcccfd93lnih1yh8a894ya6akr-rocksdb-7.9.2/lib -lrocksdb"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3_/dt3cb8952_12y960nh8fhqx40000gn/T/go-build3007670481=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Use -cover to collect coverage report in integration tests.

What did you expect to see?

go tool covdata should be able to process generated file succesfully.

What did you see instead?

$ go tool covdata merge -i integration_tests/coverage -o profile.txt
error: error: reading counter data file integration_tests/coverage/covcounters.627546bfc793cab3f06764545dba1bd1.91113.1675846217322297000: error: short read on string table

The tarball of generated coverage directory are attached, it can reproduced stably, both in CI(linux) and laptop(macOS).

coverage.tar.gz

@bcmills bcmills changed the title cmd/go: cmd/covdata: short read on string table when merging coverage counters Feb 8, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 8, 2023
@bcmills bcmills added this to the Backlog milestone Feb 8, 2023
@bcmills
Copy link
Contributor

bcmills commented Feb 8, 2023

(attn @thanm; CC @golang/compiler)

@bcmills bcmills added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 8, 2023
@thanm thanm self-assigned this Feb 8, 2023
@thanm
Copy link
Contributor

thanm commented Feb 8, 2023

Thanks, I will take a look.

@thanm thanm modified the milestones: Backlog, Go1.20.1 Feb 8, 2023
@thanm
Copy link
Contributor

thanm commented Feb 8, 2023

Thank you for sending this report, it is an interesting bug in the coverage profile reader, specifically the portion that reads counter data files. It appears that the bug is only tickled when you have a small counter data file (large ones will be mmap'd) that has a string table larged than the bufio.Reader 4k buffer size (which might explain why I didn't hit it previously in my testing).

Sending a CL with a fix shortly. I will also ask the release team if the fix can be back-ported to Go 1.20.1

@thanm thanm added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 8, 2023
@gopherbot
Copy link

Change https://go.dev/cl/466677 mentions this issue: cmd/internal/cov: fix misuse of bufio.Reader.Read in read helper

@thanm
Copy link
Contributor

thanm commented Feb 9, 2023

@gopherbot please backport to 1.20

@gopherbot
Copy link

Backport issue(s) opened: #58427 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@thanm
Copy link
Contributor

thanm commented Feb 10, 2023

I have started the process to get this approved for backport to 1.20, however I think it will have to wait until the March minor release (too late for the Feb cycle), just FYI.

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
Fix a misuse of bufio.Reader.Read in the helper class
cmd/internal/cov.MReader; the MReader method in question should have
been using io.ReadFull (passing the bufio.Reader) instead of directly
calling Read.

Using the Read method instead of io.ReadFull will result in a "short"
read when processing a specific subset of counter data files, e.g.
those that are short enough to not trigger the mmap-based scheme we
use for larger files, but also with a large args section (something
large enough to exceed the default 4k buffer size used by
bufio.Reader).

Along the way, add some additional defered Close() calls for files
opened by the CovDataReader.visitPod, to enure we don't leave any open
file descriptor following a call to CovDataReader.Visit.

Fixes golang#58411.

Change-Id: Iea48dc25c0081be1ade29f3a633df02a681fd941
Reviewed-on: https://go-review.googlesource.com/c/go/+/466677
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/468536 mentions this issue: cmd/internal/cov: fix misuse of bufio.Reader.Read in read helper

gopherbot pushed a commit that referenced this issue Feb 22, 2023
…Read in read helper

Fix a misuse of bufio.Reader.Read in the helper class
cmd/internal/cov.MReader; the MReader method in question should have
been using io.ReadFull (passing the bufio.Reader) instead of directly
calling Read.

Using the Read method instead of io.ReadFull will result in a "short"
read when processing a specific subset of counter data files, e.g.
those that are short enough to not trigger the mmap-based scheme we
use for larger files, but also with a large args section (something
large enough to exceed the default 4k buffer size used by
bufio.Reader).

Along the way, add some additional defered Close() calls for files
opened by the CovDataReader.visitPod, to enure we don't leave any open
file descriptor following a call to CovDataReader.Visit.

Fixes #58427.
Updates #58411.

Change-Id: Iea48dc25c0081be1ade29f3a633df02a681fd941
Reviewed-on: https://go-review.googlesource.com/c/go/+/466677
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit a7fe9ad)
Reviewed-on: https://go-review.googlesource.com/c/go/+/468536
Reviewed-by: Cherry Mui <cherryyz@google.com>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Mar 3, 2023
…Read in read helper

Fix a misuse of bufio.Reader.Read in the helper class
cmd/internal/cov.MReader; the MReader method in question should have
been using io.ReadFull (passing the bufio.Reader) instead of directly
calling Read.

Using the Read method instead of io.ReadFull will result in a "short"
read when processing a specific subset of counter data files, e.g.
those that are short enough to not trigger the mmap-based scheme we
use for larger files, but also with a large args section (something
large enough to exceed the default 4k buffer size used by
bufio.Reader).

Along the way, add some additional defered Close() calls for files
opened by the CovDataReader.visitPod, to enure we don't leave any open
file descriptor following a call to CovDataReader.Visit.

Fixes golang#58427.
Updates golang#58411.

Change-Id: Iea48dc25c0081be1ade29f3a633df02a681fd941
Reviewed-on: https://go-review.googlesource.com/c/go/+/466677
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit a7fe9ad)
Reviewed-on: https://go-review.googlesource.com/c/go/+/468536
Reviewed-by: Cherry Mui <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants