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

failures running on github.com/tailscale/tailscale #3

Closed
josharian opened this issue Dec 17, 2021 · 9 comments
Closed

failures running on github.com/tailscale/tailscale #3

josharian opened this issue Dec 17, 2021 · 9 comments

Comments

@josharian
Copy link

github.com/tailscale/tailscale at 5a9914a92fa72d61510b5f2aacd6665e3f8fa659

Using Go 1.18:

$ fzgen ./...
2021/12/17 10:34:42 internal error: package "errors" without types was imported from "tailscale.com/util/pidowner"

Using the Tailscale Go fork (based on 1.17):

$ fzgen ./...
panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
github.com/thepudds/fzgen/gen.avoidCollision(0x14003ef2730, 0x0, 0x14003cf4cd0, {0x14001cc5800, 0x2, 0x2})
	/Users/josh/pkg/mod/github.com/thepudds/fzgen@v0.4.1/gen/genfuncs.go:634 +0x348
github.com/thepudds/fzgen/gen.emitIndependentWrapper(0x140070a5d28, {{0x140001e6e88, 0x11}, {0x14000a680bc, 0x3}, {0x14000a33e18, 0x15}, {0x140062b7500, 0x19}, 0x14003ed6410}, ...)
	/Users/josh/pkg/mod/github.com/thepudds/fzgen@v0.4.1/gen/genfuncs.go:207 +0x690
github.com/thepudds/fzgen/gen.emitIndependentWrappers({0x16d6837f6, 0x5}, {0x14006310000, 0x449, 0x471}, {0x1, 0x1, {0x1029a9e23, 0x4}, 0x0})
	/Users/josh/pkg/mod/github.com/thepudds/fzgen@v0.4.1/gen/genfuncs.go:86 +0x4c0
github.com/thepudds/fzgen/gen.FzgenMain()
	/Users/josh/pkg/mod/github.com/thepudds/fzgen@v0.4.1/gen/fzgen.go:130 +0x56c
main.main()
	/Users/josh/pkg/mod/github.com/thepudds/fzgen@v0.4.1/cmd/fzgen/main.go:10 +0x20

I'm not fussed about this, just reporting it in case it is useful.

@thepudds
Copy link
Owner

Hi @josharian, thanks for the report, this is very helpful.

One thing is since taking the project off pause, I have only been running against a single package at a time, and only recently remembered that it historically supported an actual package pattern like ./.... 😅

So one thing to try would be just one package at a time. I will look into either making it more robust with a package pattern, or explicitly disallow them with a clear error.

Regarding this error:

internal error: package "errors" without types was imported from ...

That might be due to a mismatch between the toolchain and go/packages? I commented here recently on a related older issue:

golang/go#37617 (comment)

I will look at that as well.

Thanks for the report!!

@josharian
Copy link
Author

This works a bit better:

$ find . -type d | xargs -n 1 fzgen

...except that it drops autofuzz_test.go in the pwd, where they all overwrite each other. It'd be useful for autofuzz_test.go to always be written in the package dir.

(The index out of range [0] with length 0 panic still reproduces this way, too.)

@josharian
Copy link
Author

Oh, and go list ./... | xargs works even better than find . -type d | xargs. :P

@thepudds
Copy link
Owner

Hi @josharian if you are not offended by ugly bash one-liners, this seems to work when run from the root of the tailscale repo (or at least, I started it and it's still running).

for x in $(go list ./...); do echo $x; start=$(pwd); cd $(echo $x | sed -r 's/tailscale.com/./') ; fzgen && gotip test . || echo "--- FAILED $x ---"; cd $start; done &> out.txt &

I'll look at the results when it is finished.

That's slightly tweaked from this one-liner that I was using to stress test against the stdlib:

https://github.com/thepudds/fzgen/blob/main/gen/fzgen.go#L30-L31

All that said, it would of course be better to actually properly support package patterns, which I'll look at fixing.

@thepudds
Copy link
Owner

thepudds commented Dec 17, 2021

Also, by default fzgen only targets exported methods/functions, so that one-liner is complaining about several packages that I think don't have anything exported, at least based on brief spot checking.

fzgen supports an -unexported flag, but probably best to start with just exported...

@thepudds
Copy link
Owner

thepudds commented Jan 3, 2022

Hi @josharian, FYI, aside from the internal error: package "errors" without types was imported from ... error, I think this is hopefully addressed at this point. This succeeds:

Setup (using fzgen @main, and tailscale dd45bba):

go install github.com/thepudds/fzgen/cmd/fzgen@main
go install golang.org/dl/gotip@latest
gotip download
git clone https://github.com/tailscale/tailscale tailscale

Auto-generate fuzz files, and confirm they at least compile with gotip (go1.18-c886143 Sun Jan 2 14:27:43):

cd tailscale
fzgen ./...
fzgen -chain ./...
gotip get github.com/thepudds/fzgen/fuzzer@main
gotip mod tidy
gotip test -exec=true ./...

Regarding:

It'd be useful for autofuzz_test.go to always be written in the package dir.

That is a good suggestion, and that is now the default behavior when targeting multiple packages at the same time (e.g., if you are a repo owner, and want to do 'fzgen ./...' from repo root).

However, I'd still like to make it easy for cursory "passerby fuzzing", where the target might be a dependency or a dependency you are evaluating, and it might only be in your read-only module cache, or the current "hello world" first example in the README of fzgen encoding/ascii85, which works from an empty directory without updating your copy of the stdlib. So for now, if there is only a single package targeted, it still places the autofuzz_test.go by default in the current working directory. The destination dir can still be overridden with -o.

That is probably too subtle, but it is preserving the old behavior for a single target, which I wouldn't mind keeping at least for now...

@thepudds
Copy link
Owner

thepudds commented Jan 3, 2022

Hi @josharian, regarding the package "errors" without types was imported error you observed at the top here, as far as I understand based on some digging, in theory that should not have happened because fzgen was already on x/tools@v0.1.8, where v0.1.8 already includes a fix for a mismatch between x/tools and the toolchain export format that in theory is the root cause of that error (e.g., golang/go#49159 (comment)).

I tried to confirm my understanding, and @findleyr replied and expressed some interested in understanding the scenario better:

if you can uncover a case where this error occurs with x/tools@v0.1.8, we'd be interested to understand it.

It is an artifact of corrupt export data, due to the export format changing during the course of 1.18 development. This could be due to version skew or a stale cache, I suspect.

The most immediate question might be if this still occurs for you using your Go 1.18, and if so, what exact version is that?

@findleyr also said clearing the build cache might work around it. That said, if this is worth any additional investigation, maybe clearing the build cache is not the thing to do if @findleyr wants to poke at it...

Or, maybe this is not worth investigating more, but I wanted to at least give @findleyr an opportunity to comment.

Finally, sorry if there is some mistake here on my part 🙃

@findleyr
Copy link

If you have a chance to look at this, perhaps try copying your cache somewhere and checking if it reproduces with a clear cache.

Though I'm not sure there's much value to poking at the cache. Things moved quite quickly over the last few months, and we're mostly just concerned that what we have now (across all importers/exporters) is coherent.

@thepudds
Copy link
Owner

Closing as resolved.

(As of ~January, the only pending mystery was why package "errors" without types was imported was seen, but given it was seen by a Go compiler developer, the version skew explanation seems very plausible, and the error has not been reported since).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants