go.tools/go/ssa: emit a recover block if the function's...

go.tools/go/ssa: emit a recover block if the function's results are unnamed

It is easier for clients to recover from panics if the recover block
is always present. Otherwise, the client has to work around the lack
of a recover block by synthesizing a zero value return.

[pcc, 2014-04-12 00:41:03]

Mainly looking for opinions -- I'll ping after we unfreeze.

[adonovan, 2014-04-18 21:06:40]

> [clients] cannot rely on the lack of a recover block to act as an indication
that the function cannot recover.

That is true, but if you need an indication that a function cannot recover, it
should be easy to code it up directly.  In fact, you can do a more precise job
than ssa builder because it isn't allowed to look across packages (due to
concurrency) and as a result, each time it calls 'defer mu.Unlock()' it must
assume conservatively that Unlock() may call recover, when in fact it does not.

This should work:

// MayRecover reports whether f contains a deferred static 
// call to a function that contains a call to recover().
func MayRecover(fn *ssa.Function) bool {
    for _, b := range fn.Blocks {
        for _, instr := range b.Instrs {  
            if instr, ok := instr.(*ssa.Defer); ok { 
                // (see comment at ssa.builder's case for ast.DeferStmt.)
                callee := instr.Call.StaticCallee()
                if callee == nil {
                    return true // dynamic call; be conservative
                }
                if callee == fn {
                    return true // recursive defer---weird; be conservative
                }
                if callee.Blocks == nil {
                    return true // external function; be conservative
                }
                if callsRecover(callee) {
                    return true
                }
                // callee definitely doesn't call recover.  Ignore it.
            }
        }
    }
    return false
}

func callsRecover(f *Function) bool { 
     /* defined as in ssa/util.go */
}

[pcc, 2014-04-18 21:26:38]

That's a good point. I now wonder if go/ssa should be trying to do
interprocedural optimization at all during the SSA build if it cannot do so
reliably.


On Fri, Apr 18, 2014 at 2:06 PM, <adonovan@google.com> wrote:

> [clients] cannot rely on the lack of a recover block to act as an
>>
> indication that the function cannot recover.
>
> That is true, but if you need an indication that a function cannot
> recover, it should be easy to code it up directly.  In fact, you can do
> a more precise job than ssa builder because it isn't allowed to look
> across packages (due to concurrency) and as a result, each time it calls
> 'defer mu.Unlock()' it must assume conservatively that Unlock() may call
> recover, when in fact it does not.
>
> This should work:
>
> // MayRecover reports whether f contains a deferred static
> // call to a function that contains a call to recover().
> func MayRecover(fn *ssa.Function) bool {
>     for _, b := range fn.Blocks {
>         for _, instr := range b.Instrs {
>             if instr, ok := instr.(*ssa.Defer); ok {
>                 // (see comment at ssa.builder's case for
> ast.DeferStmt.)
>                 callee := instr.Call.StaticCallee()
>                 if callee == nil {
>                     return true // dynamic call; be conservative
>                 }
>                 if callee == fn {
>                     return true // recursive defer---weird; be
> conservative
>                 }
>                 if callee.Blocks == nil {
>                     return true // external function; be conservative
>                 }
>                 if callsRecover(callee) {
>                     return true
>                 }
>                 // callee definitely doesn't call recover.  Ignore it.
>             }
>         }
>     }
>     return false
> }
>
> func callsRecover(f *Function) bool {
>      /* defined as in ssa/util.go */
> }
>
>
>
>
> https://codereview.appspot.com/87210044/
>

[adonovan, 2014-04-18 21:35:01]

On 18 April 2014 17:26, Peter Collingbourne <pcc@google.com> wrote:

> I now wonder if go/ssa should be trying to do interprocedural optimization
> at all during the SSA build if it cannot do so reliably.
>

Interesting question.  This optimization saves 411 instructions (out of
700K) in the standard library.  So perhaps it was premature.

[adonovan, 2014-06-11 20:28:01]

(Optimization deleted in CL https://codereview.appspot.com/105020045.)


On 18 April 2014 17:35, Alan Donovan <adonovan@google.com> wrote:

> On 18 April 2014 17:26, Peter Collingbourne <pcc@google.com> wrote:
>
>> I now wonder if go/ssa should be trying to do interprocedural
>> optimization at all during the SSA build if it cannot do so reliably.
>>
>
> Interesting question.  This optimization saves 411 instructions (out of
> 700K) in the standard library.  So perhaps it was premature.
>

[pcc, 2014-06-11 23:06:08]

Updated against trunk with a better description.

[adonovan, 2014-06-12 01:52:51]

On 2014/06/11 23:06:08, pcc wrote:
> Updated against trunk with a better description.

I would rather not add Recover blocks for the vast majority of functions that
don't need them.

Does the MayRecover predicate I proposed above not address your problem?

[pcc, 2014-06-12 02:35:29]

On 2014/06/12 01:52:51, adonovan wrote:
> On 2014/06/11 23:06:08, pcc wrote:
> > Updated against trunk with a better description.
> 
> I would rather not add Recover blocks for the vast majority of functions that
> don't need them.
> 
> Does the MayRecover predicate I proposed above not address your problem?

Oh, this is solving a different problem. This CL is intended to make the API
slightly simpler for clients to deal with by always providing a recover block if
it might be needed. This is to avoid having every client deal with unnamed
return values by synthesizing a zero value return.

If you decide not to take this CL, then we might as well delete the else branch
in createRecoverBlock as it is dead code.

[adonovan, 2014-06-13 17:05:00]

LGTM

> This CL is intended to make the API slightly simpler...

Fair point.  I forgot that we're only adding a Recover block to functions that
use defer, which is a small minority, so this CL adds only about 0.05% to the
instruction count.

Please also update the description of Recover in ssa.go from this:
    "The Recover block may contain only a load of the function's named 
     return parameters followed by a return of the loaded values."
to this:
    "The Recover block may contain only a returns statement,
     possibly preceded by a load of the function's named return
     parameters, if any."

https://codereview.appspot.com/87210044/diff/60001/go/ssa/builder.go
File go/ssa/builder.go (right):

https://codereview.appspot.com/87210044/diff/60001/go/ssa/builder.go#newcode1916
go/ssa/builder.go:1916: // resumes at the Recover block. If the panicking
function
You can delete the second and third sentences since createRecoverBlock explains
its behaviour.

[adonovan, 2014-06-13 17:07:30]

On 2014/06/13 17:05:00, adonovan wrote:
> LGTM
> 
> > This CL is intended to make the API slightly simpler...
> 
> Fair point.  I forgot that we're only adding a Recover block to functions that
> use defer, which is a small minority, so this CL adds only about 0.05% to the
> instruction count.
> 
> Please also update the description of Recover in ssa.go from this:
>     "The Recover block may contain only a load of the function's named 
>      return parameters followed by a return of the loaded values."
> to this:
>     "The Recover block may contain only a returns statement,
>      possibly preceded by a load of the function's named return
>      parameters, if any."
> 
> https://codereview.appspot.com/87210044/diff/60001/go/ssa/builder.go
> File go/ssa/builder.go (right):
> 
>
https://codereview.appspot.com/87210044/diff/60001/go/ssa/builder.go#newcode1916
> go/ssa/builder.go:1916: // resumes at the Recover block. If the panicking
> function
> You can delete the second and third sentences since createRecoverBlock
explains
> its behaviour.

Never mind, I'll do it myself since I have to submit it.

cheers
alan

[adonovan, 2014-06-13 17:08:37]

*** Submitted as
https://code.google.com/p/go/source/detail?r=6ad8bf15f79a&repo=tools ***

go.tools/go/ssa: emit a recover block if the function's results are unnamed

It is easier for clients to recover from panics if the recover block
is always present. Otherwise, the client has to work around the lack
of a recover block by synthesizing a zero value return.

LGTM=adonovan
R=adonovan
CC=golang-codereviews
https://codereview.appspot.com/87210044

Committer: Alan Donovan <adonovan@google.com>