From: Alan Donovan Date: Tue, 26 Feb 2013 18:32:22 +0000 (-0500) Subject: exp/ssa: reimplement logic for field selection. X-Git-Tag: go1.1rc2~830 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=bd92dd6a5f2576fbf8144da0e531a57b4ebc8961;p=gostls13.git exp/ssa: reimplement logic for field selection. The previous approach desugared the ast.SelectorExpr to make implicit field selections explicit. But: 1) it was clunky since it required allocating temporary syntax trees. 2) it was not thread-safe since it required poking types into the shared type map for the new ASTs. 3) the desugared syntax had no place to represent the package lexically enclosing each implicit field selection, so it was as if they all occurred in the same package as the explicit field selection. This meant unexported field names changed meaning. This CL does what I should have done all along: just generate the SSA instructions directly from the original AST and the promoted field information. Also: - add logStack util for paired start/end log messages. Useful for debugging crashes. R=gri CC=golang-dev https://golang.org/cl/7395052 --- diff --git a/src/pkg/exp/ssa/builder.go b/src/pkg/exp/ssa/builder.go index 3d71a7a8de..d76d1ffa75 100644 --- a/src/pkg/exp/ssa/builder.go +++ b/src/pkg/exp/ssa/builder.go @@ -23,8 +23,6 @@ package ssa // The Builder's and Program's indices (maps) are populated and // mutated during the CREATE phase, but during the BUILD phase they // remain constant, with the following exceptions: -// - demoteSelector mutates Builder.types during the BUILD phase. -// TODO(adonovan): fix: let's not do that. // - globalValueSpec mutates Builder.nTo1Vars. // TODO(adonovan): make this a per-Package map so it's thread-safe. // - Program.methodSets is populated lazily across phases. @@ -517,63 +515,100 @@ func (b *Builder) builtin(fn *Function, name string, args []ast.Expr, typ types. return nil // treat all others as a regular function call } -// demoteSelector returns a SelectorExpr syntax tree that is -// equivalent to sel but contains no selections of promoted fields. -// It returns the field index of the explicit (=outermost) selection. -// -// pkg is the package in which the reference occurs. This is -// significant because a non-exported field is considered distinct -// from a field of that name in any other package. -// -// This is a rather clunky and inefficient implementation, but it -// (a) is simple and hopefully self-evidently correct and -// (b) permits us to decouple the demotion from the code generation, -// the latter being performed in two modes: addr() for lvalues, -// expr() for rvalues. -// It does require mutation of Builder.types though; if we want to -// make the Builder concurrent we'll have to avoid that. -// TODO(adonovan): opt: emit code directly rather than desugaring the AST. -// -func (b *Builder) demoteSelector(sel *ast.SelectorExpr, pkg *Package) (sel2 *ast.SelectorExpr, index int) { - id := makeId(sel.Sel.Name, pkg.Types) - xtype := b.exprType(sel.X) - // fmt.Fprintln(os.Stderr, xtype, id) // debugging - st := underlyingType(deref(xtype)).(*types.Struct) +// selector evaluates the selector expression e and returns its value, +// or if wantAddr is true, its address, in which case escaping +// indicates whether the caller intends to use the resulting pointer +// in a potentially escaping way. +// +func (b *Builder) selector(fn *Function, e *ast.SelectorExpr, wantAddr, escaping bool) Value { + id := makeId(e.Sel.Name, fn.Pkg.Types) + st := underlyingType(deref(b.exprType(e.X))).(*types.Struct) + index := -1 for i, f := range st.Fields { if IdFromQualifiedName(f.QualifiedName) == id { - return sel, i + index = i + break + } + } + var path *anonFieldPath + if index == -1 { + // Not a named field. Use breadth-first algorithm. + path, index = findPromotedField(st, id) + if path == nil { + panic("field not found, even with promotion: " + e.Sel.Name) } } - // Not a named field. Use breadth-first algorithm. - path, index := findPromotedField(st, id) - if path == nil { - panic("field not found, even with promotion: " + sel.Sel.Name) + fieldType := b.exprType(e) + if wantAddr { + return b.fieldAddr(fn, e.X, path, index, fieldType, escaping) } + return b.fieldExpr(fn, e.X, path, index, fieldType) +} - // makeSelector(e, [C,B,A]) returns (((e.A).B).C). - // e is the original selector's base. - // This function has no free variables. - var makeSelector func(b *Builder, e ast.Expr, path *anonFieldPath) *ast.SelectorExpr - makeSelector = func(b *Builder, e ast.Expr, path *anonFieldPath) *ast.SelectorExpr { - x := e - if path.tail != nil { - x = makeSelector(b, e, path.tail) +// fieldAddr evaluates the base expression (a struct or *struct), +// applies to it any implicit field selections from path, and then +// selects the field #index of type fieldType. +// Its address is returned. +// +// (fieldType can be derived from base+index.) +// +func (b *Builder) fieldAddr(fn *Function, base ast.Expr, path *anonFieldPath, index int, fieldType types.Type, escaping bool) Value { + var x Value + if path != nil { + switch underlyingType(path.field.Type).(type) { + case *types.Struct: + x = b.fieldAddr(fn, base, path.tail, path.index, path.field.Type, escaping) + case *types.Pointer: + x = b.fieldExpr(fn, base, path.tail, path.index, path.field.Type) } - sel := &ast.SelectorExpr{ - X: x, - Sel: &ast.Ident{Name: path.field.Name}, + } else { + switch underlyingType(b.exprType(base)).(type) { + case *types.Struct: + x = b.addr(fn, base, escaping).(address).addr + case *types.Pointer: + x = b.expr(fn, base) } - b.types[sel] = path.field.Type // TODO(adonovan): opt: not thread-safe - return sel } + v := &FieldAddr{ + X: x, + Field: index, + } + v.setType(pointer(fieldType)) + return fn.emit(v) +} - // Construct new SelectorExpr, bottom up. - sel2 = &ast.SelectorExpr{ - X: makeSelector(b, sel.X, path), - Sel: sel.Sel, +// fieldExpr evaluates the base expression (a struct or *struct), +// applies to it any implicit field selections from path, and then +// selects the field #index of type fieldType. +// Its value is returned. +// +// (fieldType can be derived from base+index.) +// +func (b *Builder) fieldExpr(fn *Function, base ast.Expr, path *anonFieldPath, index int, fieldType types.Type) Value { + var x Value + if path != nil { + x = b.fieldExpr(fn, base, path.tail, path.index, path.field.Type) + } else { + x = b.expr(fn, base) } - b.types[sel2] = b.exprType(sel) // TODO(adonovan): opt: not thread-safe - return + switch underlyingType(x.Type()).(type) { + case *types.Struct: + v := &Field{ + X: x, + Field: index, + } + v.setType(fieldType) + return fn.emit(v) + + case *types.Pointer: // *struct + v := &FieldAddr{ + X: x, + Field: index, + } + v.setType(pointer(fieldType)) + return emitLoad(fn, fn.emit(v)) + } + panic("unreachable") } // addr lowers a single-result addressable expression e to SSA form, @@ -633,20 +668,7 @@ func (b *Builder) addr(fn *Function, e ast.Expr, escaping bool) lvalue { } // e.f where e is an expression. - e, index := b.demoteSelector(e, fn.Pkg) - var x Value - switch underlyingType(b.exprType(e.X)).(type) { - case *types.Struct: - x = b.addr(fn, e.X, escaping).(address).addr - case *types.Pointer: - x = b.expr(fn, e.X) - } - v := &FieldAddr{ - X: x, - Field: index, - } - v.setType(pointer(b.exprType(e))) - return address{fn.emit(v)} + return address{b.selector(fn, e, true, escaping)} case *ast.IndexExpr: var x Value @@ -867,33 +889,19 @@ func (b *Builder) expr(fn *Function, e ast.Expr) Value { } // (*T).f or T.f, the method f from the method-set of type T. + xtype := b.exprType(e.X) if b.isType(e.X) { id := makeId(e.Sel.Name, fn.Pkg.Types) - typ := b.exprType(e.X) - if m := b.Prog.MethodSet(typ)[id]; m != nil { + if m := b.Prog.MethodSet(xtype)[id]; m != nil { return m } // T must be an interface; return method thunk. - return makeImethodThunk(b.Prog, typ, id) + return makeImethodThunk(b.Prog, xtype, id) } // e.f where e is an expression. - e, index := b.demoteSelector(e, fn.Pkg) - switch underlyingType(b.exprType(e.X)).(type) { - case *types.Struct: - // Non-addressable struct in a register. - v := &Field{ - X: b.expr(fn, e.X), - Field: index, - } - v.setType(b.exprType(e)) - return fn.emit(v) - - case *types.Pointer: // *struct - // Addressable structs; use FieldAddr and Load. - return b.addr(fn, e, false).load(fn) - } + return b.selector(fn, e, false, false) case *ast.IndexExpr: switch t := underlyingType(b.exprType(e.X)).(type) { @@ -1338,7 +1346,7 @@ func (b *Builder) globalValueSpec(init *Function, spec *ast.ValueSpec, g *Global if !b.nTo1Vars[spec] { b.nTo1Vars[spec] = true if b.mode&LogSource != 0 { - fmt.Fprintln(os.Stderr, "build globals", spec.Names) // ugly... + defer logStack("build globals %s", spec.Names)() } tuple := b.exprN(init, spec.Values[0]) rtypes := tuple.Type().(*types.Result).Values @@ -2287,6 +2295,10 @@ start: } var results []Value // Per the spec, there are three distinct cases of return. + // TODO(adonovan): fix: the design of Ret is incorrect: + // deferred procedures may modify named result locations + // after "Ret" has loaded its operands, causing the calls's + // result to change. Tricky... rethink. switch { case len(s.Results) == 0: // Return with no arguments. @@ -2412,6 +2424,10 @@ func (b *Builder) buildFunction(fn *Function) { if fn.syntax.body == nil { return // Go source function with no body (external) } + if fn.Prog.mode&LogSource != 0 { + defer logStack("build function %s @ %s", + fn.FullName(), fn.Prog.Files.Position(fn.Pos))() + } fn.start(b.idents) b.stmt(fn, fn.syntax.body) if cb := fn.currentBlock; cb != nil && (cb == fn.Blocks[0] || cb.Preds != nil) { @@ -2676,7 +2692,8 @@ func (b *Builder) buildDecl(pkg *Package, decl ast.Decl) { } else if decl.Recv == nil && id.Name == "init" { // init() block if b.mode&LogSource != 0 { - fmt.Fprintln(os.Stderr, "build init block @", b.Prog.Files.Position(decl.Pos())) + fmt.Fprintln(os.Stderr, "build init block @", + b.Prog.Files.Position(decl.Pos())) } init := pkg.Init @@ -2711,7 +2728,7 @@ func (b *Builder) BuildPackage(p *Package) { return // already done (or nothing to do) } if b.mode&LogSource != 0 { - fmt.Fprintln(os.Stderr, "build package", p.Types.Path) + defer logStack("build package %s", p.Types.Path)() } init := p.Init init.start(b.idents) diff --git a/src/pkg/exp/ssa/func.go b/src/pkg/exp/ssa/func.go index 423ae65984..eb45ee0f82 100644 --- a/src/pkg/exp/ssa/func.go +++ b/src/pkg/exp/ssa/func.go @@ -195,9 +195,6 @@ func (f *Function) addSpilledParam(obj types.Object) { // functions is skipped. // func (f *Function) start(idents map[*ast.Ident]types.Object) { - if f.Prog.mode&LogSource != 0 { - fmt.Fprintf(os.Stderr, "build function %s @ %s\n", f.FullName(), f.Prog.Files.Position(f.Pos)) - } f.currentBlock = f.newBasicBlock("entry") f.objects = make(map[types.Object]Value) // needed for some synthetics, e.g. init if f.syntax == nil { @@ -327,9 +324,6 @@ func (f *Function) finish() { if f.Prog.mode&SanityCheckFunctions != 0 { MustSanityCheck(f, nil) } - if f.Prog.mode&LogSource != 0 { - fmt.Fprintf(os.Stderr, "build function %s done\n", f.FullName()) - } } // removeNilBlocks eliminates nils from f.Blocks and updates each diff --git a/src/pkg/exp/ssa/promote.go b/src/pkg/exp/ssa/promote.go index acaf8921f5..75979e0ab3 100644 --- a/src/pkg/exp/ssa/promote.go +++ b/src/pkg/exp/ssa/promote.go @@ -263,7 +263,7 @@ func makeBridgeMethod(prog *Program, typ types.Type, cand *candidate) *Function sig.Recv = &types.Var{Name: "recv", Type: typ} if prog.mode&LogSource != 0 { - fmt.Fprintf(os.Stderr, "makeBridgeMethod %s, %s, type %s\n", typ, cand, &sig) + defer logStack("makeBridgeMethod %s, %s, type %s", typ, cand, &sig)() } fn := &Function{ @@ -361,7 +361,7 @@ func makeBridgeMethod(prog *Program, typ types.Type, cand *candidate) *Function // func makeImethodThunk(prog *Program, typ types.Type, id Id) *Function { if prog.mode&LogSource != 0 { - fmt.Fprintf(os.Stderr, "makeImethodThunk %s.%s\n", typ, id) + defer logStack("makeImethodThunk %s.%s", typ, id)() } itf := underlyingType(typ).(*types.Interface) index, meth := methodIndex(itf, itf.Methods, id) diff --git a/src/pkg/exp/ssa/ssa.go b/src/pkg/exp/ssa/ssa.go index a071535750..aba86ffa9b 100644 --- a/src/pkg/exp/ssa/ssa.go +++ b/src/pkg/exp/ssa/ssa.go @@ -945,7 +945,7 @@ type Register struct { referrers []Instruction } -// AnInstruction is a mix-in embedded by all Instructions. +// anInstruction is a mix-in embedded by all Instructions. // It provides the implementations of the Block and SetBlock methods. type anInstruction struct { Block_ *BasicBlock // the basic block of this instruction diff --git a/src/pkg/exp/ssa/util.go b/src/pkg/exp/ssa/util.go index a335e94e92..15c03d4462 100644 --- a/src/pkg/exp/ssa/util.go +++ b/src/pkg/exp/ssa/util.go @@ -6,6 +6,8 @@ import ( "fmt" "go/ast" "go/types" + "io" + "os" "reflect" ) @@ -208,3 +210,18 @@ func (p ids) Less(i, j int) bool { x.Pkg == y.Pkg && x.Name < y.Name } func (p ids) Swap(i, j int) { p[i], p[j] = p[j], p[i] } + +// logStack prints the formatted "start" message to stderr and +// returns a closure that prints the corresponding "end" message. +// Call using 'defer logStack(...)()' to show builder stack on panic. +// Don't forget trailing parens! +// +func logStack(format string, args ...interface{}) func() { + msg := fmt.Sprintf(format, args...) + io.WriteString(os.Stderr, msg) + io.WriteString(os.Stderr, "\n") + return func() { + io.WriteString(os.Stderr, msg) + io.WriteString(os.Stderr, " end\n") + } +}