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/callgraph/cha: missing edge with flag.Value.Set #23925

Closed
mvdan opened this issue Feb 19, 2018 · 4 comments
Closed

x/tools/callgraph/cha: missing edge with flag.Value.Set #23925

mvdan opened this issue Feb 19, 2018 · 4 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 Feb 19, 2018

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

go version devel +7a1347a1a1 Sun Feb 18 03:44:53 2018 +0000 linux/amd64

What did you do?

$ cat f.go
package p

import "flag"

type FlagVal string

func (f *FlagVal) Set(s string) error {
        *f = FlagVal(s)
        return nil
}

func (f *FlagVal) String() string {
        return string(*f)
}

func Foo() {
        var fv FlagVal
        flag.CommandLine.Var(&fv, "a", "b")
        flag.CommandLine.Parse([]string{"-a=x"})
        println(fv.String())
}
$ go get -u golang.org/x/tools/cmd/callgraph
$ callgraph -algo cha f.go | grep FlagVal
p.Foo   --static-20:19-->       (*p.FlagVal).String
(*fmt.pp).handleMethods --dynamic-603:25-->     (*p.FlagVal).String
runtime.printany        --dynamic-81:17-->      (*p.FlagVal).String
runtime.preprintpanics  --dynamic-404:20-->     (*p.FlagVal).String

What did you expect to see?

A call to (*p.FlagVal).Set.

Using a main package and a main func instead of Foo does not fix it.

However, using the RTA algorithm with the main package and func does work properly:

 $ callgraph -algo rta f.go | grep FlagVal
flag.isZeroValue        --dynamic-411:42-->     (*main.FlagVal).String
(*fmt.pp).handleMethods --dynamic-603:25-->     (*main.FlagVal).String
(*flag.FlagSet).Var     --dynamic-800:48-->     (*main.FlagVal).String
(*flag.FlagSet).parseOne        --dynamic-911:27-->     (*main.FlagVal).Set
main.main       --static-20:19-->       (*main.FlagVal).String

I can't figure out why CHA would not be able to find this potential call. Unless I am missing something obvious - or misunderstanding how CHA works - this seems like a bug.

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

mvdan commented Feb 19, 2018

Simpler repro:

$ cat f.go
package main

import "flag"

type Value interface {
        flag.Value
}

type FlagVal string

func (f *FlagVal) Set(s string) error {
        *f = FlagVal(s)
        return nil
}

func (f *FlagVal) String() string {
        return string(*f)
}

func main() {
        var fv FlagVal
        var v Value = &fv
        v.Set("y")
}
$ callgraph -algo cha f.go | grep FlagVal
(*fmt.pp).handleMethods --dynamic-603:25-->     (*main.FlagVal).String
runtime.printany        --dynamic-81:17-->      (*main.FlagVal).String
runtime.preprintpanics  --dynamic-404:20-->     (*main.FlagVal).String

If I change the interface type to not import the flag package, it suddenly works:

$ cat f.go
package main

type Value interface {
        Set(string) error
        String() string
}

type FlagVal string

func (f *FlagVal) Set(s string) error {
        *f = FlagVal(s)
        return nil
}

func (f *FlagVal) String() string {
        return string(*f)
}

func main() {
        var fv FlagVal
        var v Value = &fv
        v.Set("y")
}
$ callgraph -algo cha f.go | grep FlagVal
main.main       --dynamic-22:7-->       (*main.FlagVal).Set

Perhaps related to ssa (or cha) recursively working through all the program's types and functions, given how adding the flag package into the mix breaks this.

@gopherbot
Copy link

Change https://golang.org/cl/95697 mentions this issue: go/callgraph/cha: fix bug computing correspondence of abstract/concrete methods

@alandonovan
Copy link
Contributor

Thanks for the very helpful bug report.

@mvdan
Copy link
Member Author

mvdan commented Feb 21, 2018

@alandonovan thanks to you for the quick fix! Glad to see that it wasn't a bug in my program after all.

@golang golang locked and limited conversation to collaborators Feb 21, 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