code review 4958042: runtime: windows/amd64 callbacks fixed and syscall fixe...

runtime: windows/amd64 callbacks fixed and syscall fixed to allow using it in callbacks
Fixes issue 2178.
Patch2: Fixed allocating shadow space for stdcall (must be at least 32 bytes in any case)
Patch3: Made allocated chunk smaller.
Patch4: Typo
Patch5: suppress linktime warning "runtime.callbackasm: nosplit stack overflow"
Patch6: added testcase src/pkg/syscall/callback_windows_test.go
Patch7: weakly related files moved to http://codereview.appspot.com/4965050 http://codereview.appspot.com/4974041 http://codereview.appspot.com/4965051
Patch8: reflect changes http://codereview.appspot.com/4926042/
Patch9: reflect comments

[jp, 2011-08-25 04:17:09]

Here is an implementation of callback functions from windows/amd64 DLL's.
The patch is mostly modeled after the corresponding code for windows/386.
Also the wingui example made run on windows/amd64.

[brainman, 2011-08-25 04:35:18]

On 2011/08/25 04:17:09, jp wrote:
> Here is an implementation of callback functions from windows/amd64 DLL's.
> The patch is mostly modeled after the corresponding code for windows/386.
> Also the wingui example made run on windows/amd64.

It looks pretty good.

I also have http://codereview.appspot.com/4926042/ that changes similar files.
Let's wait for mine to get submitted or rejected. Some of your changes might not
be needed.

Alex

[jp, 2011-08-25 05:05:06]

You are right, we did almost the same modifications in pkg\exp\wingui\sys.s

But I see that your patch did not address the problem with syscall.NewCallback()
which does not work on windows/amd64.
My primary goal was to make it work.
exp\wingui is an example which uses syscall.NewCallback (as well as my libtiff
bindings)
Perhaps, I did not fix some issues that you did.

On 2011/08/25 04:35:18, brainman wrote:
> On 2011/08/25 04:17:09, jp wrote:
> > Here is an implementation of callback functions from windows/amd64 DLL's.
> > The patch is mostly modeled after the corresponding code for windows/386.
> > Also the wingui example made run on windows/amd64.
> 
> It looks pretty good.
> 
> I also have http://codereview.appspot.com/4926042/ that changes similar files.
> Let's wait for mine to get submitted or rejected. Some of your changes might
not
> be needed.
> 
> Alex

[brainman, 2011-08-25 05:08:38]

On 2011/08/25 05:05:06, jp wrote:
> 
> But I see that your patch did not address the problem with
syscall.NewCallback()
> which does not work on windows/amd64.
> My primary goal was to make it work.

No argument here. We just changing same asm files - I am sure we'll step on each
other toes. Lets serialize our changes <g>. Please wait.

Alex

[jp, 2011-08-25 15:33:35]

Fixed allocating shadow space for stdcall (must be at least 32 bytes in any
case)

[jp, 2011-08-25 16:18:26]

Patch3: Made allocated chunk smaller.

[jp, 2011-08-26 08:22:20]

On 2011/08/25 16:18:26, jp wrote:

added testcase for windows/386 and windows/amd64

[vcc, 2011-08-26 08:44:20]

I'd like to suggest split this CL into 3 CL, one for fixes doc/progs, one for
improve stdcall, one for callback for windows/amd64, should better for code
review. I also suggest keep thread.c and add callback.c for windows/386 and
windows/amd64.

[vcc, 2011-08-26 08:50:40]

http://codereview.appspot.com/4958042/diff/2040/src/pkg/runtime/cgo/amd64.S
File src/pkg/runtime/cgo/amd64.S (left):

http://codereview.appspot.com/4958042/diff/2040/src/pkg/runtime/cgo/amd64.S#o...
src/pkg/runtime/cgo/amd64.S:8: #if defined(__APPLE__) || defined(_WIN32)
mingw-w64 4.5.0 add underscore prefixes for C function names, 4.5.1 and up don't
add underscore.

[jp, 2011-08-26 08:51:24]

On 2011/08/26 08:44:20, vcc wrote:
> one for improve stdcall, one for callback for windows/amd64

These two are very related. 
stdcall has been fixed only because it was not reenterable and thus cannot be
called in callback context (even println does not work in callbacks without
stdcall fix).

I agree with the rest.

[jp, 2011-08-26 09:00:14]

http://codereview.appspot.com/4958042/diff/2040/src/pkg/runtime/cgo/amd64.S
File src/pkg/runtime/cgo/amd64.S (left):

http://codereview.appspot.com/4958042/diff/2040/src/pkg/runtime/cgo/amd64.S#o...
src/pkg/runtime/cgo/amd64.S:8: #if defined(__APPLE__) || defined(_WIN32)
On 2011/08/26 08:50:41, vcc wrote:
> mingw-w64 4.5.0 add underscore prefixes for C function names, 4.5.1 and up
don't
> add underscore. 

change it to:

#if defined(__APPLE__) || (defined(_WIN32) && defined(__GNUC__) &&
(__GNUC__*10000+__GNUC_MINOR__*100+__GNUC_PATCHLEVEL__) < 40501)

?

[jp, 2011-08-26 10:06:45]

On 2011/08/26 08:44:20, vcc wrote:
> I'd like to suggest split this CL into 3 CL
done

[jp, 2011-08-27 15:50:21]

On 2011/08/26 10:06:45, jp wrote:
> On 2011/08/26 08:44:20, vcc wrote:
> > I'd like to suggest split this CL into 3 CL
> done

This CL have to be redesigned after approving
http://codereview.appspot.com/4926042/ which changes same functions.
Please stop reviewing for a couple of days

[jp, 2011-08-27 16:44:19]

Reflected changes http://codereview.appspot.com/4926042/
Patch became simples for 4926042 solved syscall reenterability issue, so I just
removed syscall fix from here

[brainman, 2011-08-28 23:51:46]

Thank you for fixing this.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/Makefile
File src/pkg/Makefile (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/Makefile#oldcode214
src/pkg/Makefile:214: syscall\
I am using linux. Tests fail now:

TEST FAIL syscall
make[1]: Entering directory `/root/hg/go/src/pkg/syscall'
gotest -test.short -test.timeout=120
gotest: no test files found in current directory
make[1]: *** [testshort] Error 2
make[1]: Leaving directory `/root/hg/go/src/pkg/syscall'
make: *** [syscall.testshort] Error 1

I am not sure what to do. Let's wait for someone to help with this.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:122: // store args into "shadow space"
It is, probably, silly question, but why?

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:127: // MOVQ form instead of shorter POPQ to
suppress linktime warning "unbalanced PUSH/POP"
Do not need this comment. I do not even understand what you are referring to.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:128: MOVQ	0(SP), DX	// argsize
I might be wrong, but if you say

MOVQ argsize+0(FP), DX

you won't need the comment.
Same for the next line.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:130: ADDQ	$16, SP
This line, I think, does need a comment. Something like "removing arguments from
stack placed by our caller". Perhaps, you will come up with better description.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:141: // the warning has no sense as it is
the callback's (os thread) stack
Again, do not understand last 2 lines.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:143: MOVQ	DI, 56(SP)
Can't you just use PUSHQ DI, instead?

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:156: 
Remove this empty line. For symmetry.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/thread.c
File src/pkg/runtime/windows/thread.c (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:323: // Will keep all callbacks in a linked
list, so they don't get garbage collected.
Please put all those back. These are the same for both 386 and amd64.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:342: runtime·compilecallback(Eface fn, bool
cleanstack)
Most of this function can be shared. Just split the code that creates actual
commands.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/syscall/callback_wind...
File src/pkg/syscall/callback_windows_test.go (right):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/syscall/callback_wind...
src/pkg/syscall/callback_windows_test.go:15: t.Error("LoadLibrary(USER32)")
s/Error/Fatal/

http://codereview.appspot.com/4958042/diff/9011/src/pkg/syscall/callback_wind...
src/pkg/syscall/callback_windows_test.go:16: return
d

http://codereview.appspot.com/4958042/diff/9011/src/pkg/syscall/callback_wind...
src/pkg/syscall/callback_windows_test.go:20:
t.Error("GetProcAddress(USER32.EnumWindows)")
s/Error/Fatal/

http://codereview.appspot.com/4958042/diff/9011/src/pkg/syscall/callback_wind...
src/pkg/syscall/callback_windows_test.go:21: return
d

http://codereview.appspot.com/4958042/diff/9011/src/pkg/syscall/callback_wind...
src/pkg/syscall/callback_windows_test.go:25:
t.Error("GetProcAddress(USER32.IsWindow)")
s/Error/Fatal/

http://codereview.appspot.com/4958042/diff/9011/src/pkg/syscall/callback_wind...
src/pkg/syscall/callback_windows_test.go:26: return
d

[jp, 2011-08-29 00:45:07]

http://codereview.appspot.com/4958042/diff/9011/src/pkg/Makefile
File src/pkg/Makefile (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/Makefile#oldcode214
src/pkg/Makefile:214: syscall\
On 2011/08/28 23:51:46, brainman wrote:
> I am using linux. Tests fail now:
> 
> TEST FAIL syscall
> make[1]: Entering directory `/root/hg/go/src/pkg/syscall'
> gotest -test.short -test.timeout=120
> gotest: no test files found in current directory
> make[1]: *** [testshort] Error 2
> make[1]: Leaving directory `/root/hg/go/src/pkg/syscall'
> make: *** [syscall.testshort] Error 1
> 
> I am not sure what to do. Let's wait for someone to help with this.

If we move those tests into runtime package, will they work?

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:122: // store args into "shadow space"
On 2011/08/28 23:51:46, brainman wrote:
> It is, probably, silly question, but why?

In order to construct args vector for cgocallback().
That is windows/amd64 calling convention.
First 4 args are in CX,DX,R8,R9, args from the 5th on are on the stack. In any
case, there is reserved but uninitialized "shadow space" for the first 4 args.
And it must have space for 4 args, even if function has 0,1,2,3,4 args.
If we want to have an old good contiguous vector of args, we have to copy the
first 4 from registers into the shadow space.
There is a space for the 4 args, but there is garbage in it. The values are in
registers.
Perhaps I have to put this explanation into source code comments.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:127: // MOVQ form instead of shorter POPQ to
suppress linktime warning "unbalanced PUSH/POP"
On 2011/08/28 23:51:46, brainman wrote:
> Do not need this comment. I do not even understand what you are referring to.

Try to change it to POPQ DX, POPQ AX and 6l will not compile it, blaming there
are more POP's than PUSH'es in same procedure.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:128: MOVQ	0(SP), DX	// argsize
On 2011/08/28 23:51:46, brainman wrote:
> I might be wrong, but if you say
> 
> MOVQ argsize+0(FP), DX

Those two are not C-convention args passed on the stack.
And runtime·callbackasm is not a C-like function, it is the common part and the
continuation of every thunk compiled by syscall.NewCallback()
The thunk is: "PUSH fn / PUSH argsize / JMP runtime·callbackasm"
Those 2 values just have to be POP'ed back, after CX and DX can be freely
changed.
It is kind of thunk size optimization (remember, we have up to 2000 thunks?),
which allows not to deal with saving registers there.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:130: ADDQ	$16, SP
On 2011/08/28 23:51:46, brainman wrote:
> This line, I think, does need a comment. Something like "removing arguments
from
> stack placed by our caller". Perhaps, you will come up with better
description.

Here is no "caller" and "arguments", the function code start in the thunk, there
are 3 instructions "above", which vary from one callback to another.
And the caller calls into there.
Caller's arguments are in CX,DX,R8,R9, then from 48+SP up.
Those two are values stored in thunk.

Full code is:
PUSHQ»  fn
PUSHQ»  argsize
MOVQ»   AX, common_continuation
JUMP    AX
common_continuation:
MOVQ»   CX, (24+0)(SP)
MOVQ»   DX, (24+8)(SP)
MOVQ»   R8, (24+16)(SP)
MOVQ»   R9, (24+24)(SP)
POPQ»   DX // argsize
POPQ»   AX // fn

Without seeing the thunk code it is difficult to understand.
Got it, I will work on comments.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:143: MOVQ	DI, 56(SP)
On 2011/08/28 23:51:46, brainman wrote:
> Can't you just use PUSHQ DI, instead?

If I would use PUSHQ (it was so in the first version) 6l will blame "nosplit
stack overflow".
I do not know how to work around of it nicely.
Second try was to encode PUSHQ DI as BYTE $0xOPC0DE.
It suppressed the warning but I've got a bad comment on this style.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/thread.c
File src/pkg/runtime/windows/thread.c (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:323: // Will keep all callbacks in a linked
list, so they don't get garbage collected.
On 2011/08/28 23:51:46, brainman wrote:
> Please put all those back. These are the same for both 386 and amd64.

The code from here is moved to 386/callback.h (with no changes) and
amd64/callback.h (with changes).
The struct declarations have to be moved into *.h if you want to reuse them for
the both platforms.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:342: runtime·compilecallback(Eface fn, bool
cleanstack)
On 2011/08/28 23:51:46, brainman wrote:
> Most of this function can be shared. Just split the code that creates actual
> commands.

I'd postpone it for the next CL.

Splitting the function into portable and non-portable part would make more
complex its further changes.
And will make unreadable changes made in this CL.

Issue 2193 will change the function. 
Also 'bool cleanstack' has sense on 386 only.

[brainman, 2011-08-29 01:48:37]

http://codereview.appspot.com/4958042/diff/9011/src/pkg/Makefile
File src/pkg/Makefile (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/Makefile#oldcode214
src/pkg/Makefile:214: syscall\
On 2011/08/29 00:45:07, jp wrote:
> 
> If we move those tests into runtime package, will they work?

They will. We already have other "tests" in runtime. Sounds like a plan to me.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:122: // store args into "shadow space"
On 2011/08/29 00:45:07, jp wrote:
> 
> In order to construct args vector for cgocallback().

You are correct.

> Perhaps I have to put this explanation into source code comments.

Please, do.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:127: // MOVQ form instead of shorter POPQ to
suppress linktime warning "unbalanced PUSH/POP"
On 2011/08/29 00:45:07, jp wrote:
> 
> Try to change it to POPQ DX, POPQ AX and 6l will not compile it, blaming there
> are more POP's than PUSH'es in same procedure.

Your code here looks fine, you manipulate stack and that is fine. I think your
POPs should match PUSHs, so you can't use these commands here. I still think you
comment is not needed.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:143: MOVQ	DI, 56(SP)
On 2011/08/29 00:45:07, jp wrote:
> I do not know how to work around of it nicely.

I think, you need to make sure, your PUSH commands match POP commands. It should
work here.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/thread.c
File src/pkg/runtime/windows/thread.c (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:323: // Will keep all callbacks in a linked
list, so they don't get garbage collected.
On 2011/08/29 00:45:07, jp wrote:
> The struct declarations have to be moved into *.h if you want to reuse them
for
> the both platforms.

I don't think so. The only platform dependant code here is where it creates asm
code. None of these struct should be used there.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:342: runtime·compilecallback(Eface fn, bool
cleanstack)
On 2011/08/29 00:45:07, jp wrote:
> 
> I'd postpone it for the next CL.
> 

As you wish.

[jp, 2011-08-29 02:07:06]

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:143: MOVQ	DI, 56(SP)
This workaround is for another warning.
PUSHs and POPs perfectly match here.
6l warns that more than 120 bytes of stack is used in a single function.
It blames on PUSH but not to SUB $.., SP

I'll start a new topic in golang-dev. 
Perpahs 6l authors will advice something or Makefile maintainers allow to ignore
warnings for a single file.

On 2011/08/29 01:48:37, brainman wrote:
> On 2011/08/29 00:45:07, jp wrote:
> > I do not know how to work around of it nicely.
> 
> I think, you need to make sure, your PUSH commands match POP commands. It
should
> work here.

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/thread.c
File src/pkg/runtime/windows/thread.c (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:323: // Will keep all callbacks in a linked
list, so they don't get garbage collected.
On 2011/08/29 01:48:37, brainman wrote:
> On 2011/08/29 00:45:07, jp wrote:
> > The struct declarations have to be moved into *.h if you want to reuse them
> for
> > the both platforms.
> 
> I don't think so. The only platform dependant code here is where it creates
asm
> code. None of these struct should be used there.

Asm code is created in two spots within runtime·compilecallback (calc length
then put bytes).
Do you offer to replace two copies of runtime·compilecallback with single
runtime·compilecallback and two copies of two asm-generaning functions?
We will have 5 functions instead of 2 now.

[brainman, 2011-08-29 02:17:57]

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/thread.c
File src/pkg/runtime/windows/thread.c (left):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:323: // Will keep all callbacks in a linked
list, so they don't get garbage collected.
On 2011/08/29 02:07:07, jp wrote:
> 
> Asm code is created in two spots within runtime·compilecallback (calc length
> then put bytes).

I would create single function that completes both task: calculating the size if
no buffer is nil, then filling the buffer if passed. Like MS do whit many api
that need memory allocated.

> Do you offer to replace two copies of runtime·compilecallback with single
> runtime·compilecallback and two copies of two asm-generaning functions?
> We will have 5 functions instead of 2 now.

You didn't count duplicates of everything. You have to maintain copies in sync.

Also, I suspect, this code will change
(http://code.google.com/p/go/issues/detail?id=2193).

[jp, 2011-08-29 04:08:03]

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4958042/diff/9011/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:143: MOVQ	DI, 56(SP)
Asked, lets wait which one of the styles to write massive PUSH/POP would they
recommend.
http://groups.google.com/group/golang-dev/browse_thread/thread/9311d242074ee8c1

On 2011/08/29 02:07:07, jp wrote:
> This workaround is for another warning.
> PUSHs and POPs perfectly match here.
> 6l warns that more than 120 bytes of stack is used in a single function.
> It blames on PUSH but not to SUB $.., SP
> 
> I'll start a new topic in golang-dev. 
> Perpahs 6l authors will advice something or Makefile maintainers allow to
ignore
> warnings for a single file.
> 
> On 2011/08/29 01:48:37, brainman wrote:
> > On 2011/08/29 00:45:07, jp wrote:
> > > I do not know how to work around of it nicely.
> > 
> > I think, you need to make sure, your PUSH commands match POP commands. It
> should
> > work here.
>

[jp, 2011-08-30 00:15:15]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, vcc.163@gmail.com
(cc: golang-dev@googlegroups.com, hectorchu@gmail.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[brainman, 2011-08-30 06:19:13]

It LGTM. One small change and I will submit.

http://codereview.appspot.com/4958042/diff/8038/src/pkg/runtime/callback_wind...
File src/pkg/runtime/callback_windows_test.go (right):

http://codereview.appspot.com/4958042/diff/8038/src/pkg/runtime/callback_wind...
src/pkg/runtime/callback_windows_test.go:5: package runtime_test
Please, put this all into syscall_windows_test.go file that we already have. Do
not need to create new file for every new test.

[brainman, 2011-08-30 12:02:13]

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

runtime: windows/amd64 callbacks fixed and syscall fixed to allow using it in
callbacks
Fixes issue 2178.
Patch2: Fixed allocating shadow space for stdcall (must be at least 32 bytes in
any case)
Patch3: Made allocated chunk smaller.
Patch4: Typo
Patch5: suppress linktime warning "runtime.callbackasm: nosplit stack overflow"
Patch6: added testcase src/pkg/syscall/callback_windows_test.go
Patch7: weakly related files moved to http://codereview.appspot.com/4965050
http://codereview.appspot.com/4974041 http://codereview.appspot.com/4965051
Patch8: reflect changes http://codereview.appspot.com/4926042/
Patch9: reflect comments

R=golang-dev, alex.brainman, vcc.163
CC=golang-dev, hectorchu
http://codereview.appspot.com/4958042

Committer: Alex Brainman <alex.brainman@gmail.com>