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: panic: no ssa.Value for function argument #33279

Open
dominikh opened this issue Jul 25, 2019 · 4 comments
Open

x/tools/go/ssa: panic: no ssa.Value for function argument #33279

dominikh opened this issue Jul 25, 2019 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@dominikh
Copy link
Member 

$ go version
go version go1.12.7 linux/amd64

What did you do?

Input program:

package pkg

func fn() {
	func(arg int) []int {
		_ = arg
		return nil
	}(0)[0]++
}

Run ssadump on the package.

The panic does not occur if the result of the function call or of the indexing operation are stored in a temporary variable before doing the post-increment.

What did you expect to see?

No panic.

What did you see instead?

panic: no ssa.Value for var arg int

goroutine 1 [running]:
golang.org/x/tools/go/ssa.(*Function).lookup(0xc0000b0780, 0x7d5b20, 0xc0000ad950, 0xa09601, 0x508c4125f9dfb600, 0x0)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/func.go:428 +0x472
golang.org/x/tools/go/ssa.(*Function).lookup(0xc0000b08c0, 0x7d5b20, 0xc0000ad950, 0xa09600, 0x1, 0x0)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/func.go:430 +0x11b
golang.org/x/tools/go/ssa.(*builder).addr(0xc000175a80, 0xc0000b08c0, 0x7cec20, 0xc00000d200, 0xc00010cb00, 0xc000174290, 0xc00010cb60)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:346 +0x246
golang.org/x/tools/go/ssa.(*builder).expr(0xc000175a80, 0xc0000b08c0, 0x7cec20, 0xc00000d200, 0x722500, 0x9ed680)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:528 +0x174
golang.org/x/tools/go/ssa.(*builder).assign(0xc000175a80, 0xc0000b08c0, 0x7cfbe0, 0xa08bc8, 0x7cec20, 0xc00000d200, 0x403600, 0xc0001743a8)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:502 +0x105
golang.org/x/tools/go/ssa.(*builder).assignStmt(0xc000175a80, 0xc0000b08c0, 0xc00005a830, 0x1, 0x1, 0xc00005a840, 0x1, 0x1, 0xc00005a600)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:1050 +0x1d5
golang.org/x/tools/go/ssa.(*builder).stmt(0xc000175a80, 0xc0000b08c0, 0x7ce620, 0xc000022940)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2006 +0x209c
golang.org/x/tools/go/ssa.(*builder).stmtList(0xc000175a80, 0xc0000b08c0, 0xc00000d280, 0x2, 0x2)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:790 +0x72
golang.org/x/tools/go/ssa.(*builder).stmt(0xc000175a80, 0xc0000b08c0, 0x7ce7a0, 0xc000085800)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2102 +0x22b7
golang.org/x/tools/go/ssa.(*builder).buildFunction(0xc000175a80, 0xc0000b08c0)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2195 +0x2d2
golang.org/x/tools/go/ssa.(*builder).expr0(0xc000175a80, 0xc0000b0780, 0x7ceb20, 0xc00005a870, 0x7, 0x7cb560, 0xc000085bc0, 0x0, 0x0, 0x5555555555555555, ...)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:554 +0x3b4
golang.org/x/tools/go/ssa.(*builder).expr(0xc000175a80, 0xc0000b0780, 0x7ceb20, 0xc00005a870, 0x40bda9, 0xc0000ba700)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:530 +0x27e
golang.org/x/tools/go/ssa.(*builder).setCallFunc(0xc000175a80, 0xc0000b0780, 0xc000022980, 0xc0000ba740)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:881 +0xa5
golang.org/x/tools/go/ssa.(*builder).setCall(0xc000175a80, 0xc0000b0780, 0xc000022980, 0xc0000ba740)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:962 +0x53
golang.org/x/tools/go/ssa.(*builder).expr0(0xc000175a80, 0xc0000b0780, 0x7ce820, 0xc000022980, 0x7, 0x7cb5a0, 0xc00005a970, 0x0, 0x0, 0x7cb5a0, ...)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:596 +0x29cd
golang.org/x/tools/go/ssa.(*builder).expr(0xc000175a80, 0xc0000b0780, 0x7ce820, 0xc000022980, 0xc00005a970, 0x7f1c7d0e9678)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:530 +0x27e
golang.org/x/tools/go/ssa.(*builder).addr(0xc000175a80, 0xc0000b0780, 0x7ced20, 0xc000085830, 0xc000175500, 0xc000175680, 0xa08bc8)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:396 +0x63c
golang.org/x/tools/go/ssa.(*builder).stmt(0xc000175a80, 0xc0000b0780, 0x7cece0, 0xc00000d2e0)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2000 +0x231d
golang.org/x/tools/go/ssa.(*builder).stmtList(0xc000175a80, 0xc0000b0780, 0xc00005a8a0, 0x1, 0x1)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:790 +0x72
golang.org/x/tools/go/ssa.(*builder).stmt(0xc000175a80, 0xc0000b0780, 0x7ce7a0, 0xc000085860)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2102 +0x22b7
golang.org/x/tools/go/ssa.(*builder).buildFunction(0xc000175a80, 0xc0000b0780)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2195 +0x2d2
golang.org/x/tools/go/ssa.(*builder).buildFuncDecl(0xc000175a80, 0xc000066900, 0xc000085890)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2225 +0xed
golang.org/x/tools/go/ssa.(*Package).build(0xc000066900)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2341 +0x88a
sync.(*Once).Do(0xc00006692c, 0xc000175ce8)
	/usr/lib/go/src/sync/once.go:44 +0xb3
golang.org/x/tools/go/ssa.(*Package).Build(0xc000066900)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2260 +0x54
main.doMain(0x0, 0x0)
	/home/dominikh/prj/src/golang.org/x/tools/cmd/ssadump/main.go:146 +0x5ff
main.main()
	/home/dominikh/prj/src/golang.org/x/tools/cmd/ssadump/main.go:64 +0x26

/cc @ianthehat
@gopherbot gopherbot added this to the Unreleased milestone Jul 25, 2019
@katiehockman
Copy link
Contributor

/cc @matloob

@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 29, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@matloob
Copy link
Contributor

matloob commented Oct 30, 2019

I can reproduce this: I build ssadump with go1.13 and ran it on the test file and it panicked.

But I can't reproduce it with a version of ssadump built with tip (go version devel +47efbf0a4e Wed Oct 30 00:41:31 2019 +0000 darwin/amd64).

I wonder why that is

@dominikh
Copy link
Member Author

dominikh commented Nov 5, 2019 •
edited

The following commit fixed the crash:

commit bdd0ff08db7abf07db29cd6dca98b5c1bc26ef26
Author: Robert Griesemer <gri@golang.org>
Date:   Thu Aug 22 21:27:33 2019 -0700

    go/types: process each segment of delayed actions in FIFO order
    
    The stack of delayed actions is grown by pushing a new action
    on top with Checker.later. Checker.processDelayed processes
    all actions above a top watermark and then resets the stack
    to top.
    
    Until now, pushed actions above the watermark were processed
    in LIFO order. This change processes them in FIFO order, which
    seems more natural (if an action A was delayed before an action
    B, A should be processed before B for that stack segment).
    
    (With this change, Checker.later could be used instead of
    Checker.atEnd to postpone interface method type comparison
    and then the specific example in issue #33656 does type-check.
    However, in general we want interface method type comparisons
    to run after all interfaces are completed. With Checker.later
    we may still end up mixing interface completions and interface
    method type comparisons in ways leading to other errors for
    sufficiently convoluted code.)
    
    Also, move Checker.processDelayed from resolver.go to check.go.
    
    Change-Id: Id31254605e6944c490eab410553fff907630cc64
    Reviewed-on: https://go-review.googlesource.com/c/go/+/191458
    Reviewed-by: Matthew Dempsky <mdempsky@google.com>

 src/go/types/check.go    | 15 +++++++++++++++
 src/go/types/resolver.go | 10 ----------
 2 files changed, 15 insertions(+), 10 deletions(-)

It should've been a behavior-preserving change, but it wasn't. Pinging @griesemer to inquire if the code in the original comment would be something that might be type-checked differently before and after the commit.

Edit: edited to ping our @griesemer and not an imposter :-)

@griesemer griesemer mentioned this issue Nov 5, 2019
Open
@griesemer
Copy link
Contributor

@dominikh Yes, it's possible that type-checking might proceed in a different order - it's not obviously clear what would have changed w/o analyzing this case in detail, though. I created #35374 to follow up on this when I have a chance. Thanks for tracking this down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@dominikh @griesemer @gopherbot @matloob @katiehockman