cmd/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr) #34053

leighmcculloch · 2019-09-03T23:32:06Z

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

$ go version
go version go1.13 linux/amd64

Does this issue reproduce with the latest release?

Yes

Does this issue reproduce with the previous release?

No

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/leighmcculloch/local/bin"
GOCACHE="/home/leighmcculloch/.cache/go-build"
GOENV="/home/leighmcculloch/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/leighmcculloch/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/leighmcculloch/local/bin/go/1.13"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/leighmcculloch/local/bin/go/1.13/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/leighmcculloch/devel/stellar-go/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build252896639=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ mkdir test
$ cd test
$ go mod init test
$ cat > main.go <<-EOF
package main

import "fmt"

func main() {
        v := "bye"
        for {
                v := "hello"
                fmt.Println(v)
                break
        }
        fmt.Println(v)
}
EOF
$ go get -u golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
$ go vet -vettool=$(which shadow)

What did you expect to see?

I expected to see the output indicating the shadow variable:

$ go vet -vettool=$(which shadow)
# test
./main.go:8:3: declaration of "v" shadows declaration at line 6

What did you see instead?

The output was huge, 1541 lines of output. It was repeatedly dumping out an error flag provided but not defined: -unsafeptr once for some number of standard library packages along with the usage documentation for the shadow command. At the very end was the output I expected.

$ go vet -vettool=$(which shadow)
# internal/cpu
flag provided but not defined: -unsafeptr
shadow: check for possible unintended shadowing of variables

Usage: shadow [-flag] [package]

This analyzer check for shadowed variables.
...
# test
./main.go:8:3: declaration of "v" shadows declaration at line 6

To condense the output down a bit we can see the packages it is displaying the error about:

$ go vet -vettool=$(which shadow) 2>&1 | grep '^#'
# internal/cpu
# runtime/internal/sys
# internal/bytealg
# runtime/internal/math
# unicode/utf8
# internal/race
# sync/atomic
# unicode
# math/bits
# internal/testlog
# runtime/internal/atomic
# math
# runtime
# internal/reflectlite
# sync
# sort
# errors
# io
# internal/oserror
# strconv
# syscall
# reflect
# internal/syscall/unix
# time
# internal/fmtsort
# internal/poll
# os
# fmt
# test

Does this issue reproduce with the previous release? (details)

No. This doesn't happen with Go 1.12.9. If I run the above commands with that version of Go, it behaves as expected and outputs the shadow errors. Example:

$ go1.12.9 vet -vettool=$(which shadow)           
# test
./main.go:8:3: declaration of "v" shadows declaration at line 6

The text was updated successfully, but these errors were encountered:

leighmcculloch · 2019-09-03T23:43:49Z

The process I'm following for installing shadow is in the Go 1.12 release docs here:
https://golang.org/doc/go1.12#vet

If you run go help vet the install instructions are captured there also:

$ go1.13 help vet
usage: go vet [-n] [-x] [-vettool prog] [build flags] [vet flags] [packages]
...
The -vettool=prog flag selects a different analysis tool with alternative
or additional checks.
For example, the 'shadow' analyzer can be built and run using these commands:

  go install golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
  go vet -vettool=$(which shadow)
...

So I believe I am using the tool appropriately.

Add testing and support for Go 1.13 and discontinue support for Go 1.11. To maintain consistency with supported and released versions of Go. Go provides security updates for the last two versions of Go, this means the current release (1.13) and the previous release (1.12). Go 1.13 was released today meaning security updates will no longer be released for Go 1.11. Go 1.13 also comes with several new features relating to Modules including more efficient and reliable downloading of dependencies. As we switch to using Modules it will be a better experience for all and we'll be making the most of the change to promote and use Go 1.13. Close #1696 Summary of changes: - Update 1.13 builds to use latest release of 1.13. - Remove 1.11 from builds. - Update all documentation references of supported versions of Go. - Update `govet.sh` to use `-vettool` option that was added in Go 1.12. This was an outstanding TODO item in the file already, but we needed to do it to make the file work with Go 1.12+. Closes #1038. - Add notes to changelogs. Known limitations & issues: There is a bug in the Go 1.13's `go vet` tool that prevents it from being used with shadow. For this reason only the standard vet checks run on Go 1.13 builds, and the additional shadow check runs on Go 1.12 only. I stumbled onto this issue and opened an issue here for it: golang/go#34053.

bcmills · 2019-09-04T14:19:23Z

The -unsafeptr argument likely comes from here:

go/src/cmd/go/internal/work/exec.go

Lines 1017 to 1043 in 407010e

    
           // In GOROOT, we enable all the vet tests during 'go test', 
        
           // not just the high-confidence subset. This gets us extra 
        
           // checking for the standard library (at some compliance cost) 
        
           // and helps us gain experience about how well the checks 
        
           // work, to help decide which should be turned on by default. 
        
           // The command-line still wins. 
        
           // 
        
           // Note that this flag change applies even when running vet as 
        
           // a dependency of vetting a package outside std. 
        
           // (Otherwise we'd have to introduce a whole separate 
        
           // space of "vet fmt as a dependency of a std top-level vet" 
        
           // versus "vet fmt as a dependency of a non-std top-level vet".) 
        
           // This is OK as long as the packages that are farther down the 
        
           // dependency tree turn on *more* analysis, as here. 
        
           // (The unsafeptr check does not write any facts for use by 
        
           // later vet runs.) 
        
           if a.Package.Goroot && !VetExplicit { 
        
           	// Note that $GOROOT/src/buildall.bash 
        
           	// does the same for the misc-compile trybots 
        
           	// and should be updated if these flags are 
        
           	// changed here. 
        
           	// 
        
           	// There's too much unsafe.Pointer code 
        
           	// that vet doesn't like in low-level packages 
        
           	// like runtime, sync, and reflect. 
        
           	vetFlags = []string{"-unsafeptr=false"} 
        
           }

Probably the shadow command needs to be updated to be flag-compatible with the actual vet command.

CC @ianthehat @matloob

leighmcculloch · 2019-09-04T14:30:23Z

Could we instead not pass this flag down to custom tools? It seems unnecessary to require all vettool programs to support this flag.

gyuho · 2019-09-06T19:49:03Z

Is there any workaround? etcd etcd-io/etcd#11110 is experiencing the same issue...

matloob · 2019-09-06T20:56:44Z

Expanding on @leighmcculloch's proposal Would it be reasonable to redefine VetExplicit to mean whether flags are passed to vet or a custom vet tool is used? I think users of vet with a custom tool would be okay filtering out the unsafeptr errors in goroot if their tool included it.

One workaround (which might not be workable in your case) is to add a unsafeptr flag to your custom tool

viswajithiii · 2019-09-17T20:21:54Z

If it helps anyone, I used the following workaround (building on @matloob's suggestion):

Save this in a file called, say, customvettool/customvettool.go. Run go install ./customvettool to install the binary, and then you can invoke as go vet -vettool=$(which customvettool).

package main

import (
	"flag"

	"golang.org/x/tools/go/analysis/passes/shadow"
	"golang.org/x/tools/go/analysis/unitchecker"
)

func main() {
	unitchecker.Main(
		shadow.Analyzer,
                // ... other custom analyzers
	)
}

func init() {
	// go vet always adds this flag for certain packages in the standard library,
	// which causes "flag provided but not defined" errors when running with
	// custom vet tools.
	// So we just declare it here and swallow the flag.
	// See https://github.com/golang/go/issues/34053 for details.
	// TODO: Remove this once above issue is resolved.
	flag.String("unsafeptr", "", "")
}

kevinburke · 2019-09-23T17:27:26Z

Previously, this check was also broken in the Go 1.12 release. #29260

leighmcculloch · 2019-09-24T20:26:12Z

It also seems like we can work around this for the moment by running shadow independently of go vet. In our workflow we can replace go vet -vettool=$(which shadow) ./... with shadow ./....

Run the go vet tool `shadow` for all Go versions, specifically making it possible to run it with Go 1.13. We should have consistent checks and tests across the versions of Go we have running. When I initially added the Go 1.13 block and I excluded the shadow check it was because of golang/go#34053 and I didn't see a simple way to keep running it for Go 1.13. Simply running the tool directly is an easy way to still use the tool since we don't have to run it with `go vet`. The reason we'd want to run it with `go vet` is that's the direction analysis tools are going, and the intention is that if you run multiple tools with go vet it will do certain parsing and loading of code only once. In our case this doesn't really matter right now.

agnivade · 2019-10-13T10:14:31Z

By the time I found this issue, I already spent too much time trying to figure out what was wrong in my codebase. Now I am bothered enough to fix this.

There seems to be a separate VetTool flag which indicates a presence of an alternate binary.

// VetTool is the path to an alternate vet tool binary.
// The caller is expected to set it (if needed) before executing any vet actions.
var VetTool string

Adding this to the if condition should do it. Will send a CL.

gopherbot · 2019-10-13T15:15:59Z

Change https://golang.org/cl/200957 mentions this issue: cmd/go/internal/work: fix error while passing custom vet tool

hanzei · 2019-10-15T11:45:42Z

Will this fix get back ported into 1.13?

agnivade · 2019-10-15T15:43:46Z

It's definitely a backport candidate. But will leave it to @bcmills for a decision.

bcmills · 2019-10-15T17:40:35Z

@gopherbot, please backport to 1.13: this was an early regression and the fix is trivial.

gopherbot · 2019-10-15T17:40:41Z

Backport issue(s) opened: #34922 (for 1.13).

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

gopherbot · 2019-10-15T17:42:34Z

Change https://golang.org/cl/201237 mentions this issue: [release-branch.go1.13] cmd/go/internal/work: fix error while passing custom vet tool

… custom vet tool For GOROOT packages, we were adding -unsafeptr=false to prevent unsafe.Pointer checks. But the flag also got passed to invocations of go vet with a custom vet tool. To prevent this from happening, we add this flag only when no tools are passed. Updates #34053 Fixes #34922 Change-Id: I8bcd637fd8ec423d597fcdab2a0ceedd20786019 Reviewed-on: https://go-review.googlesource.com/c/go/+/200957 Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> (cherry picked from commit 902d5aa) Reviewed-on: https://go-review.googlesource.com/c/go/+/201237 Run-TryBot: Bryan C. Mills <bcmills@google.com> Reviewed-by: Agniva De Sarker <agniva.quicksilver@gmail.com>

Remove the work around that was added in 828c132 that built our own custom `shadow` binary that was able to handle the presence of the `unsafeptr` flag. It's good to remove hacks and work arounds when they are no longer needed. Go 1.13.3 was released today that fixes the bug (golang/go#34053) that was introduced in Go 1.13 where the `unsafeptr` flag was passed down to `go vet` `vettool` programs. The `shadow` tool wasn't capable of handling that flag and cause it to error. In Go 1.13.3 behavior has been returned to match Go 1.12 and the flag is no longer passed down. - Developers who run the script in versions of Go 1.13 that isn't the latest release will still encounter an error. It's not common for developers to run this script locally so this is unlikely to be of huge inconvenience. We should also encourage developers to be using the latest patch releases of Go as they often contain critical security fixes and bug fixes so I think accommodating them is a lower priority.

Requires Go 1.13.3 to avoid build error (actually, fails on `go vet` using `shadow`) due to golang/go#34053

leighmcculloch mentioned this issue Sep 4, 2019

all: drop support for go1.11, add support for go1.13 stellar/go#1700

Merged

7 tasks

bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 4, 2019

bcmills added this to the Go1.14 milestone Sep 4, 2019

gyuho mentioned this issue Sep 6, 2019

Go vet shadow test fails in Go 1.13 etcd-io/etcd#11131

Closed

leighmcculloch mentioned this issue Sep 24, 2019

all: run go vet shadow for all go versions stellar/go#1778

Merged

7 tasks

cmaster11 added a commit to cmaster11/overseer that referenced this issue Sep 25, 2019

fix golang/go#34053 (comment)

1e1d5df

rsc modified the milestones: Go1.14, Backlog Oct 9, 2019

gopherbot closed this as completed in 902d5aa Oct 14, 2019

gopherbot mentioned this issue Oct 15, 2019

cmd/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr) [1.13 backport] #34922

Closed

leighmcculloch mentioned this issue Oct 17, 2019

all: remove go vet shadow hack stellar/go#1860

Merged

7 tasks

akshaychhajed mentioned this issue Oct 29, 2019

Port the "roles" command from Mattermost CLI to mmctl mattermost/mattermost#12960

Closed

wiggin77 added a commit to mattermost/mmctl that referenced this issue Jan 7, 2020

Bump minimum Go version required

c3c1325

Requires Go 1.13.3 to avoid build error (actually, fails on `go vet` using `shadow`) due to golang/go#34053

wiggin77 mentioned this issue Jan 7, 2020

Bump minimum Go version required mattermost/mmctl#98

Merged

mgdelacroix pushed a commit to mattermost/mmctl that referenced this issue Jan 9, 2020

Bump minimum Go version required (#98)

c8c9855

Requires Go 1.13.3 to avoid build error (actually, fails on `go vet` using `shadow`) due to golang/go#34053

aayush-plivo mentioned this issue Feb 5, 2020

GitHub plugin has no validation if subscribing with an invalid feature mattermost/mattermost-plugin-github#165

Closed

jasdel mentioned this issue Feb 12, 2020

Passing gzip.NewReader to s3manager.UploadInput causes SIGSEGV aws/aws-sdk-go#3130

Closed

golang locked and limited conversation to collaborators Oct 14, 2020

gopherbot added the FrozenDueToAge label Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr) #34053

cmd/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr) #34053

leighmcculloch commented Sep 3, 2019 •

edited

leighmcculloch commented Sep 3, 2019

bcmills commented Sep 4, 2019

leighmcculloch commented Sep 4, 2019

gyuho commented Sep 6, 2019

matloob commented Sep 6, 2019

viswajithiii commented Sep 17, 2019

kevinburke commented Sep 23, 2019

leighmcculloch commented Sep 24, 2019

agnivade commented Oct 13, 2019

gopherbot commented Oct 13, 2019

hanzei commented Oct 15, 2019

agnivade commented Oct 15, 2019

bcmills commented Oct 15, 2019

gopherbot commented Oct 15, 2019

gopherbot commented Oct 15, 2019

cmd/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr) #34053

cmd/vet: go vet -vettool=$(which shadow) errors in go1.13 only (flag provided but not defined: -unsafeptr) #34053

Comments

leighmcculloch commented Sep 3, 2019 • edited

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

Does this issue reproduce with the latest release?

Does this issue reproduce with the previous release?

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

What did you do?

What did you expect to see?

What did you see instead?

Does this issue reproduce with the previous release? (details)

leighmcculloch commented Sep 3, 2019

bcmills commented Sep 4, 2019

leighmcculloch commented Sep 4, 2019

gyuho commented Sep 6, 2019

matloob commented Sep 6, 2019

viswajithiii commented Sep 17, 2019

kevinburke commented Sep 23, 2019

leighmcculloch commented Sep 24, 2019

agnivade commented Oct 13, 2019

gopherbot commented Oct 13, 2019

hanzei commented Oct 15, 2019

agnivade commented Oct 15, 2019

bcmills commented Oct 15, 2019

gopherbot commented Oct 15, 2019

gopherbot commented Oct 15, 2019

leighmcculloch commented Sep 3, 2019 •

edited

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

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