From 927e0f9da660f87fc3ad91a359e4ba805a639bac Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 9 Sep 2013 21:06:25 -0400 Subject: [PATCH] go.tools/oracle: describe: query content of lvalues, not their address. Background: some ssa.Values represent lvalues, e.g. var g = new(string) the *ssa.Global g is a **string, the address of what users think of as the global g. Querying pts(g) returns a singleton containing the object g, a *string. What users really want to see is what that in turn points to, i.e. the label for the call to new(). This change now lets users make "indirect" pointer queries, i.e. for pts(*v) where v is an ssa.Value. The oracle makes an indirect query if the type of the ssa.Value differs from the source expression type by a pointer, i.e. it's an lvalue. In other words, we're hiding the fact that compilers (e.g. ssa) internally represent globals by their address. + Tests. This serendipitously fixed an outstanding bug mentioned in the describe.go R=crawshaw CC=golang-dev https://golang.org/cl/13532043 --- oracle/describe.go | 112 ++++++++++++----------- oracle/peers.go | 8 +- oracle/testdata/src/main/describe.go | 6 +- oracle/testdata/src/main/describe.golden | 10 +- oracle/testdata/src/main/imports.golden | 2 - pointer/api.go | 33 ++++--- pointer/gen.go | 15 ++- 7 files changed, 110 insertions(+), 76 deletions(-) diff --git a/oracle/describe.go b/oracle/describe.go index 49ce9b9d..0a932459 100644 --- a/oracle/describe.go +++ b/oracle/describe.go @@ -342,10 +342,14 @@ func ssaValueForExpr(o *oracle, path []ast.Node) (ssa.Value, error) { func describeValue(o *oracle, path []ast.Node) (*describeValueResult, error) { var expr ast.Expr + var obj types.Object switch n := path[0].(type) { case *ast.ValueSpec: // ambiguous ValueSpec containing multiple names return nil, o.errorf(n, "multiple value specification") + case *ast.Ident: + obj = o.queryPkgInfo.ObjectOf(n) + expr = n case ast.Expr: expr = n default: @@ -353,6 +357,9 @@ func describeValue(o *oracle, path []ast.Node) (*describeValueResult, error) { return nil, o.errorf(n, "unexpected AST for expr: %T", n) } + typ := o.queryPkgInfo.TypeOf(expr) + constVal := o.queryPkgInfo.ValueOf(expr) + // From this point on, we cannot fail with an error. // Failure to run the pointer analysis will be reported later. // @@ -365,63 +372,31 @@ func describeValue(o *oracle, path []ast.Node) (*describeValueResult, error) { // - ok: Pointer has non-empty points-to set // ptaErr is non-nil only in the "error:" cases. - var value ssa.Value var ptaErr error - var obj types.Object - - // Determine the ssa.Value for the expression. - if id, ok := expr.(*ast.Ident); ok { - // def/ref of func/var/const object - obj = o.queryPkgInfo.ObjectOf(id) - value, ptaErr = ssaValueForIdent(o, obj, path) - } else { - // any other expression - if o.queryPkgInfo.ValueOf(expr) == nil { // non-constant? - value, ptaErr = ssaValueForExpr(o, path) - } - } - - // Don't run pointer analysis on non-pointerlike types. - if value != nil && !pointer.CanPoint(value.Type()) { - value = nil - } - - // Run pointer analysis of the selected SSA value. var ptrs []pointerResult - if value != nil { - buildSSA(o) - o.config.QueryValues = map[ssa.Value][]pointer.Pointer{value: nil} - ptrAnalysis(o) - - // Combine the PT sets from all contexts. - pointers := o.config.QueryValues[value] - if pointers == nil { - ptaErr = fmt.Errorf("PTA did not encounter this expression (dead code?)") - } - pts := pointer.PointsToCombined(pointers) - - if _, ok := value.Type().Underlying().(*types.Interface); ok { - // Show concrete types for interface expression. - if concs := pts.ConcreteTypes(); concs.Len() > 0 { - concs.Iterate(func(conc types.Type, pta interface{}) { - combined := pointer.PointsToCombined(pta.([]pointer.Pointer)) - labels := combined.Labels() - sort.Sort(byPosAndString(labels)) // to ensure determinism - ptrs = append(ptrs, pointerResult{conc, labels}) - }) - } + // Only run pointer analysis on pointerlike expression types. + if pointer.CanPoint(typ) { + // Determine the ssa.Value for the expression. + var value ssa.Value + if obj != nil { + // def/ref of func/var/const object + value, ptaErr = ssaValueForIdent(o, obj, path) } else { - // Show labels for other expressions. - labels := pts.Labels() - sort.Sort(byPosAndString(labels)) // to ensure determinism - ptrs = append(ptrs, pointerResult{value.Type(), labels}) + // any other expression + if o.queryPkgInfo.ValueOf(path[0].(ast.Expr)) == nil { // non-constant? + value, ptaErr = ssaValueForExpr(o, path) + } + } + if value != nil { + // TODO(adonovan): IsIdentical may be too strict; + // perhaps we need is-assignable or even + // has-same-underlying-representation? + indirect := types.IsIdentical(types.NewPointer(typ), value.Type()) + + ptrs, ptaErr = describePointer(o, value, indirect) } } - sort.Sort(byTypeString(ptrs)) // to ensure determinism - - typ := o.queryPkgInfo.TypeOf(expr) - constVal := o.queryPkgInfo.ValueOf(expr) return &describeValueResult{ expr: expr, @@ -433,6 +408,41 @@ func describeValue(o *oracle, path []ast.Node) (*describeValueResult, error) { }, nil } +// describePointer runs the pointer analysis of the selected SSA value. +func describePointer(o *oracle, v ssa.Value, indirect bool) (ptrs []pointerResult, err error) { + buildSSA(o) + + // TODO(adonovan): don't run indirect pointer analysis on non-ptr-ptrlike types. + o.config.QueryValues = map[ssa.Value]pointer.Indirect{v: pointer.Indirect(indirect)} + ptrAnalysis(o) + + // Combine the PT sets from all contexts. + pointers := o.config.QueryResults[v] + if pointers == nil { + return nil, fmt.Errorf("PTA did not encounter this expression (dead code?)") + } + pts := pointer.PointsToCombined(pointers) + + if _, ok := v.Type().Underlying().(*types.Interface); ok { + // Show concrete types for interface expression. + if concs := pts.ConcreteTypes(); concs.Len() > 0 { + concs.Iterate(func(conc types.Type, pta interface{}) { + combined := pointer.PointsToCombined(pta.([]pointer.Pointer)) + labels := combined.Labels() + sort.Sort(byPosAndString(labels)) // to ensure determinism + ptrs = append(ptrs, pointerResult{conc, labels}) + }) + } + } else { + // Show labels for other expressions. + labels := pts.Labels() + sort.Sort(byPosAndString(labels)) // to ensure determinism + ptrs = append(ptrs, pointerResult{v.Type(), labels}) + } + sort.Sort(byTypeString(ptrs)) // to ensure determinism + return ptrs, nil +} + type pointerResult struct { typ types.Type // type of the pointer (always concrete) labels []*pointer.Label diff --git a/oracle/peers.go b/oracle/peers.go index 07e807e3..23dd1bef 100644 --- a/oracle/peers.go +++ b/oracle/peers.go @@ -58,11 +58,11 @@ func peers(o *oracle) (queryResult, error) { // ignore both directionality and type names. queryType := queryOp.ch.Type() queryElemType := queryType.Underlying().(*types.Chan).Elem() - channels := map[ssa.Value][]pointer.Pointer{queryOp.ch: nil} + channels := map[ssa.Value]pointer.Indirect{queryOp.ch: false} i := 0 for _, op := range ops { if types.IsIdentical(op.ch.Type().Underlying().(*types.Chan).Elem(), queryElemType) { - channels[op.ch] = nil + channels[op.ch] = false ops[i] = op i++ } @@ -74,7 +74,7 @@ func peers(o *oracle) (queryResult, error) { ptrAnalysis(o) // Combine the PT sets from all contexts. - queryChanPts := pointer.PointsToCombined(channels[queryOp.ch]) + queryChanPts := pointer.PointsToCombined(o.config.QueryResults[queryOp.ch]) // Ascertain which make(chan) labels the query's channel can alias. var makes []token.Pos @@ -86,7 +86,7 @@ func peers(o *oracle) (queryResult, error) { // Ascertain which send/receive operations can alias the same make(chan) labels. var sends, receives []token.Pos for _, op := range ops { - for _, ptr := range o.config.QueryValues[op.ch] { + for _, ptr := range o.config.QueryResults[op.ch] { if ptr != nil && ptr.PointsTo().Intersects(queryChanPts) { if op.dir == ast.SEND { sends = append(sends, op.pos) diff --git a/oracle/testdata/src/main/describe.go b/oracle/testdata/src/main/describe.go index 52629170..91d2c168 100644 --- a/oracle/testdata/src/main/describe.go +++ b/oracle/testdata/src/main/describe.go @@ -12,6 +12,8 @@ const pi = 3.141 // @describe const-def-pi "pi" const pie = cake(pi) // @describe const-def-pie "pie" const _ = pi // @describe const-ref-pi "pi" +var global = new(string) // NB: ssa.Global is indirect, i.e. **string + func main() { // @describe func-def-main "main" // func objects _ = main // @describe func-ref-main "main" @@ -27,7 +29,8 @@ func main() { // @describe func-def-main "main" anon := func() { _ = d // @describe ref-lexical-d "d" } - _ = anon // @describe ref-anon "anon" + _ = anon // @describe ref-anon "anon" + _ = global // @describe ref-global "global" // SSA affords some local flow sensitivity. var a, b int @@ -49,7 +52,6 @@ func main() { // @describe func-def-main "main" print(real(1+2i) - 3) // @describe const-expr2 "real.*3" m := map[string]*int{"a": &a} - // TODO(adonovan): fix spurious error in map-lookup,ok result. mapval, _ := m["a"] // @describe map-lookup,ok "m..a.." _ = mapval // @describe mapval "mapval" _ = m // @describe m "m" diff --git a/oracle/testdata/src/main/describe.golden b/oracle/testdata/src/main/describe.golden index 70376e71..a58caefd 100644 --- a/oracle/testdata/src/main/describe.golden +++ b/oracle/testdata/src/main/describe.golden @@ -8,6 +8,7 @@ definition of package "describe" method (describe.I) f() type cake float64 func deadcode func() + var global *string func init func() var init$guard bool func main func() @@ -74,7 +75,13 @@ defined here reference to var anon func() defined here value may point to these labels: - func@27.10 + func@29.10 + +-------- @describe ref-global -------- +reference to var global *string +defined here +value may point to these labels: + new -------- @describe var-def-x-1 -------- definition of var x *int @@ -126,7 +133,6 @@ binary - operation of constant value -2 -------- @describe map-lookup,ok -------- index expression of type (*int, bool) -no points-to information: can't locate SSA Value for expression in describe.main -------- @describe mapval -------- reference to var mapval *int diff --git a/oracle/testdata/src/main/imports.golden b/oracle/testdata/src/main/imports.golden index b482fb4e..fdfef240 100644 --- a/oracle/testdata/src/main/imports.golden +++ b/oracle/testdata/src/main/imports.golden @@ -17,8 +17,6 @@ defined here -------- @describe ref-var -------- reference to var Var int defined here -value may point to these labels: - lib.Var -------- @describe ref-type -------- reference to type lib.Type diff --git a/pointer/api.go b/pointer/api.go index 37c9e9e5..4a8b9264 100644 --- a/pointer/api.go +++ b/pointer/api.go @@ -64,22 +64,29 @@ type Config struct { // Print func(site *ssa.CallCommon, p Pointer) - // The client populates QueryValues with {v, nil} for each - // ssa.Value v of interest. The pointer analysis will - // populate the corresponding map value when it creates the - // pointer variable for v. Upon completion the client can - // inspect the map for the results. + // The client populates QueryValues[v] for each ssa.Value v + // of interest. + // + // The boolean (Indirect) indicates whether to compute the + // points-to set for v (false) or *v (true): the latter is + // typically wanted for Values corresponding to source-level + // lvalues, e.g. an *ssa.Global. + // + // The pointer analysis will populate the corresponding + // QueryResults value when it creates the pointer variable + // for v or *v. Upon completion the client can inspect the + // map for the results. // // If a Value belongs to a function that the analysis treats - // context-sensitively, the corresponding slice may have - // multiple Pointers, one per distinct context. - // Use PointsToCombined to merge them. + // context-sensitively, the corresponding QueryResults slice + // may have multiple Pointers, one per distinct context. Use + // PointsToCombined to merge them. // - // TODO(adonovan): separate the keys set (input) from the - // key/value associations (result) and perhaps return the - // latter from Analyze(). + // TODO(adonovan): refactor the API: separate all results of + // Analyze() into a dedicated Result struct. // - QueryValues map[ssa.Value][]Pointer + QueryValues map[ssa.Value]Indirect + QueryResults map[ssa.Value][]Pointer // -------- Other configuration options -------- @@ -88,6 +95,8 @@ type Config struct { Log io.Writer } +type Indirect bool // map[ssa.Value]Indirect is not a set + func (c *Config) prog() *ssa.Program { for _, main := range c.Mains { return main.Prog diff --git a/pointer/gen.go b/pointer/gen.go index 0d2e1bde..2c00053a 100644 --- a/pointer/gen.go +++ b/pointer/gen.go @@ -72,9 +72,18 @@ func (a *analysis) setValueNode(v ssa.Value, id nodeid) { } // Record the (v, id) relation if the client has queried v. - qv := a.config.QueryValues - if ptrs, ok := qv[v]; ok { - qv[v] = append(ptrs, ptr{a, id}) + if indirect, ok := a.config.QueryValues[v]; ok { + if indirect { + tmp := a.addNodes(v.Type(), "query.indirect") + a.load(tmp, id, a.sizeof(v.Type())) + id = tmp + } + ptrs := a.config.QueryResults + if ptrs == nil { + ptrs = make(map[ssa.Value][]Pointer) + a.config.QueryResults = ptrs + } + ptrs[v] = append(ptrs[v], ptr{a, id}) } }