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
Comments
short read on string table
when merging coverage counters
(attn @thanm; CC @golang/compiler) |
Thanks, I will take a look. |
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 |
Change https://go.dev/cl/466677 mentions this issue: |
@gopherbot please backport to 1.20 |
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. |
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. |
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>
Change https://go.dev/cl/468536 mentions this issue: |
…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>
…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>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat 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?
The tarball of generated coverage directory are attached, it can reproduced stably, both in CI(linux) and laptop(macOS).
coverage.tar.gz
The text was updated successfully, but these errors were encountered: