code review 7392053: exp/ssa: a number of bug fixes.

exp/ssa: a number of bug fixes.

ssadump:
- permit naming a package (not just *.go files) on command line.
- set BuildSerially flag when setting Log* flags
  (Q. should instead the logging functions take a lock?)

Builder:
- fixed bug when calling variadic function with zero '...'-params.
  Added regression test.

interp:
- more external functions:
   the 'error' interface
   bytes.{Equal,IndexByte}
   reflect.(Value).{Bool,NumOut,Out}
   syscall.{Close,Fstat,Read,Open,Stat,Lstat,Fstat,
     Getdents,ParseDirents,Getwd}
- permit comparisons between *Function and *closure.

With this CL, ssadump can now interpret ssadump itself (!),
loading, parsing, typing, SSA-building, and running
println("Hello, World!").  While a fmt-based equivalent still
lacks some external routines, e.g. math/big, I think there are
diminishing returns in expanding the interpreter (and
debugging it is starting to feel like "Inception").

I'm pretty confident this package is now good enough for wider use.

[adonovan, 2013-02-27 18:24:17]

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

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

[gri, 2013-02-27 19:47:49]

LGTM

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/interp/exter...
File src/pkg/exp/ssa/interp/external_unix.go (right):

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/interp/exter...
src/pkg/exp/ssa/interp/external_unix.go:14: func bytesToHost(in []value) []byte
{
not a good name - I would have expected something like ...ToBytes, ...Bytes

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/interp/exter...
src/pkg/exp/ssa/interp/external_unix.go:15: b := make([]byte, 0, len(in))
b := make([]byte, len(in))
for i := range in {
   b[i] = in[i].byte
}
return b

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/ssadump.go
File src/pkg/exp/ssa/ssadump.go (right):

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/ssadump.go#n...
src/pkg/exp/ssa/ssadump.go:130: args = args[i:]
this modifies the underlying array returned by the flag package - not a good
idea

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/ssadump.go#n...
src/pkg/exp/ssa/ssadump.go:144: // duplicate (e.g.) main functions in the same
package.
Isn't this the case now?

[adonovan, 2013-02-27 21:38:47]

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/interp/exter...
File src/pkg/exp/ssa/interp/external_unix.go (right):

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/interp/exter...
src/pkg/exp/ssa/interp/external_unix.go:14: func bytesToHost(in []value) []byte
{
On 2013/02/27 19:47:49, gri wrote:
> not a good name - I would have expected something like ...ToBytes, ...Bytes

Changed to valueToBytes; I also moved the typeassert from the callers into the
function, so the arg is now a value not a []value.

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/interp/exter...
src/pkg/exp/ssa/interp/external_unix.go:15: b := make([]byte, 0, len(in))
On 2013/02/27 19:47:49, gri wrote:
> b := make([]byte, len(in))
> for i := range in {
>    b[i] = in[i].byte
> }
> return b

Much better.

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/ssadump.go
File src/pkg/exp/ssa/ssadump.go (right):

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/ssadump.go#n...
src/pkg/exp/ssa/ssadump.go:130: args = args[i:]
On 2013/02/27 19:47:49, gri wrote:
> this modifies the underlying array returned by the flag package - not a good
> idea

No, it only modifies the slice, which is local to main.

https://codereview.appspot.com/7392053/diff/7001/src/pkg/exp/ssa/ssadump.go#n...
src/pkg/exp/ssa/ssadump.go:144: // duplicate (e.g.) main functions in the same
package.
On 2013/02/27 19:47:49, gri wrote:
> Isn't this the case now?

Only if the two occurrences appear in the same source file; if they're in
different files in the same package, no error is reported.

[adonovan, 2013-02-27 21:43:22]

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

exp/ssa: a number of bug fixes.

ssadump:
- permit naming a package (not just *.go files) on command line.
- set BuildSerially flag when setting Log* flags
  (Q. should instead the logging functions take a lock?)

Builder:
- fixed bug when calling variadic function with zero '...'-params.
  Added regression test.

interp:
- more external functions:
   the 'error' interface
   bytes.{Equal,IndexByte}
   reflect.(Value).{Bool,NumOut,Out}
   syscall.{Close,Fstat,Read,Open,Stat,Lstat,Fstat,
     Getdents,ParseDirents,Getwd}
- permit comparisons between *Function and *closure.

With this CL, ssadump can now interpret ssadump itself (!),
loading, parsing, typing, SSA-building, and running
println("Hello, World!").  While a fmt-based equivalent still
lacks some external routines, e.g. math/big, I think there are
diminishing returns in expanding the interpreter (and
debugging it is starting to feel like "Inception").

I'm pretty confident this package is now good enough for wider use.

R=gri
CC=golang-dev
https://codereview.appspot.com/7392053

[adonovan, 2013-02-27 21:55:14]

This broke darwin, freebsd.  Apologies.

Fix pending...

On 27 February 2013 16:43, <adonovan@google.com> wrote:

> *** Submitted as
>
https://code.google.com/p/go/**source/detail?r=580da0187392<https://code.goog...
>
>
> exp/ssa: a number of bug fixes.
>
> ssadump:
> - permit naming a package (not just *.go files) on command line.
> - set BuildSerially flag when setting Log* flags
>   (Q. should instead the logging functions take a lock?)
>
> Builder:
> - fixed bug when calling variadic function with zero '...'-params.
>   Added regression test.
>
> interp:
> - more external functions:
>    the 'error' interface
>    bytes.{Equal,IndexByte}
>    reflect.(Value).{Bool,NumOut,**Out}
>    syscall.{Close,Fstat,Read,**Open,Stat,Lstat,Fstat,
>      Getdents,ParseDirents,Getwd}
> - permit comparisons between *Function and *closure.
>
> With this CL, ssadump can now interpret ssadump itself (!),
> loading, parsing, typing, SSA-building, and running
> println("Hello, World!").  While a fmt-based equivalent still
> lacks some external routines, e.g. math/big, I think there are
> diminishing returns in expanding the interpreter (and
> debugging it is starting to feel like "Inception").
>
> I'm pretty confident this package is now good enough for wider use.
>
> R=gri
> CC=golang-dev
>
https://codereview.appspot.**com/7392053<https://codereview.appspot.com/7392053>
>
>
>
https://codereview.appspot.**com/7392053/<https://codereview.appspot.com/7392...
>