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

go-fuzz-build: build fails at tip Go due to duplicated //go:build lines #313

Closed
findleyr opened this issue Feb 24, 2021 · 3 comments · Fixed by #323
Closed

go-fuzz-build: build fails at tip Go due to duplicated //go:build lines #313

findleyr opened this issue Feb 24, 2021 · 3 comments · Fixed by #323

Comments

@findleyr
Copy link

I believe this is only an issue for the newly landed changes in tip Go for Go 1.17: golang.org/issues/41184

When built against tip Go, go-fuzz-build fails with, for example, the following error:

failed to execute go build: exit status 1
../../../../src/go/src/os/error.go:9: errno_unix.go: multiple //go:build comments
../../../../src/go/src/internal/poll/copy_file_range_linux.go:8: at.go: multiple //go:build comments
../../../../src/go/src/reflect/value.go:10: exp_asm.go: multiple //go:build comments
../../../../src/go/src/math/fma.go:7: bits_errors.go: multiple //go:build comments
../../../../src/go/src/fmt/print.go:10: dir_unix.go: multiple //go:build comments
../../../../src/go/src/go/scanner/scanner.go:15: path_unix.go: multiple //go:build comments
../../../../src/go/src/internal/fmtsort/sort.go:13: slice_go113.go: multiple //go:build comments
../../../../src/go/src/reflect/type.go:20: bytealg.go: multiple //go:build comments

Checking the instrumented goroot, we can see that this is indeed the case, for example, in errno_unix.go:

//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris                                                            
// +build aix darwin dragonfly freebsd linux netbsd openbsd solaris                                                                                  
                                                                                                                                                     
//line /usr/local/google/home/rfindley/src/go/src/internal/poll/errno_unix.go:5                                                                      
                                                                                                                                                     
//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris                                                            
                                                                                                                                                     
//line /usr/local/google/home/rfindley/src/go/src/internal/poll/errno_unix.go:8                                                                      
package poll  

Naively skimming the code, it appears there is simultaneously special handling in trimComments to preserve comments starting with //go:, and also special handling to preserve initial comments (in initialComments). It looks like these overlap, resulting in the duplicated //go:build lines.

I was able to achieve what I wanted (fuzzing some recent changes to go/parser) by patching go-fuzz-build to just skip //go:build lines, but this is just a hack: a real solution should preserve (and not duplicate) the //go:build lines.

Apologies if I've misused terminology; this is my first time using go-fuzz. Thanks for building such a useful tool! go-fuzz found the crash I was looking for in just a few minutes :)

@josharian
Copy link
Collaborator

Thanks for the excellent bug report.

go-fuzz is slated to be replaced by something in the standard library. So your hack (“patching go-fuzz-build to just skip //go:build lines”) actually sounds like just what is called for to tide us over.

Want to send a PR, by chance?

@findleyr
Copy link
Author

Thanks for the quick response!

go-fuzz is slated to be replaced by something in the standard library.

Indeed! My concern was that https://golang.org/issue/44551 will, if accepted, only be experimental in 1.17. But actually, reading the transition plan for //go:build, I think you're right that it's safe to just skip //go:build lines in the interim.

Want to send a PR, by chance?

Sure, will do (but not tonight :))

@mvdan
Copy link
Contributor

mvdan commented May 17, 2021

Ah, I just got bit by this. I'll just do a hack to locally remove the duplicate comment until I can jump to Go's own fuzzing.

thepudds added a commit to thepudds/go-fuzz that referenced this issue May 31, 2021
…etting 'go 1.17' directive

Workaround dvyukov#313.

cmd/go should in theory pay attention to the version set in the 'go' directive when interpreting //go:build comments,
thepudds added a commit to thepudds/go-fuzz that referenced this issue May 31, 2021
Avoid preserving '//go:build' comments in both trimComments and initialComments.

Fixes dvyukov#313.
dvyukov pushed a commit that referenced this issue Jun 2, 2021
…etting 'go 1.17' directive

Workaround #313.

cmd/go should in theory pay attention to the version set in the 'go' directive when interpreting //go:build comments,
dvyukov pushed a commit that referenced this issue Jun 2, 2021
Avoid preserving '//go:build' comments in both trimComments and initialComments.

Fixes #313.
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

Successfully merging a pull request may close this issue.

3 participants