cmd/link handles reflect.Value.Call but not CallSlice?

[Brad Fitzpatrick]

In the linker (both oldlink and the new linker), there's this code that detects whether enough reflect is being used to go into conservative exported method retention mode:

https://github.com/golang/go/blob/8cc515ad3fe9f7f45470713ff1cd4faf323aef6a/src/cmd/link/internal/ld/deadcode2.go#L222

        callSym := ldr.Lookup("reflect.Value.Call", sym.SymVerABIInternal)
        methSym := ldr.Lookup("reflect.Value.Method", sym.SymVerABIInternal)
...

       d.reflectSeen = d.reflectSeen || (callSym != 0 && ldr.AttrReachable(callSym)) || (methSym != 0 && ldr.AttrReachable(methSym))

But I'm concerned that it's missing "reflect.Value.CallSlice". (Value.MethodByName calls Value.Method, but Call and CallSlice both call relect.Value.call, not each other)

Despite feeling uneasy about that, I've been unable to write a program that only calls reflect.Value.CallSlice without hitting the "d.reflectSeen" trigger (set by d.ldr.IsReflectMethod).

What am I missing? Or is there a bug here and I'm not clever enough with reflect to trigger it? (I've tried a number of things short of using unsafe, at which point crashes aren't interesting)

- Brad

[Ian Lance Taylor]

It's not immediately clear to me how you can detect whether or not
methods are retained without triggering method retention. How are you
determining whether methods are retained?

It's possible to write a program that uses CallSlice without setting
reflectSeen in the linker. Here is an example:
https://play.golang.org/p/AUKp57O22Gp . But I don't know how to go
from that to an actual problem.

Ian

[Russ Cox]

I don't see why the code checks for reflect.Value.Call at all. Calling a function is fine, and calling a method is impossible without reflect.Value.Method to get the method. So really the latter should be the only thing needed. Or maybe I too am confused.

Best,

Russ

[Ian Lance Taylor]

On Fri, Apr 17, 2020 at 1:25 PM Russ Cox <r...@golang.org> wrote:
>
> I don't see why the code checks for reflect.Value.Call at all. Calling a function is fine, and calling a method is impossible without reflect.Value.Method to get the method. So really the latter should be the only thing needed. Or maybe I too am confused.

Looks like Crawshaw added checking for reflect.Value.Call in
https://golang.org/cl/20483. Checks for reflect.Type.Method were
added slightly later in https://golang.org/cl/20489 based on the issue
filed at https://golang.org/issue/14740. I think that although CL
20489 refers only to reflect.Type.Method, it will also detect
reflect.Value.Method. So it may be that once we started checking for
reflect.Type.Method and reflect.Value.Method, that the check for
reflect.Value.Call was no longer needed.

Ian

[cherry]

The compiler marks function that calls reflect.Type.Method with REFLECTMETHOD attribute. The linker checks that attribute, if a function with such attribute is live, it knows reflect.Type.Method is used.

With that, I think Russ is right that we don't need to check reflect.Value.Call. Sent CL https://go-review.googlesource.com/c/go/+/228792. Thanks.

Cherry

[Ian Lance Taylor]

Note that I think it's important to also check for
reflect.Value.Method. Fortunately I think the current code
effectively checks for both reflect.Type.Method and
reflect.Value.Method. But the comments may need updating.

Ian

[Matthew Dempsky]

On Fri, Apr 17, 2020 at 9:19 PM Ian Lance Taylor <ia...@golang.org> wrote:

Note that I think it's important to also check for
reflect.Value.Method.  Fortunately I think the current code
effectively checks for both reflect.Type.Method and
reflect.Value.Method.

I'm not sure about that. The current code only checks for interface method calls, so it won't match on direct method calls to reflect.Value.Method. I also don't think it will match on method expressions. It would probably match on method values though, because the generated closure will have a direct method call.

For example: https://github.com/golang/go/issues/38515

[Russ Cox]

It's definitely the case that checking reflect.Value.Cal

is unnecessary and only papering over other bugs:
you can always replace v.Call() with v.Interface().(func())()

for an appropriate function type.

Let's drop the call Call check and find and fix what

real bugs that uncovers.

Best,

Russ

[cherry]

On Mon, Apr 20, 2020 at 12:43 PM Russ Cox <r...@golang.org> wrote:

It's definitely the case that checking reflect.Value.Cal

is unnecessary and only papering over other bugs:
you can always replace v.Call() with v.Interface().(func())()

for an appropriate function type.

Let's drop the call Call check and find and fix what

real bugs that uncovers.

Yeah. This is already done, CL https://go-review.googlesource.com/c/go/+/228792 and https://go-review.googlesource.com/c/go/+/228880 . Thanks.

 

[Russ Cox]

Thanks!