From be2647ec014c2ebdfd0966ee738aa18a29b24df9 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 27 Aug 2013 17:57:55 -0400 Subject: [PATCH] go.tools/ssa: fix bug in Program.VarValue. Before, VarValue looked for the ssa.Value for the 'var' object in the same package as the object was defined, but this is (obviously) wrong for a cross-package FieldVal selection, expr.f. The caller must provide the package containing the reference. + test. Also: - add 2 TODOs. - split builder.expr into two functions so we don't need defer, which makes panic dumps harder to read. R=golang-dev, crawshaw CC=golang-dev https://golang.org/cl/13257045 --- ssa/builder.go | 13 +++++++++---- ssa/source.go | 20 ++++++++------------ ssa/source_test.go | 6 +++--- ssa/ssadump.go | 2 +- ssa/testdata/objlookup.go | 6 ++++++ 5 files changed, 27 insertions(+), 20 deletions(-) diff --git a/ssa/builder.go b/ssa/builder.go index fe4f7d6486..36c0205311 100644 --- a/ssa/builder.go +++ b/ssa/builder.go @@ -482,18 +482,20 @@ func (b *builder) exprInPlace(fn *Function, loc lvalue, e ast.Expr) { // expr lowers a single-result expression e to SSA form, emitting code // to fn and returning the Value defined by the expression. // -func (b *builder) expr(fn *Function, e ast.Expr) (result Value) { +func (b *builder) expr(fn *Function, e ast.Expr) Value { // Is expression a constant? if v := fn.Pkg.info.ValueOf(e); v != nil { return NewConst(v, fn.Pkg.typeOf(e)) } - e = unparen(e) - + v := b.expr0(fn, e) if fn.debugInfo() { - defer func() { emitDebugRef(fn, e, result) }() + emitDebugRef(fn, e, v) } + return v +} +func (b *builder) expr0(fn *Function, e ast.Expr) Value { switch e := e.(type) { case *ast.BasicLit: panic("non-constant BasicLit") // unreachable @@ -827,6 +829,9 @@ func (b *builder) setCallFunc(fn *Function, e *ast.CallExpr, c *CallCommon) { c.Method = obj } else { // "Call"-mode call. + // TODO(adonovan): fix: in -build=G + // mode, declaredFunc panics for + // cross-package calls. c.Value = fn.Prog.declaredFunc(obj) c.Args = append(c.Args, v) } diff --git a/ssa/source.go b/ssa/source.go index 60ea25256d..eef1073a6c 100644 --- a/ssa/source.go +++ b/ssa/source.go @@ -217,8 +217,11 @@ func (prog *Program) ConstValue(obj *types.Const) *Const { // because its package was not built, the debug information was not // requested during SSA construction, or the value was optimized away. // -// ref must be the path to an ast.Ident (e.g. from -// PathEnclosingInterval), and that ident must resolve to obj. +// ref is the path to an ast.Ident (e.g. from PathEnclosingInterval), +// and that ident must resolve to obj. +// +// pkg is the package enclosing the reference. (A reference to a var +// may result in code, so we need to know where to find that code.) // // The Value of a defining (as opposed to referring) identifier is the // value assigned to it in its definition. @@ -235,7 +238,7 @@ func (prog *Program) ConstValue(obj *types.Const) *Const { // and all package-level vars. (This situation can be detected by // comparing the types of the Var and Value.) // -func (prog *Program) VarValue(obj *types.Var, ref []ast.Node) Value { +func (prog *Program) VarValue(obj *types.Var, pkg *Package, ref []ast.Node) Value { id := ref[0].(*ast.Ident) // Package-level variable? @@ -243,15 +246,8 @@ func (prog *Program) VarValue(obj *types.Var, ref []ast.Node) Value { return v.(*Global) } - // It's a local variable (or param) of some function. - - // The reference may occur inside a lexically nested function, - // so find that first. - pkg := prog.packages[obj.Pkg()] - if pkg == nil { - panic("no package for " + obj.String()) - } - + // Must be a function-local variable. + // (e.g. local, parameter, or field selection e.f) fn := EnclosingFunction(pkg, ref) if fn == nil { return nil // e.g. SSA not built diff --git a/ssa/source_test.go b/ssa/source_test.go index 090d8aa8d7..d6bd876801 100644 --- a/ssa/source_test.go +++ b/ssa/source_test.go @@ -96,7 +96,7 @@ func TestObjValueLookup(t *testing.T) { wantAddr = true exp = exp[1:] } - checkVarValue(t, prog, ref, obj, exp, wantAddr) + checkVarValue(t, prog, mainPkg, ref, obj, exp, wantAddr) } } } @@ -148,12 +148,12 @@ func checkConstValue(t *testing.T, prog *ssa.Program, obj *types.Const) { } } -func checkVarValue(t *testing.T, prog *ssa.Program, ref []ast.Node, obj *types.Var, expKind string, wantAddr bool) { +func checkVarValue(t *testing.T, prog *ssa.Program, pkg *ssa.Package, ref []ast.Node, obj *types.Var, expKind string, wantAddr bool) { // The prefix of all assertions messages. prefix := fmt.Sprintf("VarValue(%s @ L%d)", obj, prog.Fset.Position(ref[0].Pos()).Line) - v := prog.VarValue(obj, ref) + v := prog.VarValue(obj, pkg, ref) // Kind is the concrete type of the ssa Value. gotKind := "nil" diff --git a/ssa/ssadump.go b/ssa/ssadump.go index c41bdb06ec..28f71cdfbd 100644 --- a/ssa/ssadump.go +++ b/ssa/ssadump.go @@ -120,7 +120,7 @@ func main() { } prog.BuildAll() - prog.Package(info.Pkg).CreateTestMainFunction() // FIXME + prog.Package(info.Pkg).CreateTestMainFunction() // TODO(adonovan): remove hack // Run the interpreter. if *runFlag { diff --git a/ssa/testdata/objlookup.go b/ssa/testdata/objlookup.go index bf1c5a1587..9e59f61c39 100644 --- a/ssa/testdata/objlookup.go +++ b/ssa/testdata/objlookup.go @@ -16,6 +16,7 @@ package main // declaration is enough. import "fmt" +import "os" type J int @@ -131,4 +132,9 @@ func main() { select { case x := <-ch: // x::UnOp (receive) ch::MakeChan } + + // .Op is an inter-package FieldVal-selection. + var err os.PathError // err::UnOp + _ = err.Op // err::UnOp Op::Field + _ = &err.Op // &err::Alloc &Op::FieldAddr }