code review 75370043: reflect: Fix variadic arg arrangement for funcs created...

reflect: Fix variadic arg arrangement for funcs created by MakeFunc.

Short circuit for calling values funcs by MakeFunc was placed
before variadic arg rearrangement code in reflect.call.
Fixes issue 7534.

[crc, 2014-03-13 10:51:07]

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

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

[bradfitz, 2014-03-18 03:09:49]

R=iant




On Thu, Mar 13, 2014 at 3:51 AM, <carlchatfield@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: khr@golang.org),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> reflect: Fix variadic arg arrangement for funcs created by MakeFunc.
>
> Short circuit for calling values funcs by MakeFunc was placed
> before variadic arg rearrangement code in reflect.call.
> Fixes issue 7534.
>
> Please review this at https://codereview.appspot.com/75370043/
>
> Affected files (+28, -11 lines):
>   M src/pkg/reflect/all_test.go
>   M src/pkg/reflect/value.go
>
>
> Index: src/pkg/reflect/all_test.go
> ===================================================================
> --- a/src/pkg/reflect/all_test.go
> +++ b/src/pkg/reflect/all_test.go
> @@ -1512,6 +1512,23 @@
>         }
>  }
>
> +func TestMakeFuncVariadic(t *testing.T) {
> +       // Test that variadic arguments are packed into a slice and passed
> as last arg
> +       fn := func(_ int, is ...int) []int { return nil }
> +       fv := MakeFunc(TypeOf(fn), func(in []Value) []Value { return
> in[1:2] })
> +       ValueOf(&fn).Elem().Set(fv)
> +
> +       r := fv.Call([]Value{ValueOf(1), ValueOf(2),
> ValueOf(3)})[0].Interface().([]int)
> +       if r[0] != 2 || r[1] != 3 {
> +               t.Errorf("Call returned [%v, %v]; want 2, 3", r[0], r[1])
> +       }
> +
> +       r = fv.CallSlice([]Value{ValueOf(1), ValueOf([]int{2,
> 3})})[0].Interface().([]int)
> +       if r[0] != 2 || r[1] != 3 {
> +               t.Errorf("Call returned [%v, %v]; want 2, 3", r[0], r[1])
> +       }
> +}
> +
>  type Point struct {
>         x, y int
>  }
> Index: src/pkg/reflect/value.go
> ===================================================================
> --- a/src/pkg/reflect/value.go
> +++ b/src/pkg/reflect/value.go
> @@ -451,17 +451,6 @@
>                 panic("reflect.Value.Call: call of nil function")
>         }
>
> -       // If target is makeFuncStub, short circuit the unpack onto stack /
> -       // pack back into []Value for the args and return values.  Just do
> the
> -       // call directly.
> -       // We need to do this here because otherwise we have a situation
> where
> -       // reflect.callXX calls makeFuncStub, neither of which knows the
> -       // layout of the args.  That's bad for precise gc & stack copying.
> -       x := (*makeFuncImpl)(fn)
> -       if x.code == makeFuncStubCode {
> -               return x.fn(in)
> -       }
> -
>         isSlice := op == "CallSlice"
>         n := t.NumIn()
>         if isSlice {
> @@ -513,6 +502,17 @@
>                 in[n] = slice
>         }
>
> +       // If target is makeFuncStub, short circuit the unpack onto stack /
> +       // pack back into []Value for the args and return values.  Just do
> the
> +       // call directly.
> +       // We need to do this here because otherwise we have a situation
> where
> +       // reflect.callXX calls makeFuncStub, neither of which knows the
> +       // layout of the args.  That's bad for precise gc & stack copying.
> +       x := (*makeFuncImpl)(fn)
> +       if x.code == makeFuncStubCode {
> +               return x.fn(in)
> +       }
> +
>         nin := len(in)
>         if nin != t.NumIn() {
>                 panic("reflect.Value.Call: wrong argument count")
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[khr, 2014-04-04 17:40:28]

https://codereview.appspot.com/75370043/diff/40001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/75370043/diff/40001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:510: // layout of the args.  That's bad for precise gc
& stack copying.
LGTM.

Could you move this to after the arg count check just below?  It won't trigger
for users, but it is an internal consistency check for us.

[rsc, 2014-04-09 02:16:11]

I will apply Keith's suggested change as part of submitting this.

[rsc, 2014-04-09 02:35:27]

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

reflect: fix variadic arg for funcs created by MakeFunc.

Short circuit for calling values funcs by MakeFunc was placed
before variadic arg rearrangement code in reflect.call.
Fixes issue 7534.

LGTM=khr
R=golang-codereviews, bradfitz, khr, rsc
CC=golang-codereviews
https://codereview.appspot.com/75370043

Committer: Russ Cox <rsc@golang.org>

[crc, 2014-04-09 11:47:12]

Thank you. I am currently traveling and have only just seen the emails now.

Carl
2014/04/09 12:35 <rsc@golang.org>:

> *** Submitted as
> https://code.google.com/p/go/source/detail?r=c9b96e83a6b4 ***
>
> reflect: fix variadic arg for funcs created by MakeFunc.
>
> Short circuit for calling values funcs by MakeFunc was placed
> before variadic arg rearrangement code in reflect.call.
> Fixes issue 7534.
>
> LGTM=khr
> R=golang-codereviews, bradfitz, khr, rsc
> CC=golang-codereviews
> https://codereview.appspot.com/75370043
>
> Committer: Russ Cox <rsc@golang.org>
>
>
> https://codereview.appspot.com/75370043/
>