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/cmd/guru: could pointsto analysis for unassigned interfaces be improved? #41370

Open
siadat opened this issue Sep 13, 2020 · 7 comments
Open

Comments

@siadat
Copy link
Contributor

@siadat siadat commented Sep 13, 2020

What version of Go are you using?

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/sina/Library/Caches/go-build"
GOENV="/Users/sina/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/sina/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/sina/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/sina/clones/tools/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/l1/gr_t1lss4837728b1sqz919m0000gn/T/go-build472257919=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

cat issue41370/issue.go
package main

func main() {
	var i interface{} = nil
	_ = i
}
#	_ = i
#	    ^
#	    58
$ guru -scope ./issue41370/ pointsto ./issue41370/issue.go:#58

What did you expect to see?

guru: this interface{} cannot contain any dynamic types

What did you see instead?

guru: pointer analysis did not find expression (dead code?)
@gopherbot gopherbot added the Tools label Sep 13, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 13, 2020
@siadat siadat changed the title x/tools/cmd/guru: could pointsto analysis for unassigned interfaces be improved? x/tools/cmd/guru: could pointsto analysis for unassigned interfaces be improved? (also: x/tools/go/pointer) Sep 13, 2020
@siadat
Copy link
Contributor Author

@siadat siadat commented Sep 13, 2020

Is this an ssa optimization to use a global untyped nil interface and avoid constructing it? If so, how do I go about finding out the rule for it?

When var i interface{} = nil:

func main():
0:                                                                entry P:0 S:0
        return

When var i interface{} = 555:

func main():
0:                                                                entry P:0 S:0
        t0 = make interface{} <- int (555:int)                      interface{}
        return
@siadat siadat changed the title x/tools/cmd/guru: could pointsto analysis for unassigned interfaces be improved? (also: x/tools/go/pointer) x/tools/cmd/guru: could pointsto analysis for unassigned interfaces be improved? Sep 13, 2020
@siadat
Copy link
Contributor Author

@siadat siadat commented Sep 14, 2020

I realized I could build the naive SSA form using -N

func main():
0:                                                                entry P:0 S:0
        t0 = local interface{} (i)                                 *interface{}
        *t0 = nil:interface{}
        t1 = *t0                                                    interface{}
        rundefers
        return

I'm exploring the code and slowing getting there lol

@siadat
Copy link
Contributor Author

@siadat siadat commented Sep 14, 2020

Okay, got it. All I had to do was add ssa.NaiveForm

prog := ssautil.CreateProgram(lprog, ssa.GlobalDebug|ssa.NaiveForm)

Now guru pointsto's message is a lot better (for my use):

this interface{} cannot contain any dynamic types.

I am still interested in being able to only partially enable ssa.NaiveForm, so that I don't miss other useful features of the non-naive form.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 15, 2020

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 15, 2020

@siadat: We welcome any patches if you're interested in contributing, but guru is largely unmaintained so we won't work on making any changes ourselves.

@siadat
Copy link
Contributor Author

@siadat siadat commented Sep 16, 2020

Thank you for the replies! :) Is guru being replaced by something else? gopls comes to mind which (if I'm not mistaken) uses analyzers from github.com/dominikh/go-tools. So, would it be correct to say the focus from guru is shifted to gopls plus github.com/dominikh/go-tools?

EDIT: okay, I found the implementation doc :)

@siadat
Copy link
Contributor Author

@siadat siadat commented Sep 16, 2020

Regarding this issue: I'd love to help! :) Enabling ssa.NaiveForm in guru will change the output of the pointsto subcommand for inputs other than the one reported in this issue. (I can write an example if needed.) So, I think a good fix would follow one of these two approaches:

  • patch the ssa.lift func so that the nil empty interface is not lifted but other things are, or;
  • add a -N flag to guru to enable ssa.NaiveForm. It is analogous to ssadump's -N. In the error message we could replace the error message (dead code?) with a suggestion to use -N: (dead code? try running guru with -N)

Let me know what you think :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.