code review 6242061: runtime: update field types in preparation for GC changes

runtime: update field types in preparation for GC changes

[atom, 2012-05-26 08:30:29]

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

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

[remyoudompheng, 2012-05-26 08:39:09]

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/runtime.h#new...
src/pkg/runtime/runtime.h:481: void*	args[2];	// padded to actual size
deoesn't this change the size of the padding ?

[minux1, 2012-05-26 09:19:40]

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/runtime.h#new...
src/pkg/runtime/runtime.h:481: void*	args[2];	// padded to actual size
On 2012/05/26 08:39:09, remyoudompheng wrote:
> deoesn't this change the size of the padding ?
the actual size doesn't matter, see proc.c:/^runtime·deferproc

actually, imo, the comment is somewhat misleading here:
because it's not a padding in strict sense, it means that
deferproc will enlarge the structure to hold all the arguments.

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/thread_linux.c
File src/pkg/runtime/thread_linux.c (right):

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/thread_linux....
src/pkg/runtime/thread_linux.c:189:
runtime·signalstack((byte*)m->gsignal->stackguard - StackGuard, 32*1024);
also need to change thread_{darwin,freebsd,netbsd,openbsd}.c

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/traceback_x86.c
File src/pkg/runtime/traceback_x86.c (right):

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/traceback_x86...
src/pkg/runtime/traceback_x86.c:26: runtime·gentraceback(byte *pc0, byte *sp,
byte *lr0, G *g, int32 skip, uintptr *pcbuf, int32 max)
given all these casts, maybe we can just change the prototype to
use uintptr as type for sp (for consistency, maybe also for lr0)

btw, don't you also need to change traceback_arm.c?

[atom, 2012-05-26 10:30:23]

Hello rsc@golang.org, remyoudompheng@gmail.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[ality, 2012-05-26 10:41:07]

It feels like there's too much casting going on.

For example, in src/pkg/runtime/mgc0.c:/^addstackroots
you can change the sp and guard variables to be uintptrs.
Then you would only need to change the current byte*
casts to uintptr casts and add one new cast.

What you have now adds five new casts in that function.

Cheers,
  Anthony

[atom, 2012-05-26 11:18:56]

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/runtime.h#new...
src/pkg/runtime/runtime.h:481: void*	args[2];	// padded to actual size
On 2012/05/26 08:39:09, remyoudompheng wrote:
> deoesn't this change the size of the padding ?

Yes, in some cases it changes the number of bytes allocated.

I looked at the code in proc.c and found out that I was passing a wrong number
to runtime·malloc. The code always worked correctly, but in certain situations
it was by chance (not by reason).

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/thread_linux.c
File src/pkg/runtime/thread_linux.c (right):

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/thread_linux....
src/pkg/runtime/thread_linux.c:189:
runtime·signalstack((byte*)m->gsignal->stackguard - StackGuard, 32*1024);
On 2012/05/26 09:19:40, minux wrote:
> also need to change thread_{darwin,freebsd,netbsd,openbsd}.c

Done.

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/traceback_x86.c
File src/pkg/runtime/traceback_x86.c (right):

http://codereview.appspot.com/6242061/diff/2008/src/pkg/runtime/traceback_x86...
src/pkg/runtime/traceback_x86.c:26: runtime·gentraceback(byte *pc0, byte *sp,
byte *lr0, G *g, int32 skip, uintptr *pcbuf, int32 max)
On 2012/05/26 09:19:40, minux wrote:
> given all these casts, maybe we can just change the prototype to
> use uintptr as type for sp (for consistency, maybe also for lr0)

The patch which improves GC precision is big. By splitting it into smaller
pieces, the individual pieces are starting to make less sense (even to myself)
than the big patch.

[0xe2.0x9a.0x9b_gmail.com, 2012-05-26 11:21:51]

This patch is controversial. I suggest we blindly ignore all the 
controversies.

On Saturday, May 26, 2012 12:40:54 PM UTC+2, Anthony Martin wrote:
>
> It feels like there's too much casting going on. 
>
> For example, in src/pkg/runtime/mgc0.c:/^addstackroots 
> you can change the sp and guard variables to be uintptrs. 
> Then you would only need to change the current byte*  to uintptr casts and 
> add one new cast. 
>
> What you have now adds five new casts in that function. 
>
> Cheers, 
>   Anthony 
>

[rsc, 2012-05-29 18:32:03]

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/mgc0.c#newcod...
src/pkg/runtime/mgc0.c:560: if(gp->gcstack != 0) {
Please use (uintptr)nil here, just because that's what you used everywhere else.

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/proc.c#newcod...
src/pkg/runtime/proc.c:1328: if(siz > sizeof(d->args))
Maybe instead we should make the Defer say byte args[0] and simplify this?

[rsc, 2012-05-29 18:32:58]

On Sat, May 26, 2012 at 6:40 AM, Anthony Martin <ality@pbrane.org> wrote:
> It feels like there's too much casting going on.
>
> For example, in src/pkg/runtime/mgc0.c:/^addstackroots
> you can change the sp and guard variables to be uintptrs.
> Then you would only need to change the current byte*
> casts to uintptr casts and add one new cast.
>
> What you have now adds five new casts in that function.

casts are free.  I'm not worried about this.

[ality, 2012-05-30 12:16:30]

Russ Cox <rsc@golang.org> once said:
> casts are free.  I'm not worried about this.

Casts are only free for the compiler.
Programmers still have to understand
why they're present.

Though, I agree it's a minor complaint.

  Anthony

[rsc, 2012-05-30 13:06:20]

On Wed, May 30, 2012 at 8:16 AM, Anthony Martin <ality@pbrane.org> wrote:
> Casts are only free for the compiler.
> Programmers still have to understand
> why they're present.

I'm confident that everything will turn out okay in the end.

[atom, 2012-05-30 16:55:08]

Hello rsc@golang.org, remyoudompheng@gmail.com, minux.ma@gmail.com,
ality@pbrane.org (cc: golang-dev@googlegroups.com),

Please take another look.

[atom, 2012-05-30 17:03:23]

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/mgc0.c#newcod...
src/pkg/runtime/mgc0.c:560: if(gp->gcstack != 0) {
On 2012/05/29 18:32:03, rsc wrote:
> Please use (uintptr)nil here, just because that's what you used everywhere
else.

Done.

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6242061/diff/3008/src/pkg/runtime/proc.c#newcod...
src/pkg/runtime/proc.c:1328: if(siz > sizeof(d->args))
On 2012/05/29 18:32:03, rsc wrote:
> Maybe instead we should make the Defer say byte args[0] and simplify this?

8c cannot compile args[0]. I changed it to args[1], so that at least it is the
minimum count.

[rsc, 2012-05-30 17:07:37]

LGTM

[rsc, 2012-05-30 17:20:53]

This is submitted but I interrupted hg before it could send the mail.