[go] cmd/compile: add missing usemethod checks for reflect method iterators
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: add missing usemethod checks for reflect method iterators
Required for dead-code elimination.
Fixes #77222
Change diff
diff --git a/src/cmd/compile/internal/walk/expr.go b/src/cmd/compile/internal/walk/expr.go
index 2794671..76eb7e8 100644
--- a/src/cmd/compile/internal/walk/expr.go
+++ b/src/cmd/compile/internal/walk/expr.go
@@ -1017,7 +1017,7 @@
return false
}
-// usemethod checks calls for uses of Method and MethodByName of reflect.Value,
+// usemethod checks calls for uses of Method, Methods and MethodByName of reflect.Value,
// reflect.Type, reflect.(*rtype), and reflect.(*interfaceType).
func usemethod(n *ir.CallExpr) {
// Don't mark reflect.(*rtype).Method, etc. themselves in the reflect package.
@@ -1025,12 +1025,12 @@
// alive. We only want to mark their callers.
if base.Ctxt.Pkgpath == "reflect" {
// TODO: is there a better way than hardcoding the names?
- switch fn := ir.CurFunc.Nname.Sym().Name; {
- case fn == "(*rtype).Method", fn == "(*rtype).MethodByName":
+ switch ir.CurFunc.Nname.Sym().Name {
+ case "(*rtype).Method", "(*rtype).MethodByName", "(*rtype).Methods.func1":
return
- case fn == "(*interfaceType).Method", fn == "(*interfaceType).MethodByName":
+ case "(*interfaceType).Method", "(*interfaceType).MethodByName":
return
- case fn == "Value.Method", fn == "Value.MethodByName":
+ case "Value.Method", "Value.MethodByName", "Value.Methods.func1", "(*Value).Methods.Value.Methods.func1":
return
}
}
@@ -1042,13 +1042,53 @@
// looking for either direct method calls and interface method calls of:
// reflect.Type.Method - func(int) reflect.Method
+ // reflect.Type.Methods - func() iter.Seq[reflect.Method]
// reflect.Type.MethodByName - func(string) (reflect.Method, bool)
//
// reflect.Value.Method - func(int) reflect.Value
+ // reflect.Value.Methods - func() iter.Seq2[reflect.Method, reflect.Value]
// reflect.Value.MethodByName - func(string) reflect.Value
methodName := dot.Sel.Name
t := dot.Selection.Type
+ if methodName == "Methods" && t.NumParams() == 0 && t.NumResults() == 1 {
+ // iter.Seq[V] is defined as: func(yield func(V) bool)
+ // iter.Seq2[K, V] is defined as: func(yield func(K, V) bool)
+ res := t.Result(0).Type
+ sym := res.Sym()
+ if sym == nil || sym.Pkg == nil || sym.Pkg.Path != "iter" {
+ return
+ }
+ underlying := res.Underlying()
+ if underlying.Kind() != types.TFUNC || underlying.NumParams() != 1 {
+ return
+ }
+ yieldParam := underlying.Param(0).Type
+ if yieldParam.Kind() != types.TFUNC {
+ return
+ }
+ if yieldParam.NumResults() != 1 || yieldParam.Result(0).Type.Kind() != types.TBOOL {
+ return
+ }
+ switch yieldParam.NumParams() {
+ case 1:
+ if ps := yieldParam.Param(0).Type.Sym(); ps == nil || types.ReflectSymName(ps) != "Method" {
+ return
+ }
+ case 2:
+ if ps := yieldParam.Param(0).Type.Sym(); ps == nil || types.ReflectSymName(ps) != "Method" {
+ return
+ }
+ if ps := yieldParam.Param(1).Type.Sym(); ps == nil || types.ReflectSymName(ps) != "Value" {
+ return
+ }
+ default:
+ return
+ }
+ ir.CurFunc.LSym.Set(obj.AttrReflectMethod, true)
+ return
+ }
+
// Check the number of arguments and return values.
if t.NumParams() != 1 || (t.NumResults() != 1 && t.NumResults() != 2) {
return
Change information
- M src/cmd/compile/internal/walk/expr.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// usemethod checks calls for uses of Method, Methods and MethodByName of reflect.Value,`Methods` calls `Method`. So if the former is reachable, so is the latter. The check of `Method` should suffice, and this shouldn't be necessary.
Related details
- Martin Möhrmann
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
// usemethod checks calls for uses of Method, Methods and MethodByName of reflect.Value,`Methods` calls `Method`. So if the former is reachable, so is the latter. The check of `Method` should suffice, and this shouldn't be necessary.
That's the issue isn't it? If Type.Methods is alive in the itab and never called, this causes all methods on all types to be kept by the linker. Type.Methods is being marked as <ReflectMethod>, when it shouldn't be (only external callers should be marked).
"Those functions may be alive via the itab, which should not cause all methods
alive. We only want to mark their callers."
Why wouldn't Methods need to be treated the same way? Note that Methods returns a function that calls Method (it doesn't call Method itself).
I've confirmed that after this change, -dumpdep no longer shows <ReflectMethod> when building the reproduction sample from https://go.dev/issue/77222
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
// usemethod checks calls for uses of Method, Methods and MethodByName of reflect.Value,Quentin Quaadgras`Methods` calls `Method`. So if the former is reachable, so is the latter. The check of `Method` should suffice, and this shouldn't be necessary.
That's the issue isn't it? If Type.Methods is alive in the itab and never called, this causes all methods on all types to be kept by the linker. Type.Methods is being marked as <ReflectMethod>, when it shouldn't be (only external callers should be marked).
"Those functions may be alive via the itab, which should not cause all methods
alive. We only want to mark their callers."Why wouldn't Methods need to be treated the same way? Note that Methods returns a function that calls Method (it doesn't call Method itself).
I've confirmed that after this change, -dumpdep no longer shows <ReflectMethod> when building the reproduction sample from https://go.dev/issue/77222
Type.Methods is being marked as <ReflectMethod>. when it shouldn't be
It should. Because using `Type.Methods` does make it possible to reach all methods through reflection.
(only external callers should be marked)
Why? It does not have to be "external". Call from the reflect package is can have the same effect as a call in user code. `Type.Methods` is listed in the itab doesn't necessarily make it alive (otherwise all methods on all types are alive anyway). It is only alive if it is used directly, used through an interface call, r there is a `MethodByName("Methods")` that is reachable in the program, or through some other mechanism which causes all methods alive.
E.g. https://go.dev/play/p/aHXTWkPORWV?v=gotip breaks by the CL. This example is a bit contrived, but it shows that `Methods` does need to be treated as `<ReflectMethod>`.
Note that Methods returns a function that calls Method (it doesn't call Method itself).
Sorry, I should be more precise in my previous comment. From a reachability perspective, there is no difference: if `Methods` is reachable, so is `Method`. The linker does reachability analysis.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Martin Möhrmann
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
// usemethod checks calls for uses of Method, Methods and MethodByName of reflect.Value,Quentin Quaadgras`Methods` calls `Method`. So if the former is reachable, so is the latter. The check of `Method` should suffice, and this shouldn't be necessary.
Cherry MuiThat's the issue isn't it? If Type.Methods is alive in the itab and never called, this causes all methods on all types to be kept by the linker. Type.Methods is being marked as <ReflectMethod>, when it shouldn't be (only external callers should be marked).
"Those functions may be alive via the itab, which should not cause all methods
alive. We only want to mark their callers."Why wouldn't Methods need to be treated the same way? Note that Methods returns a function that calls Method (it doesn't call Method itself).
I've confirmed that after this change, -dumpdep no longer shows <ReflectMethod> when building the reproduction sample from https://go.dev/issue/77222
Type.Methods is being marked as <ReflectMethod>. when it shouldn't be
It should. Because using `Type.Methods` does make it possible to reach all methods through reflection.
(only external callers should be marked)
Why? It does not have to be "external". Call from the reflect package is can have the same effect as a call in user code. `Type.Methods` is listed in the itab doesn't necessarily make it alive (otherwise all methods on all types are alive anyway). It is only alive if it is used directly, used through an interface call, r there is a `MethodByName("Methods")` that is reachable in the program, or through some other mechanism which causes all methods alive.
E.g. https://go.dev/play/p/aHXTWkPORWV?v=gotip breaks by the CL. This example is a bit contrived, but it shows that `Methods` does need to be treated as `<ReflectMethod>`.
Note that Methods returns a function that calls Method (it doesn't call Method itself).
Sorry, I should be more precise in my previous comment. From a reachability perspective, there is no difference: if `Methods` is reachable, so is `Method`. The linker does reachability analysis.
Taking a look at your comment on https://go.dev/issue/77222, it does seem like this may be more of an unfortunate coincidence than a bug in e8fdfeb7 (where I introduced the iterator methods). Whew!
Why? It does not have to be "external".
Sorry, I was just meaning to say that only callers outside of this exceptional group of functions should be marked.
It should. Because using Type.Methods does make it possible to reach all methods through reflection.
The same goes for the other exceptions here, doesn't it? Are you suggesting that the entire switch case is redundant? (`TestDeadcode/ifacemethod6` and `TestDeadcode/structof_funcof` fail without the switch).
Typically, yes, that's possible but I think it should also be possible to have the expression `reflect.TypeFor[T]().Methods()` only keep alive the methods of T. In order to implement such an optimization, wouldn't this require these exceptions?
After learning about this code, I'm keen to implement something like this (which I would expect needs to go into a new CR). Do I need to open an issue on GH to discuss this change first?
Regardless, I added your example as a test-case and fixed it by specifically checking for `"Methods"` & `"Method"` constants for `MethodByName`. It's now clearer in the `-dumpdep` output that the cause of the issue is the protobuf package calling `MethodByName`.
`google.golang.org/protobuf/internal/descfmt.formatDescOpt <ReflectMethod>`
https://github.com/protocolbuffers/protobuf-go/blob/96a179180f0ad6bba9b1e7b6e38d0affb0168e9a/internal/descfmt/stringer.go#L265
Related details
- Martin Möhrmann
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |