go-fuzz-build: use go/packages #211
Conversation
|
This has not been tested on Windows yet. (I'm working on it, but my access to Windows machines is very limited. I'd love help testing on Windows, if anyone wants to chip in.) But I wanted to get this up to start the review, since it is a very large change. |
josharian
force-pushed
the
go-packages
branch
from
d5c1687
to
00fa60b
Compare
golang.org/x/tools/go/packages is a new package designed to aid in locating, parsing, and typechecking Go packages. This change updates go-fuzz-build to use it. This effectively resulted in a rewrite of go-fuzz-build/main.go. Some benefits are: * Improved performance. go-fuzz-build appears to be 2x faster on first build, and 4x faster on subsequent builds. This is due to several factors. One big one is the use of cached type information. And there are more performance improvements available; see the TODOs in the code. * More robust. Previously, the list of ignored packages had to be updated every time the standard library changed. Now the ignored packages are computed on the fly. * More user-friendly. You can now invoke go-fuzz-build without a package path; it assumes ".", and relative paths work seamlessly. * Closer to functionality in a module-enabled world. See the TODOs in the code. * Simpler and better documented. Outstanding issue: * cgo code is not instrumented. See the discussion and linked Go toolchain issue in the code. Implementation notes: * go-fuzz-defs has been split into two. The constants, which are shared across go-fuzz, go-fuzz-build, and instrumented code, have been left in place. The types, which were only needed by go-fuzz and go-fuzz-build, have been moved to a new internal package. This allowed us to break the go-fuzz-dep/go-fuzz-defs dependency and std-depends-on-non-std problems in a simpler way: We simply make another copy of go-fuzz-defs. See the comments in the code for details. * There is now a shared AST across the coverage and sonar passes. This works out OK, since the sonar pass used to request coverage first. Now we just have to be careful about the order in which we use and mutate the AST. This is well documented in the code.
josharian
force-pushed
the
go-packages
branch
from
00fa60b
to
f0f010c
Compare
|
Basic Windows testing done; one bug found and fixed. I don't plan to do further Windows testing at this point, although further help and bug reports are of course welcome. |
This makes go-fuzz a little easier to use; one fewer flag to set.
This is what go-fuzz-corpus uses, and I have used elsewhere. One fewer required flag to set when using go-fuzz.
$ go-fuzz-build && go-fuzz Yay!
|
I've added on a few bonus commits. :) And now I'll stop messing with this until I have some review feedback. |
my favorite feature
this is not a regression, right?
this was annoying |
go-fuzz-build/main.go
Outdated
| // It's a non-trivial part of build time. | ||
| // Question: Do it here or in copyDir? | ||
|
|
||
| // TODO: consider using hard links for |
There was a problem hiding this comment.
I think this can't work across mounts, and /tmp is frequently a different mount.
There was a problem hiding this comment.
Ack. Will remove that comment in a follow-up.
go-fuzz-build/main.go
Outdated
| do := func(fn func()) { | ||
| wg.Add(1) | ||
| sem <- 0 | ||
| fn() |
There was a problem hiding this comment.
Wait, shouldn't this include go somewhere? ;)
There was a problem hiding this comment.
Ouch. :P That's what I get for refactoring while benchmarking.
I just re-benchmarked with higher N (with go!) and can't reproduce the speedup. I'll force push this branch to remove this commit, at least for now.
|
I read the code and I don't have any particular comments. |
Right. I just had high hopes that go/packages would help here. |
Dmitry pointed out during code review that hard links won't work across mounts, and that /tmp is often a mount, so it probably isn't worth the code to try and then fall back.
|
Force push replaced my broken concurrent-I/O commit and replaced it with a hard links comment tweak. Thanks for the speedy review, Dmitry. |
golang.org/x/tools/go/packages is a new package designed to aid in
locating, parsing, and typechecking Go packages.
This change updates go-fuzz-build to use it.
This effectively resulted in a rewrite of go-fuzz-build/main.go.
Some benefits are:
Improved performance. go-fuzz-build appears to be 2x faster on
first build, and 4x faster on subsequent builds.
This is due to several factors. One big one is the use of cached
type information.
And there are more performance improvements available;
see the TODOs in the code.
More robust. Previously, the list of ignored packages had to be
updated every time the standard library changed.
Now the ignored packages are computed on the fly.
More user-friendly. You can now invoke go-fuzz-build without
a package path; it assumes ".", and relative paths work seamlessly.
Closer to functionality in a module-enabled world.
See the TODOs in the code.
Simpler and better documented.
Outstanding issue:
See the discussion and linked Go toolchain issue in the code.
Implementation notes:
go-fuzz-defs has been split into two.
The constants, which are shared across go-fuzz, go-fuzz-build,
and instrumented code, have been left in place.
The types, which were only needed by go-fuzz and go-fuzz-build,
have been moved to a new internal package.
This allowed us to break the go-fuzz-dep/go-fuzz-defs dependency
and std-depends-on-non-std problems in a simpler way:
We simply make another copy of go-fuzz-defs.
See the comments in the code for details.
There is now a shared AST across the coverage and sonar passes.
This works out OK, since the sonar pass used to request coverage first.
Now we just have to be careful about the order in which we use and
mutate the AST. This is well documented in the code.