code review 12163043: cmd/gc: record argument size for all indirect function calls

cmd/gc: record argument size for all indirect function calls
This is required to properly unwind reflect.methodValueCall/makeFuncStub.
Fixes issue 5954.
Stats for 'go install std':
61849 total INSTCALL
24655 currently have ArgSize metadata
27278 have ArgSize metadata with this change
godoc size before: 11351888, after: 11364288

[dvyukov, 2013-07-31 13:21:16]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-07-31 14:03:06]

by "anonymous" I meant "indirect"

On Wed, Jul 31, 2013 at 5:21 PM,  <dvyukov@google.com> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> cmd/gc: record argument size for all anonymous function calls
> This is required to properly unwind
> reflect.methodValueCall/makeFuncStub.
> Fixes issue 5954.
> Stats for 'go install std':
> 61849 total INSTCALL
> 24655 currently have ArgSize metadata
> 27278 have ArgSize metadata with this change
> godoc size before: 11351888, after: 11364288
>
> Please review this at https://codereview.appspot.com/12163043/
>
> Affected files:
>   M src/cmd/5g/ggen.c
>   M src/cmd/6g/ggen.c
>   M src/cmd/8g/ggen.c
>   M src/pkg/reflect/asm_386.s
>   M src/pkg/reflect/asm_amd64.s
>   M src/pkg/reflect/asm_arm.s
>   M src/pkg/runtime/traceback_arm.c
>   M src/pkg/runtime/traceback_x86.c
>
>
> Index: src/cmd/5g/ggen.c
> ===================================================================
> --- a/src/cmd/5g/ggen.c
> +++ b/src/cmd/5g/ggen.c
> @@ -81,7 +81,12 @@
>                 setmaxarg(f->type);
>
>         arg = -1;
> -       if(f->type != T && ((f->sym != S && f->sym->pkg == runtimepkg) ||
> proc == 1 || proc == 2)) {
> +       // Most functions have a fixed-size argument block, so traceback
> uses that during unwind.
> +       // Not all, though: there are some variadic functions in package
> runtime,
> +       // and for those we emit call-specific metadata recorded by caller.
> +       // Reflect generates anonymous functions with variable argsize
> (reflect.methodValueCall/makeFuncStub),
> +       // so we do this for all anonymous functions as well.
> +       if(f->type != T && (f->sym == S || (f->sym != S && f->sym->pkg ==
> runtimepkg) || proc == 1 || proc == 2)) {
>                 arg = f->type->argwid;
>                 if(proc == 1 || proc == 2)
>                         arg += 3*widthptr;
> Index: src/cmd/6g/ggen.c
> ===================================================================
> --- a/src/cmd/6g/ggen.c
> +++ b/src/cmd/6g/ggen.c
> @@ -79,7 +79,12 @@
>                 setmaxarg(f->type);
>
>         arg = -1;
> -       if(f->type != T && ((f->sym != S && f->sym->pkg == runtimepkg) ||
> proc == 1 || proc == 2)) {
> +       // Most functions have a fixed-size argument block, so traceback
> uses that during unwind.
> +       // Not all, though: there are some variadic functions in package
> runtime,
> +       // and for those we emit call-specific metadata recorded by caller.
> +       // Reflect generates anonymous functions with variable argsize
> (reflect.methodValueCall/makeFuncStub),
> +       // so we do this for all anonymous functions as well.
> +       if(f->type != T && (f->sym == S || (f->sym != S && f->sym->pkg ==
> runtimepkg) || proc == 1 || proc == 2)) {
>                 arg = f->type->argwid;
>                 if(proc == 1 || proc == 2)
>                         arg += 2*widthptr;
> Index: src/cmd/8g/ggen.c
> ===================================================================
> --- a/src/cmd/8g/ggen.c
> +++ b/src/cmd/8g/ggen.c
> @@ -123,7 +123,12 @@
>                 setmaxarg(f->type);
>
>         arg = -1;
> -       if(f->type != T && ((f->sym != S && f->sym->pkg == runtimepkg) ||
> proc == 1 || proc == 2)) {
> +       // Most functions have a fixed-size argument block, so traceback
> uses that during unwind.
> +       // Not all, though: there are some variadic functions in package
> runtime,
> +       // and for those we emit call-specific metadata recorded by caller.
> +       // Reflect generates anonymous functions with variable argsize
> (reflect.methodValueCall/makeFuncStub),
> +       // so we do this for all anonymous functions as well.
> +       if(f->type != T && (f->sym == S || (f->sym != S && f->sym->pkg ==
> runtimepkg) || proc == 1 || proc == 2)) {
>                 arg = f->type->argwid;
>                 if(proc == 1 || proc == 2)
>                         arg += 2*widthptr;
> Index: src/pkg/reflect/asm_386.s
> ===================================================================
> --- a/src/pkg/reflect/asm_386.s
> +++ b/src/pkg/reflect/asm_386.s
> @@ -5,6 +5,7 @@
>  // makeFuncStub is the code half of the function returned by MakeFunc.
>  // See the comment on the declaration of makeFuncStub in makefunc.go
>  // for more details.
> +// No argsize here, gc generates argsize info at call site.
>  TEXT ·makeFuncStub(SB),7,$8
>         MOVL    DX, 0(SP)
>         LEAL    argframe+0(FP), CX
> @@ -15,6 +16,7 @@
>  // methodValueCall is the code half of the function returned by
> makeMethodValue.
>  // See the comment on the declaration of methodValueCall in makefunc.go
>  // for more details.
> +// No argsize here, gc generates argsize info at call site.
>  TEXT ·methodValueCall(SB),7,$8
>         MOVL    DX, 0(SP)
>         LEAL    argframe+0(FP), CX
> Index: src/pkg/reflect/asm_amd64.s
> ===================================================================
> --- a/src/pkg/reflect/asm_amd64.s
> +++ b/src/pkg/reflect/asm_amd64.s
> @@ -5,6 +5,7 @@
>  // makeFuncStub is the code half of the function returned by MakeFunc.
>  // See the comment on the declaration of makeFuncStub in makefunc.go
>  // for more details.
> +// No argsize here, gc generates argsize info at call site.
>  TEXT ·makeFuncStub(SB),7,$16
>         MOVQ    DX, 0(SP)
>         LEAQ    argframe+0(FP), CX
> @@ -15,6 +16,7 @@
>  // methodValueCall is the code half of the function returned by
> makeMethodValue.
>  // See the comment on the declaration of methodValueCall in makefunc.go
>  // for more details.
> +// No argsize here, gc generates argsize info at call site.
>  TEXT ·methodValueCall(SB),7,$16
>         MOVQ    DX, 0(SP)
>         LEAQ    argframe+0(FP), CX
> Index: src/pkg/reflect/asm_arm.s
> ===================================================================
> --- a/src/pkg/reflect/asm_arm.s
> +++ b/src/pkg/reflect/asm_arm.s
> @@ -5,6 +5,7 @@
>  // makeFuncStub is jumped to by the code generated by MakeFunc.
>  // See the comment on the declaration of makeFuncStub in makefunc.go
>  // for more details.
> +// No argsize here, gc generates argsize info at call site.
>  TEXT ·makeFuncStub(SB),7,$8
>         MOVW    R7, 4(R13)
>         MOVW    $argframe+0(FP), R1
> @@ -15,6 +16,7 @@
>  // methodValueCall is the code half of the function returned by
> makeMethodValue.
>  // See the comment on the declaration of methodValueCall in makefunc.go
>  // for more details.
> +// No argsize here, gc generates argsize info at call site.
>  TEXT ·methodValueCall(SB),7,$8
>         MOVW    R7, 4(R13)
>         MOVW    $argframe+0(FP), R1
> Index: src/pkg/runtime/traceback_arm.c
> ===================================================================
> --- a/src/pkg/runtime/traceback_arm.c
> +++ b/src/pkg/runtime/traceback_arm.c
> @@ -102,7 +102,7 @@
>                 // Most functions have a fixed-size argument block,
>                 // so we can use metadata about the function f.
>                 // Not all, though: there are some variadic functions
> -               // in package runtime, and for those we use call-specific
> +               // in package runtime and reflect, and for those we use
> call-specific
>                 // metadata recorded by f's caller.
>                 if(callback != nil || printing) {
>                         frame.argp = (byte*)frame.fp + sizeof(uintptr);
> Index: src/pkg/runtime/traceback_x86.c
> ===================================================================
> --- a/src/pkg/runtime/traceback_x86.c
> +++ b/src/pkg/runtime/traceback_x86.c
> @@ -118,7 +118,7 @@
>                 // Most functions have a fixed-size argument block,
>                 // so we can use metadata about the function f.
>                 // Not all, though: there are some variadic functions
> -               // in package runtime, and for those we use call-specific
> +               // in package runtime and reflect, and for those we use
> call-specific
>                 // metadata recorded by f's caller.
>                 if(callback != nil || printing) {
>                         frame.argp = (byte*)frame.fp;
>
>

[dvyukov, 2013-07-31 14:06:11]

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

Please take another look.

[rsc, 2013-07-31 15:20:44]

LGTM

please update the CL description and comments to say indirect

[dvyukov, 2013-07-31 15:24:58]

On 2013/07/31 15:20:44, rsc wrote:
> LGTM
> 
> please update the CL description and comments to say indirect

I've done this already 1 hour, 21 minutes ago

[rsc, 2013-07-31 15:26:37]

LGTM

[dvyukov, 2013-07-31 16:00:38]

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

cmd/gc: record argument size for all indirect function calls
This is required to properly unwind reflect.methodValueCall/makeFuncStub.
Fixes issue 5954.
Stats for 'go install std':
61849 total INSTCALL
24655 currently have ArgSize metadata
27278 have ArgSize metadata with this change
godoc size before: 11351888, after: 11364288

R=golang-dev, rsc
CC=golang-dev
https://codereview.appspot.com/12163043