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

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

mvdan opened this issue Oct 9, 2018 · 7 comments
FrozenDueToAge NeedsInvestigation


Copy link

@mvdan mvdan commented Oct 9, 2018

I encountered this weird issue while converting to go/packages. You can see the CI status here, where 1.11.1 is green, yet 1.10.4 is red:

I've boiled it down to this piece of code:

To reproduce, run:

cat >main.go <<EOF
package main

import (

func main() {
        cfg := &packages.Config{Mode: packages.LoadSyntax}
        pkgs, err := packages.Load(cfg, "main.go")
        if err  != nil {
        prog, _ := ssautil.Packages(pkgs, 0)
go get -d -u
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]:*Package).typeOf(0xc4237b6060, 0x7207a0, 0xc42021d9c0, 0x0, 0x0)
        /go/src/ +0x1be*builder).addr(0xc4229c7d00, 0xc420098140, 0x7207a0, 0xc42021d9c0, 0x1, 0x0, 0x0)
        /go/src/ +0xb82*builder).expr0(0xc4229c7d00, 0xc420098140, 0x721020, 0xc4202346e0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc420838f80, ...)
        /go/src/ +0x1cf2*builder).expr(0xc4229c7d00, 0xc420098140, 0x721020, 0xc4202346e0, 0x0, 0x0)
        /go/src/ +0x289*builder).stmt(0xc4229c7d00, 0xc420098140, 0x720d60, 0xc420234700)
        /go/src/ +0x194c*builder).stmtList(0xc4229c7d00, 0xc420098140, 0xc42000f4c0, 0x1, 0x1)
        /go/src/ +0x59*builder).stmt(0xc423510d00, 0xc420098140, 0x720620, 0xc4201ddcb0)
        /go/src/ +0x2646*builder).buildFunction(0xc423510d00, 0xc420098140)
        /go/src/ +0x2f1*builder).buildFuncDecl(0xc423510d00, 0xc4237b6060, 0xc4201ddce0)
        /go/src/ +0xf4*Package).build(0xc4237b6060)
        /go/src/ +0x7f9*Package).(
        /go/src/ +0x2a
sync.(*Once).Do(0xc4237b608c, 0xc420263f98)
        /usr/local/go/src/sync/once.go:44 +0xbe*Package).Build(0xc4237b6060)
        /go/src/ +0x4c*Program).Build.func1(0xc422865c70, 0xc4237b6060)
        /go/src/ +0x2b
created by*Program).Build
        /go/src/ +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]:*Program).packageLevelValue(0xc425fe14a0, 0x0, 0x0, 0x90e8e0, 0x78ad40)
	/home/travis/gopath/src/ +0x26*builder).expr0(0xc4200d7d00, 0xc4237b97c0, 0x78bbc0, 0xc4228b39a0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc427f443e0, ...)
	/home/travis/gopath/src/ +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 label Oct 9, 2018
@gopherbot gopherbot added this to the Unreleased milestone Oct 9, 2018
Copy link

@alandonovan alandonovan commented Oct 9, 2018

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.

Copy link
Member Author

@mvdan 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
Copy link

@alandonovan alandonovan commented Oct 12, 2018

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.

Copy link

@gopherbot gopherbot commented Oct 12, 2018

Change mentions this issue: go/ssa/ssautil: fix a crash in Packages

Copy link

@alandonovan alandonovan commented Oct 12, 2018

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. contains a fix.

Copy link
Member Author

@mvdan 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?

Copy link

@alandonovan alandonovan commented Oct 15, 2018


@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.
FrozenDueToAge NeedsInvestigation
None yet

No branches or pull requests

3 participants