dr2chase
commented
8 months ago @taking @adonovan @zpavlinovic
Open samuknet opened 8 months ago
dr2chase
commented
8 months ago @taking @adonovan @zpavlinovic
zpavlinovic
commented
8 months ago It seems to me that what happens is that the function exampleB.InterfaceFn is not even considered by the core cha algorithm. That is, ssautil.AllFunctions is called first to collect all functions potentially needed by the program in a linker-style fashion and this linker does not pull in exampleB.InterfaceFn.
This linker is conservative, but it will try to remove functions that clearly cannot be called. Here, it figures out that since exampleB is not passed to any interface in the program, then it cannot be called in EntryPoint. Hence, exampleB cannot be really used anywhere in this particular program.
On the other hand, exampleA is returned as an interface exampleInterface so in principle exampleA.InterfaceFn can be called in EntryPoint.
You can see this if you change the return type of newB to be the interface or you do something like this:
// NewB() returns an exampleB. Note the return type is *exampleB.
func NewB() *exampleB {
b := &exampleB{}
var i exampleInterface
i = b
fmt.Printf("%v\n", i)
return b
} 
adonovan
commented
8 months ago Thanks for the report.
This seems to be a regression introduced by my CL https://go.dev/cl/538357, which caused AllFunctions to return fewer items in this case. But fundamentally I think the problem is that CHA should not be using AllFunctions: its name is misleading, as it doesn't return all functions, it does (and always has done) a reachability analysis, which is not appropriate for CHA, which wants to return the maximal sound call graph without any reachability filtering at all.
I think the solution is for CHA to (like VTA) stop using AllFunctions.
Even more reduced example demonstrating the significance of exportedness:
package example
func EntryPoint(i I) { i.f() }
type I interface{ f() }
type A struct{}
func (*A) f() {}
func NewA() I { return new(A) }
type b struct{}
func (*b) f() {}
func Newb() *b { return new(b) }
type C struct{}
func (*C) f() {}
func NewC() *C { return new(C) }$ go run ./cmd/callgraph -algo=cha ./a.go
command-line-arguments.EntryPoint --dynamic-3:27--> (*command-line-arguments.A).f
command-line-arguments.EntryPoint --dynamic-3:27--> (*command-line-arguments.C).f
gopherbot
commented
2 months ago Change https://go.dev/cl/609281 mentions this issue: go/callgraph/cha: make CHA, VTA faster and more precise
Go version
go version go1.22.1 darwin/arm64
Output of
go envin your module/workspace:What did you do?
callgraphRun
callgraph -algo=cha .in the directory ofexample.goOutput is
What did you see happen?
Output includes just
exampleA.InterfaceFn():What did you expect to see?
Output to also include
exampleB.InterfaceFn()in addition toexampleA.InterfaceFn().Note this report is similar to this issue, but more specific to interfaces and reproduces regardless of whether the interface type is exported.
In particular:
NewA()example above.NewB()above.The issue persists regardless of whether or not the interface type is exported.
Findings so far We've identified this commit resulted in the change of behaviour.
Prior to this commit, the callgraph for
example.gocontains both interface implementations. After the commit it includes justexampleA.InterfaceFn()but notexampleB.InterfaceFn().From the commit I can see it's now expected for unexported types not to be included in the callgraph by default. Though I'd like to understand whether this change also intended to no longer include interface implementations in the callgraph in the example above.