code review 52250043: go.tools/ssa/interp: fix windows build.

go.tools/ssa/interp: fix windows build.

- Add missing import.
- platform-specific files syswrite() wrapper presents
  standard API to syscall.Write, even on Windows where
  first arg is a Handle.

Fixes issue 7100

[adonovan, 2014-01-14 20:45:07]

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

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

[minux1, 2014-01-14 20:47:15]

https://codereview.appspot.com/52250043/diff/40001/ssa/interp/external_window...
File ssa/interp/external_windows.go (right):

https://codereview.appspot.com/52250043/diff/40001/ssa/interp/external_window...
ssa/interp/external_windows.go:8: "syscall"
this addition alone is not enough to fix the build for windows.

windows's syscall.Write takes uintptr not an int as do the other OSes.

[adonovan, 2014-01-14 22:39:03]

On 2014/01/14 20:47:15, minux wrote:
>
https://codereview.appspot.com/52250043/diff/40001/ssa/interp/external_window...
> File ssa/interp/external_windows.go (right):
> 
>
https://codereview.appspot.com/52250043/diff/40001/ssa/interp/external_window...
> ssa/interp/external_windows.go:8: "syscall"
> this addition alone is not enough to fix the build for windows.
> 
> windows's syscall.Write takes uintptr not an int as do the other OSes.

Good catch!  PTAL.

[gri, 2014-01-14 22:49:36]

LGTM

Assuming it works...

[minux1, 2014-01-14 23:07:42]

https://codereview.appspot.com/52250043/diff/50001/ssa/interp/ops.go
File ssa/interp/ops.go (left):

https://codereview.appspot.com/52250043/diff/50001/ssa/interp/ops.go#oldcode918
ssa/interp/ops.go:918: return syscall.Write(fd, b)
i think we can keep this function here and just add a wrapper for syscall.Write
to external_GOOS.go.

[adonovan, 2014-01-14 23:29:24]

On 2014/01/14 23:07:42, minux wrote:
> https://codereview.appspot.com/52250043/diff/50001/ssa/interp/ops.go
> File ssa/interp/ops.go (left):
> 
>
https://codereview.appspot.com/52250043/diff/50001/ssa/interp/ops.go#oldcode918
> ssa/interp/ops.go:918: return syscall.Write(fd, b)
> i think we can keep this function here and just add a wrapper for
syscall.Write
> to external_GOOS.go.

Done, PTAL.

[minux1, 2014-01-15 00:12:53]

LGTM.

[brainman, 2014-01-15 02:01:37]

https://codereview.appspot.com/52250043/diff/70001/ssa/interp/external_window...
File ssa/interp/external_windows.go (right):

https://codereview.appspot.com/52250043/diff/70001/ssa/interp/external_window...
ssa/interp/external_windows.go:48: return syscall.Write(syscall.Handle(fd), b)
You assume that passing fd=1 here will write to stdout. I don't think this will
work. I suppose you can use syscall.Stdout for that.

You must have a reason, but why don't you use "os" package in the first place?

[brainman, 2014-01-15 02:03:13]

Also a test would be nice, so we don't have to guess, if it is correct or not.

Alex

[minux1, 2014-01-15 02:05:57]

On 2014/01/15 02:01:37, brainman wrote:
>
https://codereview.appspot.com/52250043/diff/70001/ssa/interp/external_window...
> File ssa/interp/external_windows.go (right):
> 
>
https://codereview.appspot.com/52250043/diff/70001/ssa/interp/external_window...
> ssa/interp/external_windows.go:48: return syscall.Write(syscall.Handle(fd), b)
> You assume that passing fd=1 here will write to stdout. I don't think this
will
> work. I suppose you can use syscall.Stdout for that.
Ah, yes, there must be switch in the windows implementation.
h := syscall.Handle(fd)
swtich fd {
case 1: h = os.Stdout.Fd()
case 2: h = os.Stderr.Fd()
}
> You must have a reason, but why don't you use "os" package in the first place?
after reading ops.go, i think it's possible to just migrate to
os.Std{out,err}.Write in ops.go,
so no more Write wrappers in external_GOOS.go

[minux1, 2014-01-15 02:06:56]

On 2014/01/15 02:03:13, brainman wrote:
> Also a test would be nice, so we don't have to guess, if it is correct or not.
ssa/interp doesn't work on windows yet...
we need the wrapper functions in external_windows.go.

[brainman, 2014-01-15 02:09:49]

On 2014/01/15 02:06:56, minux wrote:
> ssa/interp doesn't work on windows yet...
> we need the wrapper functions in external_windows.go.

Fair enough.

Alex

[adonovan, 2014-01-15 02:17:56]

https://codereview.appspot.com/52250043/diff/70001/ssa/interp/external_window...
File ssa/interp/external_windows.go (right):

https://codereview.appspot.com/52250043/diff/70001/ssa/interp/external_window...
ssa/interp/external_windows.go:48: return syscall.Write(syscall.Handle(fd), b)
On 2014/01/15 02:01:38, brainman wrote:
> You assume that passing fd=1 here will write to stdout. I don't think this
will
> work. I suppose you can use syscall.Stdout for that.

You're right that I make that assumption; thanks for pointing that out.
Strictly speaking. that assumption is made elsewhere in this package, not here.

> You must have a reason, but why don't you use "os" package in the first place?

Because I would still need to implement the syscall package to interpret its
uses that don't go via the os package.

> Also a test would be nice, so we don't have to guess, if it is correct or not.

There is a test, I just can't run it easily because I don't have access to a MS
WIndows machine, only POSIX (Linux + Mac).

FWIW, this whole package is essentially a test of the SSA construction
algorithm, so I'm not too worried about platform-specific interpreter problems
if they don't interfere with my ability to improve go.tools/ssa.  Also, the
motivation for this change is to fix a build breakage, not a failing test.

If any MS WIndows users are interested in using the ssa/interp package per se,
or just want to contribute build/test fixes, that would be most welcome, but
it's not a priority for me at the moment.

[brainman, 2014-01-15 02:31:49]

On 2014/01/15 02:17:56, adonovan wrote:
> ...
> > You must have a reason, but why don't you use "os" package in the first
place?
> 
> Because I would still need to implement the syscall package to interpret its
> uses that don't go via the os package.

I still don't see why you cannot use "os" package. But that is OK. I don't need
to know.

> There is a test, I just can't run it easily because I don't have access to a
MS
> WIndows machine, only POSIX (Linux + Mac).

I am happy to test things for you, if you like. Just let me know.

> ...  Also, the
> motivation for this change is to fix a build breakage, not a failing test.

Oh, the package builds alright with this CL. So you can submit, if that is your
goal.

Alex

[minux1, 2014-01-15 02:35:35]

On Tue, Jan 14, 2014 at 9:31 PM, <alex.brainman@gmail.com> wrote:
>
> Oh, the package builds alright with this CL. So you can submit, if that
> is your goal.
>
I suggest incorporating the five line switch fix before submitting this CL.
(although the ssa interpreter still won't work on windows, at least
it will be once the stubs are implemented)

[adonovan, 2014-01-15 02:42:05]

On 14 January 2014 21:35, minux <minux.ma@gmail.com> wrote:

>
> On Tue, Jan 14, 2014 at 9:31 PM, <alex.brainman@gmail.com> wrote:
>>
>> Oh, the package builds alright with this CL. So you can submit, if that
>> is your goal.
>>
> I suggest incorporating the five line switch fix before submitting this CL.
> (although the ssa interpreter still won't work on windows, at least
> it will be once the stubs are implemented)
>

As you pointed out (I had forgotten), this package has never worked on
Windows.  Though the 5-line switch would make syswrite correct for MS
Windows, it wouldn't fix the POSIX assumptions in the write() utility which
captures stdout (fd=1) and stderr (fd=2).  So I'd prefer to submit as-is,
if that's ok, since the build is fixed, and we can make the interpreter
actually run on MS Windows another day.

LG?

[brainman, 2014-01-15 02:43:25]

LGTM

Sorry. I didn't know you wait for me.

Alex

[adonovan, 2014-01-15 02:47:33]

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

go.tools/ssa/interp: fix windows build.

- Add missing import.
- platform-specific files syswrite() wrapper presents
  standard API to syscall.Write, even on Windows where
  first arg is a Handle.

Fixes issue 7100

R=gri, minux.ma, alex.brainman
CC=golang-codereviews
https://codereview.appspot.com/52250043