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

Jump to bottom

x/tools/go/ssa/ssautil: clarify that LoadAllSyntax is required, and provide better errors #28106

Closed
mvdan opened this issue Oct 9, 2018 · 7 comments
Closed

x/tools/go/ssa/ssautil: clarify that LoadAllSyntax is required, and provide better errors #28106

mvdan opened this issue Oct 9, 2018 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@mvdan
Copy link
Member

mvdan commented Oct 9, 2018

I encountered this weird issue while converting https://github.com/mvdan/unparam to go/packages. You can see the CI status here, where 1.11.1 is green, yet 1.10.4 is red: https://travis-ci.org/mvdan/unparam/builds/439236099

I've boiled it down to this piece of code: https://play.golang.org/p/vUTgArM21wJ

To reproduce, run:

cat >main.go <<EOF
package main

import (
        "golang.org/x/tools/go/packages"
        "golang.org/x/tools/go/ssa/ssautil"
)

func main() {
        cfg := &packages.Config{Mode: packages.LoadSyntax}
        pkgs, err := packages.Load(cfg, "main.go")
        if err  != nil {
                panic(err)
        }
        prog, _ := ssautil.Packages(pkgs, 0)
        prog.Build()
}
EOF
go get -d -u golang.org/x/tools/go/ssa/ssautil
go run main.go

Locally, on go version devel +165ebaf97b Sun Oct 7 02:36:02 2018 +0000 linux/amd64, the commands above succeed.

Here is the behavior with 1.10.4 - I'm using a Docker image to ensure that it's easy to reproduce for everyone. Funnily enough, I could not reproduce it on my Linux system with a binary installation of 1.10.4.

$ docker run -it golang:1.10.4-stretch bash
# [ commands above ]
# go run main.go
panic: no type for *ast.CompositeLit @ /usr/local/go/src/errors/errors.go:10:10

goroutine 967 [running]:
golang.org/x/tools/go/ssa.(*Package).typeOf(0xc4237b6060, 0x7207a0, 0xc42021d9c0, 0x0, 0x0)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2377 +0x1be
golang.org/x/tools/go/ssa.(*builder).addr(0xc4229c7d00, 0xc420098140, 0x7207a0, 0xc42021d9c0, 0x1, 0x0, 0x0)
        /go/src/golang.org/x/tools/go/ssa/builder.go:351 +0xb82
golang.org/x/tools/go/ssa.(*builder).expr0(0xc4229c7d00, 0xc420098140, 0x721020, 0xc4202346e0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc420838f80, ...)
        /go/src/golang.org/x/tools/go/ssa/builder.go:603 +0x1cf2
golang.org/x/tools/go/ssa.(*builder).expr(0xc4229c7d00, 0xc420098140, 0x721020, 0xc4202346e0, 0x0, 0x0)
        /go/src/golang.org/x/tools/go/ssa/builder.go:530 +0x289
golang.org/x/tools/go/ssa.(*builder).stmt(0xc4229c7d00, 0xc420098140, 0x720d60, 0xc420234700)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2045 +0x194c
golang.org/x/tools/go/ssa.(*builder).stmtList(0xc4229c7d00, 0xc420098140, 0xc42000f4c0, 0x1, 0x1)
        /go/src/golang.org/x/tools/go/ssa/builder.go:790 +0x59
golang.org/x/tools/go/ssa.(*builder).stmt(0xc423510d00, 0xc420098140, 0x720620, 0xc4201ddcb0)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2102 +0x2646
golang.org/x/tools/go/ssa.(*builder).buildFunction(0xc423510d00, 0xc420098140)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2195 +0x2f1
golang.org/x/tools/go/ssa.(*builder).buildFuncDecl(0xc423510d00, 0xc4237b6060, 0xc4201ddce0)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2225 +0xf4
golang.org/x/tools/go/ssa.(*Package).build(0xc4237b6060)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2341 +0x7f9
golang.org/x/tools/go/ssa.(*Package).(golang.org/x/tools/go/ssa.build)-fm()
        /go/src/golang.org/x/tools/go/ssa/builder.go:2260 +0x2a
sync.(*Once).Do(0xc4237b608c, 0xc420263f98)
        /usr/local/go/src/sync/once.go:44 +0xbe
golang.org/x/tools/go/ssa.(*Package).Build(0xc4237b6060)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2260 +0x4c
golang.org/x/tools/go/ssa.(*Program).Build.func1(0xc422865c70, 0xc4237b6060)
        /go/src/golang.org/x/tools/go/ssa/builder.go:2244 +0x2b
created by golang.org/x/tools/go/ssa.(*Program).Build
        /go/src/golang.org/x/tools/go/ssa/builder.go:2243 +0x11a
exit status 2

The original panic was different; I'm pasting the start of it below. However, since both only happen on 1.10 and this is the minimised case of the original panic, I'm pretty sure they are the same bug.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x629bb6]
goroutine 1232 [running]:
golang.org/x/tools/go/ssa.(*Program).packageLevelValue(0xc425fe14a0, 0x0, 0x0, 0x90e8e0, 0x78ad40)
	/home/travis/gopath/src/golang.org/x/tools/go/ssa/source.go:189 +0x26
golang.org/x/tools/go/ssa.(*builder).expr0(0xc4200d7d00, 0xc4237b97c0, 0x78bbc0, 0xc4228b39a0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc427f443e0, ...)
	/home/travis/gopath/src/golang.org/x/tools/go/ssa/builder.go:684 +0x1fce

Since it only happens on 1.10, this might be a bug in a package in the standard library like go/types. I haven't investigated further.

Unfortunately, this does make using go/packages in my tool quite painful. I'd rather not drop support for Go 1.10 just yet.

/cc @ianthehat @alandonovan
@mvdan mvdan added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 9, 2018
@gopherbot gopherbot added this to the Unreleased milestone Oct 9, 2018
@alandonovan
Copy link
Contributor

The doc comment for ssautil.Packages says "a set of packages loaded from source syntax". This is admittedly not very clear, but it means you need to use Mode: packages.LoadAllSyntax in go/packages.

Both the documentation and the way in which the error is reported could certainly be improved.

@mvdan
Copy link
Member Author

mvdan commented Oct 10, 2018

You're right; I should have noticed that. I agree that the docs could be a bit clearer, and that it would be great if ssautil errored sooner with a more obvious message.

I got confused by how this succeeded on 1.11, and failed on 1.10 under some circumstances. I guess I was getting lucky somehow.

@mvdan mvdan changed the title x/tools/go/ssa/ssautil: panic on Go 1.10.4 with go/packages x/tools/go/ssa/ssautil: clarify that LoadAllSyntax is required, and provide better errors Oct 10, 2018
@alandonovan
Copy link
Contributor

Hmm, on closer inspection the API does intend to support exactly what you're doing, with a mixture of packages loaded from source and from export data, and there's even an Example of it. Using AllSyntax will likely be an effective workaround, but the bug as originally reported is real.

@gopherbot
Copy link

Change https://golang.org/cl/141686 mentions this issue: go/ssa/ssautil: fix a crash in Packages

@alandonovan
Copy link
Contributor

The root cause is that go/packages' go1.10 "fallback" loads the entire program from source because go1.10 list doesn't generate export data; however, it doesn't bother to type-check function bodies of imports in this mode since the function types are all that it needs. However, the SSA builder assumes that when a package has syntax trees, they have been type checked, and consequently it panics when it finds function bodies without type information.

https://go-review.googlesource.com/c/tools/+/141686 contains a fix.

@mvdan
Copy link
Member Author

mvdan commented Oct 15, 2018

Does this mean that it's entirely reasonable for me to use LoadSyntax instead of LoadAllSyntax if I'm not interested in the SSA form of the dependencies?

@alandonovan
Copy link
Contributor

Yes.

@golang golang locked and limited conversation to collaborators Oct 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@mvdan @alandonovan @gopherbot