code review 11374044: runtime: use new frame argument size information

runtime: use new frame argument size information

With this CL, I believe the runtime always knows
the frame size during the gc walk. There is no fallback
to "assume entire stack frame of caller" anymore.

[rsc, 2013-07-16 20:25:56]

Hello golang-dev@googlegroups.com,

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

[khr, 2013-07-16 20:58:53]

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/panic.c#new...
src/pkg/runtime/panic.c:161: runtime·deferreturn(uintptr arg0, ...)
Why is this necessary?  Deferreturn's argument area contains dead values.  Is it
because data is copied in there before the calls to popdefer/freedefer?

What argsize does the compiler annotate a deferreturn call with?

[cshapiro1, 2013-07-16 21:04:18]

There were a number of changes related to this one that have gone by.  I am
somewhat concerned about the handling of varargs routines.  Rather than having a
varargs call on the stack, we can avoid the use of varargs by passing a pointer.
 I am not sure what the design motiviation is for this change, but it would help
to have some additional context, at least in this regard.

[rsc, 2013-07-16 21:14:53]

On 2013/07/16 21:04:18, cshapiro1 wrote:
> There were a number of changes related to this one that have gone by.  I am
> somewhat concerned about the handling of varargs routines.  Rather than having
a
> varargs call on the stack, we can avoid the use of varargs by passing a
pointer.
>  I am not sure what the design motiviation is for this change, but it would
help
> to have some additional context, at least in this regard.

The runtime is now written such that it needs to know the size of the argument
frame, yet today it does not for many important cases. This CL, combined with
the 6c and 6g changes, makes the runtime actually have the answer 100% of the
time (and if not it will crash loudly).

There are a few cases, as outlined in the traceback comments:

  * If the function has a fixed-size argument block, that number is used.
  * If the function is at the top of a stack segment, stk->argsize records
    the size of the argument block. That number is used.
  * Otherwise, it's a variadic C function, and we require the caller to have
    recorded the argument size as PCDATA $0. That number is used.

In order to make that work:
  * All Go and non-variadic C functions record their argument size
    (even if textflag==7).
  * I annotated many textflag-7 assembly functions to record the true
    argument size.
  * All C function calls record the argument size at the time of the call
    as PCDATA $0 metadata.
  * All Go calls into package runtime do the same.

These last two are a bit conservative: we could limit the C calls to just 
variadic functions and we could limit the Go calls to just the ones that
we know are polymorphic.

In the longer term it is probably possible to eliminate some or all of the
variadic functions. We should have that discussion at some point.
It is not my intent today to make such a sweeping change.
I am only trying to make the traceback that is already there and depends
on having correct argument sizes work 100% of the time.

[rsc, 2013-07-16 21:15:34]

Not sure what this one didn't send.

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/panic.c#new...
src/pkg/runtime/panic.c:161: runtime·deferreturn(uintptr arg0, ...)
On 2013/07/16 20:58:53, khr wrote:
> Why is this necessary?  Deferreturn's argument area contains dead values.  Is
it
> because data is copied in there before the calls to popdefer/freedefer?
> 
> What argsize does the compiler annotate a deferreturn call with?

Having a ... means that the compiler does not annotate the deferreturn with an
arg size, so that if the runtime asks for one it will not find one, so that if
it really needs one (it should not ask), it will fail, and we can debug.

[cshapiro1, 2013-07-16 21:35:01]

On 2013/07/16 21:14:53, rsc wrote:
> On 2013/07/16 21:04:18, cshapiro1 wrote:
> > There were a number of changes related to this one that have gone by.  I am
> > somewhat concerned about the handling of varargs routines.  Rather than
having
> a
> > varargs call on the stack, we can avoid the use of varargs by passing a
> pointer.
> >  I am not sure what the design motiviation is for this change, but it would
> help
> > to have some additional context, at least in this regard.
> 
> The runtime is now written such that it needs to know the size of the argument
> frame, yet today it does not for many important cases. This CL, combined with
> the 6c and 6g changes, makes the runtime actually have the answer 100% of the
> time (and if not it will crash loudly).
> 
> There are a few cases, as outlined in the traceback comments:
> 
>   * If the function has a fixed-size argument block, that number is used.
>   * If the function is at the top of a stack segment, stk->argsize records
>     the size of the argument block. That number is used.
>   * Otherwise, it's a variadic C function, and we require the caller to have
>     recorded the argument size as PCDATA $0. That number is used.
> 
> In order to make that work:
>   * All Go and non-variadic C functions record their argument size
>     (even if textflag==7).
>   * I annotated many textflag-7 assembly functions to record the true
>     argument size.
>   * All C function calls record the argument size at the time of the call
>     as PCDATA $0 metadata.
>   * All Go calls into package runtime do the same.
> 
> These last two are a bit conservative: we could limit the C calls to just 
> variadic functions and we could limit the Go calls to just the ones that
> we know are polymorphic.
> 
> In the longer term it is probably possible to eliminate some or all of the
> variadic functions. We should have that discussion at some point.
> It is not my intent today to make such a sweeping change.
> I am only trying to make the traceback that is already there and depends
> on having correct argument sizes work 100% of the time.

Thanks, I appreciate the additional detail.

[dvyukov, 2013-07-17 09:17:16]

LGTM

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/panic.c#new...
src/pkg/runtime/panic.c:161: runtime·deferreturn(uintptr arg0, ...)
please add a comment

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/softfloat_a...
File src/pkg/runtime/softfloat_arm.c (right):

https://codereview.appspot.com/11374044/diff/8001/src/pkg/runtime/softfloat_a...
src/pkg/runtime/softfloat_arm.c:581: runtime·_sfloat2(uint32 *lr, uint32 r0,
...)
the same reason as in deferreturn?
add a short comment as well, this is tricky

[rsc, 2013-07-17 16:43:26]

I commented both.

sfloat2 is a different reason: the actual arguments are uint32 r0, uint32
r1, uint32 r2, and so on, and the author of the code was too lazy to list
them all and used ... instead.

[rsc, 2013-07-17 16:43:43]

Or I should say, used nothing instead, but I am adding the ... to make that
omission explicit.

[rsc, 2013-07-17 16:47:21]

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

runtime: use new frame argument size information

With this CL, I believe the runtime always knows
the frame size during the gc walk. There is no fallback
to "assume entire stack frame of caller" anymore.

R=golang-dev, khr, cshapiro, dvyukov
CC=golang-dev
https://codereview.appspot.com/11374044