code review 26970044: reflect: prevent the callXX routines from calling makeF...

reflect: prevent the callXX routines from calling makeFuncStub
and methodValueCall directly.  Instead, we inline their behavior
inside of reflect.call.

This change is required because otherwise we have a situation where
reflect.callXX calls makeFuncStub, neither of which knows the
layout of the args passed between them.  That's bad for
precise gc & stack copying.

Fixes issue 6619.

[khr, 2013-11-19 06:00:47]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-11-19 06:08:40]

On Tue, Nov 19, 2013 at 10:00 AM,  <khr@golang.org> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://khr%40golang.org@code.google.com/p/go/
>
>
> Description:
> reflect: prevent the callXX routines from calling makeFuncStub
> and methodValueCall directly.  Instead, we inline their behavior
> inside of reflect.call.
>
> This change is required because otherwise we have a situation where
> reflect.callXX calls makeFuncStub, neither of which knows the
> layout of the args passed between them.  That's bad for
> precise gc & stack copying.
>
> Fixes bug 6619.

It always was "Fixes issue NNN". Does "Fixes bug NNN" work as well?

[rsc, 2013-11-19 14:31:10]

there are a variety of syntaxes. i don't know whether fixes bug nnn will
work, but please use "Fixes issue 6619." just to be consistent with other
CLs

[iant, 2013-11-19 14:31:28]

Test?

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

https://codereview.appspot.com/26970044/diff/40001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:392: if x.code == makeFuncStubCode {
If we do implement shared libraries this kind of comparison of code pointers for
equality becomes inconvenient.  I think it will force us to use a PLT for
makeFuncStubCode which there is otherwise no need to do.  Would it work if you
wrote t.Name() == "reflect.makeFuncImpl" or something like t ==
TypeOf(&reflect.MakeFuncImpl).Elem()?

https://codereview.appspot.com/26970044/diff/40001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:393: println("short circuit makeFuncStub")
Omit debugging println.

https://codereview.appspot.com/26970044/diff/40001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:497: println("short circuit methodValueCall")
Omit debugging println.

[khr1, 2013-11-19 18:59:53]

On Tue, Nov 19, 2013 at 6:31 AM, <iant@golang.org> wrote:

> Test?
>
>
Test added.

I also have a patch (CL 28870043) which triggers a traceback on every
function entry.  Maybe I'll check that in at some point, but it isn't
something to leave on by default.


> https://codereview.appspot.com/26970044/diff/40001/src/
> pkg/reflect/value.go
> File src/pkg/reflect/value.go (right):
>
> https://codereview.appspot.com/26970044/diff/40001/src/
> pkg/reflect/value.go#newcode392
> src/pkg/reflect/value.go:392: if x.code == makeFuncStubCode {
> If we do implement shared libraries this kind of comparison of code
> pointers for equality becomes inconvenient.  I think it will force us to
> use a PLT for makeFuncStubCode which there is otherwise no need to do.
> Would it work if you wrote t.Name() == "reflect.makeFuncImpl" or
> something like t == TypeOf(&reflect.MakeFuncImpl).Elem()?
>

Well, the == test and the target function will always be in the same shared
library, right?  So I don't think this would be hard to get right even in a
shared library situation.
I don't think there's an easy way to get the name other than FuncForPC,
which is kind of expensive (and would have to change in a PLT world anyway,
I think).

I could use my new global variables to set the code field in
makefunc.go:MakeFunc and makefunc.go:makeMethodValue, if you think that
would help.


>
> https://codereview.appspot.com/26970044/diff/40001/src/
> pkg/reflect/value.go#newcode393
> src/pkg/reflect/value.go:393: println("short circuit makeFuncStub")
> Omit debugging println.
>
> https://codereview.appspot.com/26970044/diff/40001/src/
> pkg/reflect/value.go#newcode497
> src/pkg/reflect/value.go:497: println("short circuit methodValueCall")
> Omit debugging println.
>

Fixed both.


>
> https://codereview.appspot.com/26970044/
>

[iant, 2013-11-19 19:17:54]

On 2013/11/19 18:59:53, khr1 wrote:

> > src/pkg/reflect/value.go:392: if x.code == makeFuncStubCode {
> > If we do implement shared libraries this kind of comparison of code
> > pointers for equality becomes inconvenient.  I think it will force us to
> > use a PLT for makeFuncStubCode which there is otherwise no need to do.
> > Would it work if you wrote t.Name() == "reflect.makeFuncImpl" or
> > something like t == TypeOf(&reflect.MakeFuncImpl).Elem()?
> >
> 
> Well, the == test and the target function will always be in the same shared
> library, right?

Not if we permit multiple shared libraries to share a Go runtime.
I don't know if we will do that, but we may.

> I don't think there's an easy way to get the name other than FuncForPC,
> which is kind of expensive (and would have to change in a PLT world anyway,
> I think).

The function has an interface{} that has a dynamic type, and I would expect that
dynamic type to reliably be reflect.makeFuncImpl.  Am I wrong about that?

Ian

[khr1, 2013-11-19 21:27:07]

On Tue, Nov 19, 2013 at 11:17 AM, <iant@golang.org> wrote:

> On 2013/11/19 18:59:53, khr1 wrote:
>
>  > src/pkg/reflect/value.go:392: if x.code == makeFuncStubCode {
>> > If we do implement shared libraries this kind of comparison of code
>> > pointers for equality becomes inconvenient.  I think it will force
>>
> us to
>
>> > use a PLT for makeFuncStubCode which there is otherwise no need to
>>
> do.
>
>> > Would it work if you wrote t.Name() == "reflect.makeFuncImpl" or
>> > something like t == TypeOf(&reflect.MakeFuncImpl).Elem()?
>> >
>>
>
>  Well, the == test and the target function will always be in the same
>>
> shared
>
>> library, right?
>>
>
> Not if we permit multiple shared libraries to share a Go runtime.
> I don't know if we will do that, but we may.
>
>
But surely package reflect would be part of that single shared runtime?


>
>  I don't think there's an easy way to get the name other than
>>
> FuncForPC,
>
>> which is kind of expensive (and would have to change in a PLT world
>>
> anyway,
>
>> I think).
>>
>
> The function has an interface{} that has a dynamic type, and I would
> expect that
> dynamic type to reliably be reflect.makeFuncImpl.  Am I wrong about
> that?
>
>
A makeFuncImpl is the data stored at the target of a closure pointer.  The
reference to it isn't an interface{} - it isn't typed.


> Ian
>
> https://codereview.appspot.com/26970044/
>

[iant, 2013-11-19 22:38:08]

On 2013/11/19 21:27:07, khr1 wrote:
> On Tue, Nov 19, 2013 at 11:17 AM, <mailto:iant@golang.org> wrote:
> 
> > On 2013/11/19 18:59:53, khr1 wrote:
> >
> >  > src/pkg/reflect/value.go:392: if x.code == makeFuncStubCode {
> >> > If we do implement shared libraries this kind of comparison of code
> >> > pointers for equality becomes inconvenient.  I think it will force
> >>
> > us to
> >
> >> > use a PLT for makeFuncStubCode which there is otherwise no need to
> >>
> > do.
> >
> >> > Would it work if you wrote t.Name() == "reflect.makeFuncImpl" or
> >> > something like t == TypeOf(&reflect.MakeFuncImpl).Elem()?
> >> >
> >>
> >
> >  Well, the == test and the target function will always be in the same
> >>
> > shared
> >
> >> library, right?
> >>
> >
> > Not if we permit multiple shared libraries to share a Go runtime.
> > I don't know if we will do that, but we may.
> >
> >
> But surely package reflect would be part of that single shared runtime?

I was imagining multiple copies of the reflect package.  But, you're right, that
is unlikely.  Let's not worry about this.


> >  I don't think there's an easy way to get the name other than
> >>
> > FuncForPC,
> >
> >> which is kind of expensive (and would have to change in a PLT world
> >>
> > anyway,
> >
> >> I think).
> >>
> >
> > The function has an interface{} that has a dynamic type, and I would
> > expect that
> > dynamic type to reliably be reflect.makeFuncImpl.  Am I wrong about
> > that?
> >
> >
> A makeFuncImpl is the data stored at the target of a closure pointer.  The
> reference to it isn't an interface{} - it isn't typed.

Oh yeah, sorry for the noise.

Ian

[rsc, 2013-11-21 14:56:06]

LGTM

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

https://codereview.appspot.com/26970044/diff/60001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:393: println("short circuit makeFuncStub")
delete

https://codereview.appspot.com/26970044/diff/60001/src/pkg/reflect/value.go#n...
src/pkg/reflect/value.go:497: println("short circuit methodValueCall")
delete

[khr1, 2013-11-21 17:13:39]

Sorry, missed an "hg upload".  Fixed.


On Thu, Nov 21, 2013 at 6:56 AM, <rsc@golang.org> wrote:

> LGTM
>
>
>
> https://codereview.appspot.com/26970044/diff/60001/src/
> pkg/reflect/value.go
> File src/pkg/reflect/value.go (right):
>
> https://codereview.appspot.com/26970044/diff/60001/src/
> pkg/reflect/value.go#newcode393
>
> src/pkg/reflect/value.go:393: println("short circuit makeFuncStub")
> delete
>
> https://codereview.appspot.com/26970044/diff/60001/src/
> pkg/reflect/value.go#newcode497
>
> src/pkg/reflect/value.go:497: println("short circuit methodValueCall")
> delete
>
> https://codereview.appspot.com/26970044/
>

[khr, 2013-12-02 21:36:53]

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

reflect: prevent the callXX routines from calling makeFuncStub
and methodValueCall directly.  Instead, we inline their behavior
inside of reflect.call.

This change is required because otherwise we have a situation where
reflect.callXX calls makeFuncStub, neither of which knows the
layout of the args passed between them.  That's bad for
precise gc & stack copying.

Fixes issue 6619.

R=golang-dev, dvyukov, rsc, iant, khr
CC=golang-dev
https://codereview.appspot.com/26970044