[go] reflect: store receiver in pointer slot for reflect call
Cherry Mui (Gerrit)
Cherry Mui has uploaded this change for review.
reflect: store receiver in pointer slot for reflect call
The code comment says that the receiver doesn't need to go into
the pointer slot as it will be kept alive in this frame. But it
doesn't. There is no direct reference of rcvr or v (the receiver)
after storing the arguments. Also, it is clearer to explicitly
keep it alive.
XXX same for method value call (reflect.callMethod)?
Fixes #52800.
Change-Id: Ie3fa8e83f6ecc69d62e8bfab767314d5181f5dc0
---
M src/reflect/value.go
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/reflect/value.go b/src/reflect/value.go
index 0fc19ef..8ab0807 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -499,11 +499,10 @@
switch st := abid.call.steps[0]; st.kind {
case abiStepStack:
storeRcvr(rcvr, stackArgs)
- case abiStepIntReg, abiStepPointer:
- // Even pointers can go into the uintptr slot because
- // they'll be kept alive by the Values referenced by
- // this frame. Reflection forces these to be heap-allocated,
- // so we don't need to worry about stack copying.
+ case abiStepPointer:
+ storeRcvr(rcvr, unsafe.Pointer(®Args.Ptrs[st.ireg]))
+ fallthrough
+ case abiStepIntReg:
storeRcvr(rcvr, unsafe.Pointer(®Args.Ints[st.ireg]))
case abiStepFloatReg:
storeRcvr(rcvr, unsafe.Pointer(®Args.Floats[st.freg]))
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Patch set 1:Run-TryBot +1
Michael Knyszek (Gerrit)
Attention is currently required from: Austin Clements, Cherry Mui.
Patch set 1:Code-Review +2
2 comments:
Commit Message:
Patch Set #1, Line 15:
XXX same for method value call (reflect.callMethod)?yeah I think this needs the same treatment.
Patchset:
ugh, the bug tail on the regabi reflect implementation is long... thanks for figuring this out!!
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Austin Clements, Cherry Mui.
Cherry Mui uploaded patch set #2 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Cherry Mui, TryBot-Result+1 by Gopher Robot
reflect: store receiver in pointer slot for reflect call
The code comment says that the receiver doesn't need to go into
the pointer slot as it will be kept alive in this frame. But it
doesn't. There is no direct reference of rcvr or v (the receiver)
after storing the arguments. Also, it is clearer to explicitly
keep it alive.
XXX same for method value call (reflect.callMethod)?
Fixes #52800.
Change-Id: Ie3fa8e83f6ecc69d62e8bfab767314d5181f5dc0
---
M src/reflect/value.go
1 file changed, 34 insertions(+), 8 deletions(-)
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Austin Clements.
Patch set 2:Run-TryBot +1
Cherry Mui (Gerrit)
Attention is currently required from: Austin Clements, Cherry Mui.
Cherry Mui uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Cherry Mui
reflect: store receiver in pointer slot for reflect call
The code comment says that the receiver doesn't need to go into
the pointer slot as it will be kept alive in this frame. But it
doesn't. There is no direct reference of rcvr or v (the receiver)
after storing the arguments. Also, it is clearer to explicitly
keep it alive.
Fixes #52800.
Change-Id: Ie3fa8e83f6ecc69d62e8bfab767314d5181f5dc0
---
M src/reflect/value.go
1 file changed, 32 insertions(+), 8 deletions(-)
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Austin Clements.
2 comments:
Commit Message:
Patch Set #1, Line 15:
XXX same for method value call (reflect.callMethod)?yeah I think this needs the same treatment.
Done
File src/reflect/value.go:
Patch Set #3, Line 984:
methodRegs.Ints[st.ireg]The old code uses &Ints, which is effectively &Ints[0]. I think st.ireg should always be 0 here. If you &Ints or &Ints[0] is preferred, I can change it back.
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Austin Clements.
Patch set 3:Run-TryBot +1
Michael Knyszek (Gerrit)
Attention is currently required from: Austin Clements, Cherry Mui.
Patch set 3:Code-Review +2
1 comment:
File src/reflect/value.go:
Patch Set #3, Line 984:
methodRegs.Ints[st.ireg]
The old code uses &Ints, which is effectively &Ints[0]. I think st.ireg should always be 0 here. […]
I think that was just to avoid the zero-indexing issue. I think this is good.
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Austin Clements.
1 comment:
Patchset:
Thanks.
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Cherry Mui submitted this change.
reflect: store receiver in pointer slot for reflect call
The code comment says that the receiver doesn't need to go into
the pointer slot as it will be kept alive in this frame. But it
doesn't. There is no direct reference of rcvr or v (the receiver)
after storing the arguments. Also, it is clearer to explicitly
keep it alive.
Fixes #52800.
Change-Id: Ie3fa8e83f6ecc69d62e8bfab767314d5181f5dc0
Reviewed-on: https://go-review.googlesource.com/c/go/+/407508
Run-TryBot: Cherry Mui <cher...@google.com>
Reviewed-by: Michael Knyszek <mkny...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M src/reflect/value.go
1 file changed, 36 insertions(+), 8 deletions(-)
diff --git a/src/reflect/value.go b/src/reflect/value.go
index 0fc19ef..400f24f 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -499,11 +499,10 @@
switch st := abid.call.steps[0]; st.kind {
case abiStepStack:
storeRcvr(rcvr, stackArgs)
- case abiStepIntReg, abiStepPointer:
- // Even pointers can go into the uintptr slot because
- // they'll be kept alive by the Values referenced by
- // this frame. Reflection forces these to be heap-allocated,
- // so we don't need to worry about stack copying.
+ case abiStepPointer:
+ storeRcvr(rcvr, unsafe.Pointer(®Args.Ptrs[st.ireg]))
+ fallthrough
+ case abiStepIntReg:
storeRcvr(rcvr, unsafe.Pointer(®Args.Ints[st.ireg]))
case abiStepFloatReg:
storeRcvr(rcvr, unsafe.Pointer(®Args.Floats[st.freg]))
@@ -972,13 +971,21 @@
var methodRegs abi.RegArgs
// Deal with the receiver. It's guaranteed to only be one word in size.
- if st := methodABI.call.steps[0]; st.kind == abiStepStack {
+ switch st := methodABI.call.steps[0]; st.kind {
+ case abiStepStack:
// Only copy the receiver to the stack if the ABI says so.
// Otherwise, it'll be in a register already.
storeRcvr(rcvr, methodFrame)
- } else {
+ case abiStepPointer:
// Put the receiver in a register.
- storeRcvr(rcvr, unsafe.Pointer(&methodRegs.Ints))
+ storeRcvr(rcvr, unsafe.Pointer(&methodRegs.Ptrs[st.ireg]))
+ fallthrough
+ case abiStepIntReg:
+ storeRcvr(rcvr, unsafe.Pointer(&methodRegs.Ints[st.ireg]))
+ case abiStepFloatReg:
+ storeRcvr(rcvr, unsafe.Pointer(&methodRegs.Floats[st.freg]))
+ default:
+ panic("unknown ABI parameter kind")
}
// Translate the rest of the arguments.
To view, visit change 407508. To unsubscribe, or for help writing mail filters, visit settings.