code review 7403043: exp/ssa: add dedicated Panic instruction.

exp/ssa: add dedicated Panic instruction.

By avoiding the need for self-loops following calls to panic,
we reduce the number of basic blocks considerably.

[adonovan, 2013-02-20 22:51:09]

Hello gri@golang.org (cc: golang-dev@googlegroups.com, iant@golang.org),

I'd like you to review this change to
https://code.google.com/p/go/

[gri, 2013-02-21 03:14:13]

I get

error: old chunk mismatch

for all files in this CL (last version). Can you re-upload? Thanks.
- gri


On Wed, Feb 20, 2013 at 2:51 PM, <adonovan@google.com> wrote:
>
> Reviewers: gri,
>
> Message:
> Hello gri@golang.org (cc: golang-dev@googlegroups.com, iant@golang.org),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> exp/ssa: add dedicated Panic instruction.
>
> By avoiding the need for self-loops following calls to panic,
> we reduce the number of basic blocks considerably.
>
> Please review this at https://codereview.appspot.com/7403043/
>
> Affected files:
>   src/pkg/exp/ssa/builder.go
>   src/pkg/exp/ssa/emit.go
>   src/pkg/exp/ssa/interp/interp.go
>   src/pkg/exp/ssa/print.go
>   src/pkg/exp/ssa/sanity.go
>   src/pkg/exp/ssa/ssa.go
>
>
> Index: src/pkg/exp/ssa/builder.go
> ===================================================================
> --- a/src/pkg/exp/ssa/builder.go
> +++ b/src/pkg/exp/ssa/builder.go
> @@ -62,6 +62,7 @@
>         tInvalid    = types.Typ[types.Invalid]
>         tUntypedNil = types.Typ[types.UntypedNil]
>         tRangeIter  = &types.Basic{Name: "iter"} // the type of all
"range" iterators
> +       tEface      = new(types.Interface)
>
>         // The result type of a "select".
>         tSelect = &types.Result{Values: []*types.Var{
> @@ -511,6 +512,11 @@
>                         return intLiteral(at.Len)
>                 }
>                 // Otherwise treat as normal.
> +
> +       case "panic":
> +               fn.emit(&Panic{X: emitConv(fn, b.expr(fn, args[0]),
tEface)})
> +               fn.currentBlock = fn.newBasicBlock("unreachable")
> +               return vFalse // any non-nil Value will do
>         }
>         return nil // treat all others as a regular function call
>  }
> @@ -773,32 +779,20 @@
>                         // Type conversion, e.g. string(x) or big.Int(x)
>                         return emitConv(fn, b.expr(fn, e.Args[0]), typ)
>                 }
> -               // Call to "intrinsic" built-ins, e.g. new, make.
> -               wasPanic := false
> +               // Call to "intrinsic" built-ins, e.g. new, make, panic.
>                 if id, ok := e.Fun.(*ast.Ident); ok {
>                         obj := b.obj(id)
>                         if _, ok := fn.Prog.Builtins[obj]; ok {
>                                 if v := b.builtin(fn, id.Name, e.Args,
typ); v != nil {
>                                         return v
>                                 }
> -                               wasPanic = id.Name == "panic"
>                         }
>                 }
>                 // Regular function call.
>                 var v Call
>                 b.setCall(fn, e, &v.CallCommon)
>                 v.setType(typ)
> -               fn.emit(&v)
> -
> -               // Compile panic as if followed by for{} so that its
> -               // successor is unreachable.
> -               // TODO(adonovan): consider a dedicated Panic instruction
> -               // (in which case, don't forget Go and Defer).
> -               if wasPanic {
> -                       emitSelfLoop(fn)
> -                       fn.currentBlock = fn.newBasicBlock("unreachable")
> -               }
> -               return &v
> +               return fn.emit(&v)
>
>         case *ast.UnaryExpr:
>                 switch e.Op {
> @@ -1160,7 +1154,7 @@
>                         bptypes = append(bptypes, nil) // map
>                         bptypes = append(bptypes, nil) // key
>                 case "print", "println": // print{,ln}(any, ...any)
> -                       vt = new(types.Interface) // variadic
> +                       vt = tEface // variadic
>                         if !c.HasEllipsis {
>                                 args, varargs = args[:1], args[1:]
>                         }
> @@ -1187,7 +1181,7 @@
>                         }
>                         bptypes = append(bptypes, argType, argType)
>                 case "panic":
> -                       bptypes = append(bptypes, new(types.Interface))
> +                       bptypes = append(bptypes, tEface)
>                 case "recover":
>                         // no-op
>                 default:
> @@ -2256,14 +2250,14 @@
>
>         case *ast.GoStmt:
>                 // The "intrinsics" new/make/len/cap are forbidden here.
> -               // panic() is not forbidden, but is not (yet) an
intrinsic.
> +               // panic is treated like an ordinary function call.
>                 var v Go
>                 b.setCall(fn, s.Call, &v.CallCommon)
>                 fn.emit(&v)
>
>         case *ast.DeferStmt:
>                 // The "intrinsics" new/make/len/cap are forbidden here.
> -               // panic() is not forbidden, but is not (yet) an
intrinsic.
> +               // panic is treated like an ordinary function call.
>                 var v Defer
>                 b.setCall(fn, s.Call, &v.CallCommon)
>                 fn.emit(&v)
> Index: src/pkg/exp/ssa/emit.go
> ===================================================================
> --- a/src/pkg/exp/ssa/emit.go
> +++ b/src/pkg/exp/ssa/emit.go
> @@ -247,15 +247,3 @@
>         f.emit(&ret)
>         f.currentBlock = nil
>  }
> -
> -// emitSelfLoop emits to f a self-loop.
> -// This is a defensive measure to ensure control-flow integrity.
> -// It should never be reachable.
> -// Postcondition: f.currentBlock is nil.
> -//
> -func emitSelfLoop(f *Function) {
> -       loop := f.newBasicBlock("selfloop")
> -       emitJump(f, loop)
> -       f.currentBlock = loop
> -       emitJump(f, loop)
> -}
> Index: src/pkg/exp/ssa/interp/interp.go
> ===================================================================
> --- a/src/pkg/exp/ssa/interp/interp.go
> +++ b/src/pkg/exp/ssa/interp/interp.go
> @@ -171,6 +171,9 @@
>                 }
>                 return kReturn
>
> +       case *ssa.Panic:
> +               panic(targetPanic{fr.get(instr.X)})
> +
>         case *ssa.Send:
>                 fr.get(instr.Chan).(chan value) <-
copyVal(fr.get(instr.X))
>
> Index: src/pkg/exp/ssa/print.go
> ===================================================================
> --- a/src/pkg/exp/ssa/print.go
> +++ b/src/pkg/exp/ssa/print.go
> @@ -274,6 +274,10 @@
>         return printCall(&s.CallCommon, "go ", s)
>  }
>
> +func (s *Panic) String() string {
> +       return "panic " + relName(s.X, s)
> +}
> +
>  func (s *Ret) String() string {
>         var b bytes.Buffer
>         b.WriteString("ret")
> Index: src/pkg/exp/ssa/sanity.go
> ===================================================================
> --- a/src/pkg/exp/ssa/sanity.go
> +++ b/src/pkg/exp/ssa/sanity.go
> @@ -96,7 +96,7 @@
>
>  func (s *sanity) checkInstr(idx int, instr Instruction) {
>         switch instr := instr.(type) {
> -       case *If, *Jump, *Ret:
> +       case *If, *Jump, *Ret, *Panic:
>                 s.errorf("control flow instruction not at end of block")
>         case *Phi:
>                 if idx == 0 {
> @@ -172,6 +172,12 @@
>                 }
>                 // TODO(adonovan): check number and types of results
>
> +       case *Panic:
> +               if nsuccs := len(s.block.Succs); nsuccs != 0 {
> +                       s.errorf("Panic-terminated block has %d
successors; expected none", nsuccs)
> +                       return
> +               }
> +
>         default:
>                 s.errorf("non-control flow instruction at end of block")
>         }
> Index: src/pkg/exp/ssa/ssa.go
> ===================================================================
> --- a/src/pkg/exp/ssa/ssa.go
> +++ b/src/pkg/exp/ssa/ssa.go
> @@ -248,7 +248,7 @@
>  // An SSA basic block.
>  //
>  // The final element of Instrs is always an explicit transfer of
> -// control (If, Jump or Ret).
> +// control (If, Jump, Ret or Panic).
>  //
>  // A block may contain no Instructions only if it is unreachable,
>  // i.e. Preds is nil.  Empty blocks are typically pruned.
> @@ -835,6 +835,22 @@
>         Results []Value
>  }
>
> +// Panic initiates a panic with value X.
> +//
> +// A Panic instruction must be the last instruction of its containing
> +// BasicBlock, which must have no successors.
> +//
> +// NB: 'go panic(x)' and 'defer panic(x)' do not use this instruction;
> +// they are treated as calls to a built-in function.
> +//
> +// Example printed form:
> +//     panic t0
> +//
> +type Panic struct {
> +       anInstruction
> +       X Value // an interface{}
> +}
> +
>  // Go creates a new goroutine and calls the specified function
>  // within it.
>  //
> @@ -1118,6 +1134,7 @@
>  func (*MakeSlice) ImplementsInstruction()       {}
>  func (*MapUpdate) ImplementsInstruction()       {}
>  func (*Next) ImplementsInstruction()            {}
> +func (*Panic) ImplementsInstruction()           {}
>  func (*Phi) ImplementsInstruction()             {}
>  func (*Range) ImplementsInstruction()           {}
>  func (*Ret) ImplementsInstruction()             {}
> @@ -1220,6 +1237,10 @@
>         return append(rands, &v.Iter)
>  }
>
> +func (s *Panic) Operands(rands []*Value) []*Value {
> +       return append(rands, &s.X)
> +}
> +
>  func (v *Phi) Operands(rands []*Value) []*Value {
>         for i := range v.Edges {
>                 rands = append(rands, &v.Edges[i])
>
>

[gri, 2013-02-21 16:52:14]

LGTM

[adonovan, 2013-02-21 17:14:38]

*** Submitted as https://code.google.com/p/go/source/detail?r=a46b7f530ba9 ***

exp/ssa: add dedicated Panic instruction.

By avoiding the need for self-loops following calls to panic,
we reduce the number of basic blocks considerably.

R=gri
CC=golang-dev, iant
https://codereview.appspot.com/7403043