[go] runtime: implement Pinner API for object pinning
Sven Anderson (Gerrit)
1 comment:
Patchset:
R=go1.19
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Sven Anderson has uploaded this change for review.
runtime: implement Pinner API for object pinning
Some C APIs require the use or structures that contain pointers to
buffers (iovec, io_uring, ...). The pointer passing rules would
require that these buffers are allocated in C memory and to process
this data with Go libraries it would need to be copied.
In order to provide a zero-copy way to use these C APIs, this CL
implements a Pinner API that allows to pin Go objects, which
guarantees that the garbage collector does not move these objects
while pinned. This allows to relax the pointer passing rules so that
pinned pointers can be stored in C allocated memory or can be
contained in Go memory that is passed to C functions.
Pinning is supported for pointers of any type, unsafe.Pointer,
strings and functions. Slices and arrays can be pinned by pinning the
pointer to the first element. Pinning of maps is not supported.
Behaviour decisions: If the GC collects unreachable Pinner holding
pinned objects it panics. Trying to pin the following kind of objects
panics as well:
- pinned objects
- zero-size or tiny objects
- global objects that are not allocated in the heap
- pointer not pointing to the beginning of an allocated object
Performance considerations: This change has no impact on execution
time on existing code, because checks are only done in code paths,
that would panic otherwise. The memory footprint is one pointer per
memory span.
Fixes: #46787
Signed-off-by: Sven Anderson <sv...@anderson.de>
Change-Id: I110031fe789b92277ae45a9455624687bd1c54f2
---
M src/runtime/cgocheck.go
A src/runtime/pinner_test.go
M src/runtime/cgocall.go
M src/runtime/mheap.go
M src/runtime/lockrank.go
M src/runtime/mbitmap.go
A src/runtime/pinner.go
M src/runtime/mgcmark.go
8 files changed, 593 insertions(+), 32 deletions(-)
diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go
index 694b3e6..b9f1d7f 100644
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -473,7 +473,7 @@
if indir {
p = *(*unsafe.Pointer)(p)
}
- if !cgoIsGoPointer(p) {
+ if !cgoIsGoPointer(p) || isPinned(uintptr(p)) {
return
}
panic(errorString(msg))
@@ -492,7 +492,7 @@
if !cgoIsGoPointer(p) {
return
}
- if !top {
+ if !top && !isPinned(uintptr(p)) {
panic(errorString(msg))
}
cgoCheckArg(it, p, it.kind&kindDirectIface == 0, false, msg)
@@ -503,7 +503,7 @@
if p == nil || !cgoIsGoPointer(p) {
return
}
- if !top {
+ if !top && !isPinned(uintptr(p)) {
panic(errorString(msg))
}
if st.elem.ptrdata == 0 {
@@ -518,7 +518,7 @@
if !cgoIsGoPointer(ss.str) {
return
}
- if !top {
+ if !top && !isPinned(uintptr(ss.str)) {
panic(errorString(msg))
}
case kindStruct:
@@ -547,7 +547,7 @@
if !cgoIsGoPointer(p) {
return
}
- if !top {
+ if !top && !isPinned(uintptr(p)) {
panic(errorString(msg))
}
@@ -573,7 +573,8 @@
// No more possible pointers.
break
}
- if hbits.isPointer() && cgoIsGoPointer(*(*unsafe.Pointer)(unsafe.Pointer(base + i))) {
+ pi := *(*unsafe.Pointer)(unsafe.Pointer(base + i))
+ if hbits.isPointer() && cgoIsGoPointer(pi) && !isPinned(uintptr(pi)) {
panic(errorString(msg))
}
hbits = hbits.next()
diff --git a/src/runtime/cgocheck.go b/src/runtime/cgocheck.go
index 3acbadf..1dd3a16 100644
--- a/src/runtime/cgocheck.go
+++ b/src/runtime/cgocheck.go
@@ -43,6 +43,11 @@
return
}
+ // If the object is pinned, it's OK.
+ if isPinned(src) {
+ return
+ }
+
// It's OK if writing to memory allocated by persistentalloc.
// Do this check last because it is more expensive and rarely true.
// If it is false the expense doesn't matter since we are crashing.
@@ -155,7 +160,7 @@
bits := hbits.bits()
if i >= off && bits&bitPointer != 0 {
v := *(*unsafe.Pointer)(add(src, i))
- if cgoIsGoPointer(v) {
+ if cgoIsGoPointer(v) && !isPinned(uintptr(v)) {
throw(cgoWriteBarrierFail)
}
}
@@ -188,7 +193,7 @@
} else {
if bits&1 != 0 {
v := *(*unsafe.Pointer)(add(src, i))
- if cgoIsGoPointer(v) {
+ if cgoIsGoPointer(v) && !isPinned(uintptr(v)) {
throw(cgoWriteBarrierFail)
}
}
diff --git a/src/runtime/lockrank.go b/src/runtime/lockrank.go
index 4a16bc0..bee6375 100644
--- a/src/runtime/lockrank.go
+++ b/src/runtime/lockrank.go
@@ -55,7 +55,7 @@
lockRankFin
lockRankNotifyList
lockRankTraceStrings
- lockRankMspanSpecial
+ lockRankMspan
lockRankProf
lockRankGcBitsArenas
lockRankRoot
@@ -77,6 +77,7 @@
lockRankWbufSpans
lockRankMheap
lockRankMheapSpecial
+ lockRankMheapPinnerBitsAlloc
// Memory-related leaf locks
lockRankGlobalAlloc
@@ -136,7 +137,7 @@
lockRankFin: "fin",
lockRankNotifyList: "notifyList",
lockRankTraceStrings: "traceStrings",
- lockRankMspanSpecial: "mspanSpecial",
+ lockRankMspan: "mspan",
lockRankProf: "prof",
lockRankGcBitsArenas: "gcBitsArenas",
lockRankRoot: "root",
@@ -214,7 +215,7 @@
lockRankFin: {lockRankSysmon, lockRankScavenge, lockRankSched, lockRankAllg, lockRankTimers, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf},
lockRankNotifyList: {},
lockRankTraceStrings: {lockRankTraceBuf},
- lockRankMspanSpecial: {lockRankSysmon, lockRankScavenge, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf, lockRankNotifyList, lockRankTraceStrings},
+ lockRankMspan: {lockRankSysmon, lockRankScavenge, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf, lockRankNotifyList, lockRankTraceStrings},
lockRankProf: {lockRankSysmon, lockRankScavenge, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf, lockRankNotifyList, lockRankTraceStrings},
lockRankGcBitsArenas: {lockRankSysmon, lockRankScavenge, lockRankAssistQueue, lockRankCpuprof, lockRankSched, lockRankAllg, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf, lockRankNotifyList, lockRankTraceStrings},
lockRankRoot: {},
@@ -231,8 +232,8 @@
lockRankStackLarge: {lockRankSysmon, lockRankAssistQueue, lockRankSched, lockRankItab, lockRankHchan, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankSpanSetSpine, lockRankGscan},
lockRankDefer: {},
lockRankSudog: {lockRankHchan, lockRankNotifyList},
- lockRankWbufSpans: {lockRankSysmon, lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankPollDesc, lockRankSched, lockRankAllg, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankFin, lockRankNotifyList, lockRankTraceStrings, lockRankMspanSpecial, lockRankProf, lockRankRoot, lockRankGscan, lockRankDefer, lockRankSudog},
- lockRankMheap: {lockRankSysmon, lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankPollDesc, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf, lockRankFin, lockRankNotifyList, lockRankTraceStrings, lockRankMspanSpecial, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankSpanSetSpine, lockRankGscan, lockRankStackpool, lockRankStackLarge, lockRankDefer, lockRankSudog, lockRankWbufSpans},
+ lockRankWbufSpans: {lockRankSysmon, lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankSweep, lockRankPollDesc, lockRankSched, lockRankAllg, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankFin, lockRankNotifyList, lockRankTraceStrings, lockRankMspan, lockRankProf, lockRankRoot, lockRankGscan, lockRankDefer, lockRankSudog},
+ lockRankMheap: {lockRankSysmon, lockRankScavenge, lockRankSweepWaiters, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankPollDesc, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf, lockRankFin, lockRankNotifyList, lockRankTraceStrings, lockRankMspan, lockRankProf, lockRankGcBitsArenas, lockRankRoot, lockRankSpanSetSpine, lockRankGscan, lockRankStackpool, lockRankStackLarge, lockRankDefer, lockRankSudog, lockRankWbufSpans},
lockRankMheapSpecial: {lockRankSysmon, lockRankScavenge, lockRankAssistQueue, lockRankCpuprof, lockRankSweep, lockRankPollDesc, lockRankSched, lockRankAllg, lockRankAllp, lockRankTimers, lockRankItab, lockRankReflectOffs, lockRankHchan, lockRankTraceBuf, lockRankNotifyList, lockRankTraceStrings},
lockRankGlobalAlloc: {lockRankProf, lockRankSpanSetSpine, lockRankMheap, lockRankMheapSpecial},
lockRankPageAllocScav: {lockRankMheap},
diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go
index 3330ddd..5012996 100644
--- a/src/runtime/mbitmap.go
+++ b/src/runtime/mbitmap.go
@@ -231,6 +231,7 @@
// n must be within [0, s.npages*_PageSize),
// or may be exactly s.npages*_PageSize
// if s.elemsize is from sizeclasses.go.
+//go:nosplit
func (s *mspan) divideByElemSize(n uintptr) uintptr {
const doubleCheck = false
@@ -244,6 +245,7 @@
return q
}
+//go:nosplit
func (s *mspan) objIndex(p uintptr) uintptr {
return s.divideByElemSize(p - s.base())
}
diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go
index a5129bd..5a3dd91 100644
--- a/src/runtime/mgcmark.go
+++ b/src/runtime/mgcmark.go
@@ -378,7 +378,7 @@
// Lock the specials to prevent a special from being
// removed from the list while we're traversing it.
- lock(&s.speciallock)
+ lock(&s.lock)
for sp := s.specials; sp != nil; sp = sp.next {
if sp.kind != _KindSpecialFinalizer {
continue
@@ -397,7 +397,7 @@
// The special itself is a root.
scanblock(uintptr(unsafe.Pointer(&spf.fn)), goarch.PtrSize, &oneptrmask[0], gcw, nil)
}
- unlock(&s.speciallock)
+ unlock(&s.lock)
}
}
}
diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go
index ecbd0a3..9b74331 100644
--- a/src/runtime/mheap.go
+++ b/src/runtime/mheap.go
@@ -213,6 +213,8 @@
specialReachableAlloc fixalloc // allocator for specialReachable
speciallock mutex // lock for special record allocators.
arenaHintAlloc fixalloc // allocator for arenaHints
+ pinnerBitsAlloc fixalloc // allocator for *pBits
+ pinnerBitsAllocLock mutex // lock for *pBits allocator
unused *specialfinalizer // never set, just here to force the specialfinalizer type into DWARF
}
@@ -449,16 +451,17 @@
// if sweepgen == h->sweepgen + 3, the span was swept and then cached and is still cached
// h->sweepgen is incremented by 2 after every GC
- sweepgen uint32
- divMul uint32 // for divide by elemsize
- allocCount uint16 // number of allocated objects
- spanclass spanClass // size class and noscan (uint8)
- state mSpanStateBox // mSpanInUse etc; accessed atomically (get/set methods)
- needzero uint8 // needs to be zeroed before allocation
- elemsize uintptr // computed from sizeclass or from npages
- limit uintptr // end of data in span
- speciallock mutex // guards specials list
- specials *special // linked list of special records sorted by offset.
+ sweepgen uint32
+ divMul uint32 // for divide by elemsize
+ allocCount uint16 // number of allocated objects
+ spanclass spanClass // size class and noscan (uint8)
+ state mSpanStateBox // mSpanInUse etc; accessed atomically (get/set methods)
+ needzero uint8 // needs to be zeroed before allocation
+ elemsize uintptr // computed from sizeclass or from npages
+ limit uintptr // end of data in span
+ lock mutex // guards specials list and pinnedBits
+ specials *special // linked list of special records sorted by offset.
+ pinnedBits *pBits // bitmap for pinned objects
}
func (s *mspan) base() uintptr {
@@ -474,6 +477,31 @@
return
}
+//go:notinheap
+type pBits byte
+
+func (h *mheap) newPinnedBits() *pBits {
+ lock(&h.pinnerBitsAllocLock)
+ pinnedBits := (*pBits)(h.pinnerBitsAlloc.alloc())
+ unlock(&h.pinnerBitsAllocLock)
+ return pinnedBits
+}
+
+func (h *mheap) freePinnedBits(p *pBits) {
+ lock(&h.pinnerBitsAllocLock)
+ h.pinnerBitsAlloc.free(unsafe.Pointer(p))
+ unlock(&h.pinnerBitsAllocLock)
+}
+
+//go:nosplit
+func (s *mspan) getPinnedBits() *pBits {
+ return (*pBits)(atomic.Loadp(unsafe.Pointer(&s.pinnedBits)))
+}
+
+func (s *mspan) setPinnedBits(p *pBits) {
+ atomicstorep(unsafe.Pointer(&s.pinnedBits), unsafe.Pointer(p))
+}
+
// recordspan adds a newly allocated span to h.allspans.
//
// This only happens the first time a span is allocated from
@@ -697,6 +725,7 @@
func (h *mheap) init() {
lockInit(&h.lock, lockRankMheap)
lockInit(&h.speciallock, lockRankMheapSpecial)
+ lockInit(&h.pinnerBitsAllocLock, lockRankMheapPinnerBitsAlloc)
h.spanalloc.init(unsafe.Sizeof(mspan{}), recordspan, unsafe.Pointer(h), &memstats.mspan_sys)
h.cachealloc.init(unsafe.Sizeof(mcache{}), nil, nil, &memstats.mcache_sys)
@@ -704,6 +733,7 @@
h.specialprofilealloc.init(unsafe.Sizeof(specialprofile{}), nil, nil, &memstats.other_sys)
h.specialReachableAlloc.init(unsafe.Sizeof(specialReachable{}), nil, nil, &memstats.other_sys)
h.arenaHintAlloc.init(unsafe.Sizeof(arenaHint{}), nil, nil, &memstats.other_sys)
+ h.pinnerBitsAlloc.init((maxObjsPerSpan+7)/8, nil, nil, &memstats.other_sys)
// Don't zero mspan allocations. Background sweeping can
// inspect a span concurrently with allocating it, so it's
@@ -1510,6 +1540,12 @@
// Mark the space as free.
h.pages.free(s.base(), s.npages, false)
+ // Free pinnedBits if set.
+ if pinnedBits := s.getPinnedBits(); pinnedBits != nil {
+ s.setPinnedBits(nil)
+ h.freePinnedBits(pinnedBits)
+ }
+
// Free the span structure. We no longer have a use for it.
s.state.set(mSpanDead)
h.freeMSpanLocked(s)
@@ -1561,14 +1597,15 @@
span.allocCount = 0
span.spanclass = 0
span.elemsize = 0
- span.speciallock.key = 0
+ span.lock.key = 0
span.specials = nil
span.needzero = 0
span.freeindex = 0
span.allocBits = nil
span.gcmarkBits = nil
+ span.pinnedBits = nil
span.state.set(mSpanDead)
- lockInit(&span.speciallock, lockRankMspanSpecial)
+ lockInit(&span.lock, lockRankMspan)
}
func (span *mspan) inList() bool {
@@ -1722,7 +1759,7 @@
offset := uintptr(p) - span.base()
kind := s.kind
- lock(&span.speciallock)
+ lock(&span.lock)
// Find splice point, check for existing record.
t := &span.specials
@@ -1732,7 +1769,7 @@
break
}
if offset == uintptr(x.offset) && kind == x.kind {
- unlock(&span.speciallock)
+ unlock(&span.lock)
releasem(mp)
return false // already exists
}
@@ -1747,7 +1784,7 @@
s.next = *t
*t = s
spanHasSpecials(span)
- unlock(&span.speciallock)
+ unlock(&span.lock)
releasem(mp)
return true
@@ -1771,7 +1808,7 @@
offset := uintptr(p) - span.base()
var result *special
- lock(&span.speciallock)
+ lock(&span.lock)
t := &span.specials
for {
s := *t
@@ -1790,7 +1827,7 @@
if span.specials == nil {
spanHasNoSpecials(span)
}
- unlock(&span.speciallock)
+ unlock(&span.lock)
releasem(mp)
return result
}
diff --git a/src/runtime/pinner.go b/src/runtime/pinner.go
new file mode 100644
index 0000000..e8e815a
--- /dev/null
+++ b/src/runtime/pinner.go
@@ -0,0 +1,144 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package runtime
+
+import (
+ "runtime/internal/atomic"
+ "unsafe"
+)
+
+// Pinner represents a set of pinned Go objects. An object can be pinned with
+// the Pin() method and all pinned objects of a Pinner can be unpinned with the
+// Unpin() method.
+type Pinner struct {
+ *pinnerInstance
+}
+
+// Pin a Go object. The object will not be touched by the garbage collector
+// until the Unpin() method has been called. The pointer to a pinned object can
+// be directly stored in C memory or can be contained in Go memory passed to C
+// functions. The argument must be a pointer of any type, an unsafe.Pointer, a
+// string or a function. The object must not be pinned already. The object must
+// be allocated in Go heap memory, which excludes objects smaller than 16 bytes
+// and global pointer-free objects. If one of these conditions are not met,
+// Pin() will panic.
+func (p *Pinner) Pin(pointer interface{}) {
+ if p.pinnerInstance == nil {
+ p.pinnerInstance = &pinnerInstance{}
+ SetFinalizer(p.pinnerInstance, func(i *pinnerInstance) {
+ if i.refs != nil {
+ pinnerUnpin(i)
+ pinnerLeakPanic()
+ }
+ })
+ }
+ ptr := pinnerGetPtr(&pointer)
+
+ if !cgoIsGoPointer(ptr) {
+ panic(errorString("runtime.Pinner.Pin: argument is not a Go pointer"))
+ }
+ if setPinned(uintptr(ptr), true) {
+ p.refs = append(p.refs, ptr)
+ } else {
+ panic(errorString("runtime.Pinner.Pin: already pinned"))
+ }
+}
+
+// Unpin all pinned objects of the Pinner. If the Pinner holds at least one
+// pinned object, Unpin() must be called on the same Pinner, or the garbage
+// collector will panic.
+func (p *Pinner) Unpin() {
+ pinnerUnpin(p.pinnerInstance)
+}
+
+type pinnerInstance struct {
+ refs []unsafe.Pointer
+}
+
+func pinnerUnpin(p *pinnerInstance) {
+ if p == nil || p.refs == nil {
+ return
+ }
+ for i := range p.refs {
+ setPinned(uintptr(p.refs[i]), false)
+ p.refs[i] = nil
+ }
+ p.refs = nil
+}
+
+func pinnerGetPtr(i *interface{}) unsafe.Pointer {
+ e := efaceOf(i)
+ etyp := e._type
+ if etyp == nil {
+ panic(errorString("runtime.Pinner: argument is nil"))
+ }
+ var p unsafe.Pointer
+ switch etyp.kind & kindMask {
+ case kindPtr, kindUnsafePointer:
+ p = e.data
+ case kindString:
+ ss := (*stringStruct)(e.data)
+ p = ss.str
+ case kindFunc:
+ p = e.data
+ default:
+ panic(errorString("runtime.Pinner: type of argument is unsupported: " + etyp.string()))
+ }
+ return p
+}
+
+//go:nosplit
+func isPinned(p uintptr) bool {
+ span := spanOfHeap(p)
+ if span == nil {
+ return false
+ }
+ pinnedBits := span.getPinnedBits()
+ if pinnedBits == nil {
+ return false
+ }
+ objIndex := span.objIndex(p)
+ bytep := addb((*byte)(pinnedBits), objIndex/8)
+ mask := byte(1 << (objIndex % 8))
+ return (atomic.Load8(bytep) & mask) != 0
+}
+
+func setPinned(p uintptr, val bool) bool {
+ span := spanOfHeap(p)
+ if span == nil {
+ panic(errorString("runtime.Pinner: invalid pointer"))
+ }
+ if span.spanclass == tinySpanClass {
+ panic(errorString("runtime.Pinner: invalid pointer to tiny object"))
+ }
+ objIndex := span.objIndex(p)
+ if p != span.base()+objIndex*span.elemsize {
+ panic(errorString("runtime.Pinner: pointer not at beginning of allocated block"))
+ }
+ lock(&span.lock) // guard against concurrent calls of setPinned()
+ pinnedBits := span.getPinnedBits()
+ if pinnedBits == nil {
+ pinnedBits = mheap_.newPinnedBits()
+ span.setPinnedBits(pinnedBits)
+ }
+ bytep := addb((*byte)(pinnedBits), objIndex/8)
+ mask := byte(1 << (objIndex % 8))
+ changed := val != ((atomic.Load8(bytep) & mask) != 0)
+ if changed {
+ if val {
+ atomic.Or8(bytep, mask)
+ } else {
+ atomic.And8(bytep, ^mask)
+ }
+ }
+ unlock(&span.lock)
+ return changed
+}
+
+// To be able to test that the GC panics when a pinned pointer is leaking, this
+// panic function is a variable, that can be overwritten by a test.
+var pinnerLeakPanic = func() {
+ panic(errorString("Pinner: Found leaking pinned pointer. Forgot to call Unpin()?"))
+}
diff --git a/src/runtime/pinner_test.go b/src/runtime/pinner_test.go
new file mode 100644
index 0000000..ad142c3
--- /dev/null
+++ b/src/runtime/pinner_test.go
@@ -0,0 +1,330 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package runtime_test
+
+import (
+ "runtime"
+ "testing"
+ "time"
+ "unsafe"
+)
+
+type _dbgVar struct {
+ name string
+ value *int32
+}
+
+//go:linkname _dbgvars runtime.dbgvars
+var _dbgvars []_dbgVar
+
+var cgocheck = func() func() int32 {
+ for i := range _dbgvars {
+ if _dbgvars[i].name == "cgocheck" {
+ p := _dbgvars[i].value
+ return func() int32 {
+ return *p
+ }
+ }
+ }
+ panic("Couln't find cgocheck debug variable")
+}()
+
+//go:linkname _cgoCheckPointer runtime.cgoCheckPointer
+func _cgoCheckPointer(ptr interface{}, arg interface{})
+
+//go:linkname _isPinned runtime.isPinned
+func _isPinned(ptr unsafe.Pointer) bool
+
+//go:linkname _pinnerLeakPanic runtime.pinnerLeakPanic
+var _pinnerLeakPanic func()
+
+var globalUintptr uintptr
+
+func assertPanics(t *testing.T) {
+ if recover() == nil {
+ t.Fatal("did not panic")
+ }
+}
+
+func assertCgoCheckPanics(t *testing.T, p interface{}) {
+ defer func() {
+ if recover() == nil {
+ t.Fatal("cgoCheckPointer did not panic, run with cgocheck=1")
+ }
+ }()
+ _cgoCheckPointer(p, true)
+}
+
+func TestPinnerPinKeepsAlive(t *testing.T) {
+ var pinner runtime.Pinner
+ p := new(*byte)
+ done := make(chan struct{})
+ runtime.SetFinalizer(p, func(interface{}) {
+ done <- struct{}{}
+ })
+ pinner.Pin(p)
+ p = nil
+ runtime.GC()
+ runtime.GC()
+ select {
+ case <-done:
+ t.Fatal("Pin didn't keep object alive")
+ case <-time.After(time.Millisecond * 10):
+ break
+ }
+ pinner.Unpin()
+ runtime.GC()
+ runtime.GC()
+ select {
+ case <-done:
+ break
+ case <-time.After(time.Second):
+ t.Fatal("Unpin didn't release object")
+ }
+}
+
+func TestPinnerDoublePinPanics(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ p := new(*byte)
+ pinner.Pin(p)
+ defer assertPanics(t)
+ pinner.Pin(p)
+}
+
+func TestPinnerPinZerosizeObjPanics(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ p := new([0]byte)
+ defer assertPanics(t)
+ pinner.Pin(p)
+}
+
+func TestPinnerPinGlobalObjPanics(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ defer assertPanics(t)
+ pinner.Pin(&globalUintptr)
+}
+
+func TestPinnerPinTinyObjPanics(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ p := new(byte)
+ defer assertPanics(t)
+ pinner.Pin(p)
+}
+
+func TestPinnerEmptyUnpin(t *testing.T) {
+ var pinner runtime.Pinner
+ pinner.Unpin()
+ pinner.Unpin()
+}
+
+func TestPinnerLeakPanics(t *testing.T) {
+ func() {
+ defer assertPanics(t)
+ _pinnerLeakPanic()
+ }()
+ done := make(chan struct{})
+ old := _pinnerLeakPanic
+ _pinnerLeakPanic = func() {
+ done <- struct{}{}
+ }
+ func() {
+ var pinner runtime.Pinner
+ p := new(*byte)
+ pinner.Pin(p)
+ }()
+ runtime.GC()
+ runtime.GC()
+ select {
+ case <-done:
+ break
+ case <-time.After(time.Second):
+ t.Fatal("leak didn't make GC to panic")
+ }
+ _pinnerLeakPanic = old
+}
+
+func TestPinnerCgoCheckPtr2Ptr(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ p := new(*byte)
+ p2 := &p
+ assertCgoCheckPanics(t, p2)
+ pinner.Pin(p)
+ _cgoCheckPointer(p2, true)
+}
+
+func TestPinnerReuse(t *testing.T) {
+ var pinner runtime.Pinner
+ p := new(*byte)
+ p2 := &p
+ assertCgoCheckPanics(t, p2)
+ pinner.Pin(p)
+ _cgoCheckPointer(p2, true)
+ pinner.Unpin()
+ assertCgoCheckPanics(t, p2)
+ pinner.Pin(p)
+ _cgoCheckPointer(p2, true)
+ pinner.Unpin()
+}
+
+func TestPinnerCgoCheckPtr2UnsafePtr(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ p := unsafe.Pointer(new(*byte))
+ p2 := &p
+ assertCgoCheckPanics(t, p2)
+ pinner.Pin(p)
+ _cgoCheckPointer(p2, true)
+}
+
+func TestPinnerCgoCheckPtr2UnknownPtr(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ p := unsafe.Pointer(new(*byte))
+ p2 := &p
+ assertCgoCheckPanics(t, p2)
+ pinner.Pin(p)
+ pinner.Pin(p2)
+ _cgoCheckPointer(p2, true)
+}
+
+func TestPinnerCgoCheckString(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ s := string((&[32]byte{})[:])
+ p := &s
+ assertCgoCheckPanics(t, p)
+ pinner.Pin(s)
+ _cgoCheckPointer(p, true)
+}
+
+func TestPinnerCgoCheckSlice(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ sl := make([]byte, 32)
+ p := &sl
+ assertCgoCheckPanics(t, p)
+ pinner.Pin(&sl[0])
+ _cgoCheckPointer(p, true)
+}
+
+func TestPinnerCgoCheckIfc(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ var ifc interface{}
+ var o *byte
+ ifc = &o
+ p := &ifc
+ assertCgoCheckPanics(t, p)
+ pinner.Pin(&o)
+ _cgoCheckPointer(p, true)
+}
+
+func TestPinnerCgoCheckFunc(t *testing.T) {
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ var b *byte
+ f := func() byte {
+ return *b
+ }
+ p := &f
+ assertCgoCheckPanics(t, p)
+ pinner.Pin(f)
+ _cgoCheckPointer(p, true)
+}
+
+func TestPinnerCgoCheckWriteToNonGoMem(t *testing.T) {
+ if x := cgocheck(); x != 2 {
+ t.Skipf("skipped: running with cgocheck=%v, not cgocheck=2", x)
+ }
+ var pinner runtime.Pinner
+ defer pinner.Unpin()
+ nonGoPtr := (*unsafe.Pointer)(unsafe.Pointer(&globalUintptr))
+ o := new(*byte)
+ pinner.Pin(o)
+ *nonGoPtr = unsafe.Pointer(o)
+}
+
+func BenchmarkPinnerPinUnpinBatch(b *testing.B) {
+ const Batch = 1000
+ var data [Batch]**byte
+ for i := 0; i < Batch; i++ {
+ data[i] = new(*byte)
+ }
+ var pinner runtime.Pinner
+ b.ResetTimer()
+ for n := 0; n < b.N; n++ {
+ for i := 0; i < Batch; i++ {
+ pinner.Pin(data[i])
+ }
+ pinner.Unpin()
+ }
+}
+
+func BenchmarkPinnerPinUnpin(b *testing.B) {
+ var pinner runtime.Pinner
+ b.ResetTimer()
+ for n := 0; n < b.N; n++ {
+ pinner.Pin(new(*byte))
+ pinner.Unpin()
+ }
+}
+
+func BenchmarkPinnerPinUnpinParallel(b *testing.B) {
+ b.ResetTimer()
+ b.RunParallel(func(pb *testing.PB) {
+ var pinner runtime.Pinner
+ for pb.Next() {
+ pinner.Pin(new(*byte))
+ pinner.Unpin()
+ }
+ })
+}
+
+func BenchmarkPinnerIsPinnedOnPinned(b *testing.B) {
+ var pinner runtime.Pinner
+ ptr := new(*byte)
+ pinner.Pin(ptr)
+ b.ResetTimer()
+ for n := 0; n < b.N; n++ {
+ _isPinned(unsafe.Pointer(ptr))
+ }
+ pinner.Unpin()
+}
+
+func BenchmarkPinnerIsPinnedOnUnpinned(b *testing.B) {
+ ptr := new(*byte)
+ b.ResetTimer()
+ for n := 0; n < b.N; n++ {
+ _isPinned(unsafe.Pointer(ptr))
+ }
+}
+
+func BenchmarkPinnerIsPinnedOnPinnedParallel(b *testing.B) {
+ var pinner runtime.Pinner
+ ptr := new(*byte)
+ pinner.Pin(ptr)
+ b.ResetTimer()
+ b.RunParallel(func(pb *testing.PB) {
+ for pb.Next() {
+ _isPinned(unsafe.Pointer(ptr))
+ }
+ })
+ pinner.Unpin()
+}
+
+func BenchmarkPinnerIsPinnedOnUnpinnedParallel(b *testing.B) {
+ ptr := new(*byte)
+ b.ResetTimer()
+ b.RunParallel(func(pb *testing.PB) {
+ for pb.Next() {
+ _isPinned(unsafe.Pointer(ptr))
+ }
+ })
+}
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Michael Knyszek, Austin Clements, Michael Pratt, Keith Randall.
Sven Anderson uploaded patch set #2 to this change.
runtime: implement Pinner API for object pinning
Some C APIs require the use or structures that contain pointers to
buffers (iovec, io_uring, ...). The pointer passing rules would
require that these buffers are allocated in C memory and to process
this data with Go libraries it would need to be copied.
In order to provide a zero-copy way to use these C APIs, this CL
implements a Pinner API that allows to pin Go objects, which
guarantees that the garbage collector does not move these objects
while pinned. This allows to relax the pointer passing rules so that
pinned pointers can be stored in C allocated memory or can be
contained in Go memory that is passed to C functions.
The Pin() method accepts pointers to objects of any type and
unsafe.Pointer. Slices and arrays can be pinned by calling Pin()
with the pointer to the first element. Pinning of maps is not
supported.
Behaviour decisions: If the GC collects unreachable Pinner holding
pinned objects it panics. Trying to call Pin() with the following
arguments panics as well:
- other type than pointer
- pointer to pinned object
- pointer to zero-size or tiny object
- pointer to global object that is not allocated in the heap
- pointer not pointing to the beginning of an allocated object
Performance considerations: This change has no impact on execution
time on existing code, because checks are only done in code paths,
that would panic otherwise. The memory footprint is one pointer per
memory span.
Fixes: #46787
Signed-off-by: Sven Anderson <sv...@anderson.de>
Change-Id: I110031fe789b92277ae45a9455624687bd1c54f2
---
M src/runtime/cgocheck.go
A src/runtime/pinner_test.go
M src/runtime/cgocall.go
M src/runtime/mheap.go
M src/runtime/lockrank.go
M src/runtime/mbitmap.go
A src/runtime/pinner.go
M src/runtime/mgcmark.go
8 files changed, 562 insertions(+), 32 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Attention is currently required from: Michael Knyszek, Michael Pratt, Keith Randall, Sven Anderson.
10 comments:
Patchset:
I haven't read the code closely, but here are a few comments at a quick skim.
File src/runtime/lockrank.go:
Patch Set #2, Line 80:
lockRankMheapPinnerBitsAllocThis looks like a leaf lock to me, in which case it doesn't have to be in the order. (If it's not a leaf lock, that's very surprising and should probably be revisited.)
File src/runtime/mbitmap.go:
Patch Set #2, Line 234:
//go:nosplitPlease update the comment to explain why this is nosplit (and elsewhere). In general, we try to add explanations of nosplit as we add them.
File src/runtime/mheap.go:
Patch Set #2, Line 480:
//go:notinheapThis hunk should probably live in pinner.go.
Patch Set #2, Line 481:
type pBits byteThis would be much cleaner and less unsafe as [(maxObjsPerSpan+7)/8]byte.
Patch Set #2, Line 498:
return (*pBits)(atomic.Loadp(unsafe.Pointer(&s.pinnedBits)))Is the pinnedBits field itself protected by mspan.lock, or just its contents? If so, then these atomic accesses seem unnecessary.
File src/runtime/pinner.go:
Patch Set #2, Line 23:
The object must not be pinned already.This seems like an unfortunate constraint, since it would mean you can't pin the same object from two goroutines concurrently. You might not even realize this can happen until you get an unfortunate schedule and it panics.
Patch Set #2, Line 60:
func pinnerUnpin(p *pinnerInstance) {This can be a method of pinnerInstance to keep the namespace clutter down.
Patch Set #2, Line 103:
panic(errorString("runtime.Pinner: invalid pointer"))This could definitely use a comment. There are two reasons that should combine to make this impossible:
1. Pin only passes in a Go pointer, so this can't be, say, a pointer to C memory.
2. Pin also forces the pointer to escape to the heap, so this can't be a Go stack pointer.
This isn't obvious and these conditions are enforced outside this function, so it's worth explaining here.
Patch Set #2, Line 106:
panic(errorString("runtime.Pinner: invalid pointer to tiny object"))This seems like it would lead to rather unpredictable panics and is an odd restriction to communicate to users. Note that this span class isn't used by just the tiny allocator. It will include *any* pointer-free allocations up to 16 bytes, even if they're not packed together by the tiny allocator.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Austin Clements, Michael Knyszek, Michael Pratt, Keith Randall.
10 comments:
Patchset:
I haven't read the code closely, but here are a few comments at a quick skim.
Thanks for the review!
File src/runtime/lockrank.go:
Patch Set #2, Line 80:
lockRankMheapPinnerBitsAlloc
This looks like a leaf lock to me, in which case it doesn't have to be in the order. […]
I see. Actually I have no idea, what is the criterion for a ranked lock. I simply put it next to the special lock. Why is the special lock a ranked lock then?
File src/runtime/mbitmap.go:
Patch Set #2, Line 234:
//go:nosplit
Please update the comment to explain why this is nosplit (and elsewhere). […]
"Is called by nosplit code" is enough reason for nosplit, correct?
File src/runtime/mheap.go:
Patch Set #2, Line 480:
//go:notinheapThis hunk should probably live in pinner.go.
Ack
Patch Set #2, Line 481:
type pBits byteThis would be much cleaner and less unsafe as [(maxObjsPerSpan+7)/8]byte.
Ack
Patch Set #2, Line 498:
return (*pBits)(atomic.Loadp(unsafe.Pointer(&s.pinnedBits)))
Is the pinnedBits field itself protected by mspan. […]
mspan.lock only serializes _writes_ to pinnedBits (the pointer, not the bitmask it's pointing to). Reads of pinnedBits and access to the bitmask it's pointing to, are done lock-free with atomic operations.
File src/runtime/pinner.go:
Patch Set #2, Line 23:
The object must not be pinned already.
This seems like an unfortunate constraint, since it would mean you can't pin the same object from tw […]
How do you suggest to implement this with a bitmap otherwise? How could we make sure, that only the last Unpin() of concurring Pinners actually does remove the pin bit? That constrain is for me a consequence of using a bitmap. My only idea would be to additionally use a map with a counter. Do you have another idea?
Patch Set #2, Line 60:
func pinnerUnpin(p *pinnerInstance) {This can be a method of pinnerInstance to keep the namespace clutter down.
Ack
Patch Set #2, Line 103:
panic(errorString("runtime.Pinner: invalid pointer"))
This could definitely use a comment. […]
Ack
Patch Set #2, Line 106:
panic(errorString("runtime.Pinner: invalid pointer to tiny object"))
This seems like it would lead to rather unpredictable panics and is an odd restriction to communicat […]
I actually wasn't sure, how to handle these cases. Can we assume tiny objects to be implicitly pinned? What is the right approach here? I would need some guidance, I guess.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Austin Clements, Michael Pratt, Keith Randall, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Patch Set #2, Line 106:
panic(errorString("runtime.Pinner: invalid pointer to tiny object"))I actually wasn't sure, how to handle these cases. […]
I think the answer is to just pin the whole tiny block. any tiny object can keep a whole tiny block live, and I think this is the right analogue.
for that to work, I don't believe you need any special cases. tiny blocks are just objects in the 16-byte span class.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Attention is currently required from: Michael Knyszek, Michael Pratt, Keith Randall, Sven Anderson.
5 comments:
File src/runtime/lockrank.go:
Patch Set #2, Line 80:
lockRankMheapPinnerBitsAlloc
I see. Actually I have no idea, what is the criterion for a ranked lock. […]
Oh, I see, you didn't actually put it in the order below. You should add the constant since it labels the lock class, but just put it in the "Memory-related leaf locks" section instead of this one.
File src/runtime/mbitmap.go:
Patch Set #2, Line 234:
//go:nosplit
"Is called by nosplit code" is enough reason for nosplit, correct?
Perhaps more specifically "is called by deeply nosplit code", but yes.
File src/runtime/mheap.go:
Patch Set #2, Line 498:
return (*pBits)(atomic.Loadp(unsafe.Pointer(&s.pinnedBits)))mspan.lock only serializes _writes_ to pinnedBits (the pointer, not the bitmask it's pointing to). […]
Ah, okay. Please add a comment on the mspan.pinnedBits field explaining that.
File src/runtime/pinner.go:
Patch Set #2, Line 23:
The object must not be pinned already.How do you suggest to implement this with a bitmap otherwise? How could we make sure, that only the […]
I can think of a few possibilities. Maybe Michael will have more ideas.
It seems the constraints are that 1. you need to be able to probe "pinned > 0" for an object very efficiently in the cgo paths and 2. pin/unpin need to be pretty efficient. In addition, it's probably worth optimizing for the pinned == 1 case since that's likely to be extremely common.
Given this, my suggestion is the make the pinned bitmap be 2 bits per object and to add a special for objects with pinned > 1. Bit 0 means "pinned > 0" and bit 1 means "object has a pinning special" (roughly "pinned may be > 1").
isPinned would check bit 0 for the object being queried.
Pin would check the bitmap. If bit 0 is unset, it would just set bit 0 and return. If bit 0 is set and bit 1 is unset, it would set bit 1 and create a new special for the object tracking that its pin count is 2, and return. If both bits 0 and 1 are set, it would find the pinned special and increment the pinned count in it.
Unpin would be the reverse. If bit 0 is set and bit 1 is unset, it would just clear bit 0 and return. If both bits are set, it would look for the special and decrement the pin count. If the count reaches 0, it would clear both bits and delete the special.
Patch Set #2, Line 106:
panic(errorString("runtime.Pinner: invalid pointer to tiny object"))
I think the answer is to just pin the whole tiny block. […]
I think to do what Michael suggested, you would also need to drop the restriction that p point to the beginning of the object, since it could point into the middle of a tiny allocator block; and allow for pin counts > 1 since you could pin multiple tiny objects from the same tiny block.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Austin Clements, Michael Knyszek, Michael Pratt, Keith Randall.
5 comments:
File src/runtime/lockrank.go:
Patch Set #2, Line 80:
lockRankMheapPinnerBitsAlloc
Oh, I see, you didn't actually put it in the order below. […]
Ack
File src/runtime/mbitmap.go:
Patch Set #2, Line 234:
//go:nosplit
Perhaps more specifically "is called by deeply nosplit code", but yes.
Ack
File src/runtime/mheap.go:
Patch Set #2, Line 498:
return (*pBits)(atomic.Loadp(unsafe.Pointer(&s.pinnedBits)))
Ah, okay. Please add a comment on the mspan.pinnedBits field explaining that.
Ack
File src/runtime/pinner.go:
Patch Set #2, Line 23:
The object must not be pinned already.
I can think of a few possibilities. Maybe Michael will have more ideas. […]
Interesting idea. But seems like it's introducing a lot complexity for "just" disabling optional warnings about a problem, that the current GC doesn't even have. :-)
So, do you think using a hash-map lookup in the Pin()/Unpin() code-paths (instead of using the specials list) is too expensive? Or can we not use maps in that code area? (I know it's not working to use maps in the write-barrier cgo-checks, I tried that before. I'm still not 100% sure, where exactly I can use GC-managed data-structures, and where not.)
Patch Set #2, Line 106:
panic(errorString("runtime.Pinner: invalid pointer to tiny object"))
I think to do what Michael suggested, you would also need to drop the restriction that p point to th […]
Yeah, seems to me like when the multi-pinning is solved, this is also no problem anymore.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Attention is currently required from: Keith Randall, Michael Knyszek, Michael Pratt, Sven Anderson.
2 comments:
Patchset:
Hi Sven. I haven't seen and updated version of this CL and I was just curious what your plan and availability to work on it are like (no pressure one way or another, just for my own awareness).
File src/runtime/pinner.go:
Patch Set #2, Line 23:
The object must not be pinned already.
Interesting idea. […]
It's not an optional warning, though, it's a panic. And the fact that the GC doesn't currently move objects seems orthogonal: the constraint on the object not already being pinned is a property of this API regardless of the GC implementation.
Unfortunately, I don't think it would work to use a hash map. As you point out, you wouldn't be able to use it from the cgo checking code (it's true that where you can and can't use various runtime features is complicated, but generally speaking you can't use much of anything in the write barrier path :). I think the locking overhead would also be a serious problem.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Austin Clements, Keith Randall, Michael Knyszek, Michael Pratt.
1 comment:
Patchset:
Hi Sven. […]
Thanks for asking. My plan is definitely to land this CL! I only was waiting for feedback to my last questions, and I didn't want to be impatient, because I know you guys have a lot of other things to do.
I mainly want to avoid to run into the wrong direction, that's why would like to clarify some things first and decide with you on an approach before I put more work into it. I see a couple of options, and I have several "building blocks" lying around already from former experimentation. Do you want me to lay them out here, or should we do that in the Github issue for the proposal?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Keith Randall, Michael Knyszek, Michael Pratt, Sven Anderson.
5 comments:
Patchset:
Thanks for asking. […]
What questions need to be answered? I couldn't see them in a quick look. Thanks.
File src/runtime/lockrank.go:
Patch Set #2, Line 80:
lockRankMheapPinnerBitsAlloc
Ack
Ack
File src/runtime/mbitmap.go:
Patch Set #2, Line 234:
//go:nosplit
Ack
Ack
File src/runtime/mheap.go:
Patch Set #2, Line 498:
return (*pBits)(atomic.Loadp(unsafe.Pointer(&s.pinnedBits)))
Ack
Ack
File src/runtime/pinner.go:
Patch Set #2, Line 106:
panic(errorString("runtime.Pinner: invalid pointer to tiny object"))Yeah, seems to me like when the multi-pinning is solved, this is also no problem anymore.
Ack
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Keith Randall, Michael Knyszek, Michael Pratt.
1 comment:
Patchset:
What questions need to be answered? I couldn't see them in a quick look. Thanks.
It was the question above about using maps, which Austin partly answered in the meanwhile. But now the main question is: where should I lay out my various ideas how to implement this further? I can do it here in the comments, but the UI seems a bit confusing for that purpose, so I could do it in the Github issue instead? But I'm fine with both.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Keith Randall, Michael Knyszek, Michael Pratt, Sven Anderson.
1 comment:
Patchset:
It was the question above about using maps, which Austin partly answered in the meanwhile. […]
The GitHub issue is usually a better place. Thanks.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Keith Randall, Michael Knyszek, Sven Anderson.
1 comment:
Patchset:
The GitHub issue is usually a better place. Thanks.
Ok, please see https://github.com/golang/go/issues/46787#issuecomment-1173830490
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Keith Randall, Michael Knyszek, Sven Anderson.
1 comment:
Patchset:
R=go1. […]
R=go1.20
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Austin Clements, Keith Randall, Michael Knyszek, Sven Anderson.
1 comment:
Patchset:
This seems to have picked up a merge conflict.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Keith Randall, Michael Knyszek.
1 comment:
Patchset:
This seems to have picked up a merge conflict.
yes, it’s on hold, please see: https://github.com/golang/go/issues/46787#issuecomment-1173830490
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Michael Knyszek.
Ansiwen uploaded patch set #4 to this change.
runtime: implement Pinner API for object pinning
Some C APIs require the use or structures that contain pointers to
buffers (iovec, io_uring, ...). The pointer passing rules would
require that these buffers are allocated in C memory and to process
this data with Go libraries it would need to be copied.
In order to provide a zero-copy way to use these C APIs, this CL
implements a Pinner API that allows to pin Go objects, which
guarantees that the garbage collector does not move these objects
while pinned. This allows to relax the pointer passing rules so that
pinned pointers can be stored in C allocated memory or can be
contained in Go memory that is passed to C functions.
The Pin() method accepts pointers to objects of any type and
unsafe.Pointer. Slices and arrays can be pinned by calling Pin()
with the pointer to the first element. Pinning of maps is not
supported.
Behaviour decisions: If the GC collects unreachable Pinner holding
pinned objects it panics. Trying to call Pin() with the following
arguments panics as well:
- other type than pointer
- pointer to zero-size object
- pointer to global object that is not allocated in the heap
Performance considerations: This change has no impact on execution
time on existing code, because checks are only done in code paths,
that would panic otherwise. The memory footprint on existing code is
one pointer per memory span.
Fixes: #46787
Signed-off-by: Sven Anderson <sv...@anderson.de>
Change-Id: I110031fe789b92277ae45a9455624687bd1c54f2
---
M src/runtime/cgocall.go
M src/runtime/cgocheck.go
M src/runtime/mbitmap.go
M src/runtime/mheap.go
A src/runtime/pinner.go
A src/runtime/pinner_test.go
6 files changed, 736 insertions(+), 53 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek, Sven Anderson.
8 comments:
Patchset:
Thanks for the review!
Done
Patchset:
Ok, please see https://github. […]
Done
yes, it’s on hold, please see: https://github. […]
Done
Patchset:
I finally finished a new version. I believe I addressed all comments. This version allows multi-pinning, tiny-objects, and pointers that don't point to the beginning of the object. The pin counters are based on a new specials records. I also had a finished version based on a global map, but I had to scratch my itch and see if I can implement a specials-based version. ;-)
@aus...@google.com, @mkny...@google.com PTAL, thanks!
File src/runtime/mheap.go:
Patch Set #2, Line 480:
//go:notinheapAck
Done
Patch Set #2, Line 481:
type pBits byte
Ack
Done
File src/runtime/pinner.go:
Patch Set #2, Line 23:
The object must not be pinned already.It's not an optional warning, though, it's a panic. […]
Acknowledged
Patch Set #2, Line 103:
panic(errorString("runtime.Pinner: invalid pointer"))
Ack
Done
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek, Sven Anderson.
5 comments:
Patchset:
I made som inline comments with open questions I have.
File src/runtime/pinner.go:
Patch Set #4, Line 119:
alreadySet := pin == ((atomic.Load8(bytep) & mask) != 0)Unfortunately there is no atomic CompareAndSwap for 8 bit, so I couldn't make this lock-free. I could try to rework this with Uintptr accesses, but on the other hand, Pin() is not so performance critical. WDYT?
Patch Set #4, Line 124:
systemstack(func() {I'm actually not sure if the systemstack is necessary here. I simply copied it from SetFinalizer().
Patch Set #4, Line 137:
systemstack(func() {dito
Patch Set #4, Line 182:
func (span *mspan) incPinCounter(offset uintptr) {The two methods incPinCounter() and decPinCounter() share a lot of code with addspecial() and removespecial(), which I don't like, but they are different enough that I couldn't come up with efficient functions that work for both cases. (Besides the specialFindSlicePoint() helper.)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek, Sven Anderson.
1 comment:
Patchset:
R=go1.21
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek, Sven Anderson.
1 comment:
File src/runtime/mheap.go:
Patch Set #4, Line 56:
maxObjsPerSpan = pageSize / smallSizeDivI'm not 100% sure `smallSizeDiv` is semantically coreect constant to use here. Please confirm.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Patch Set #4, Line 112:
lock(&span.speciallock) // guard against concurrent calls of setPinned() on same spanIt might be necessary to aquirem() already here? I wonder what happens if the span is swept while holding speciallock.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Patch Set #4, Line 119:
alreadySet := pin == ((atomic.Load8(bytep) & mask) != 0)Unfortunately there is no atomic CompareAndSwap for 8 bit, so I couldn't make this lock-free. […]
Actually it's not trivial at all to implement this in a lock free manner in relation to the specials list, I just tried. So disregard this comment.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek, Sven Anderson.
Ansiwen uploaded patch set #5 to this change.
6 files changed, 818 insertions(+), 53 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
I made it a bit more efficient in the single pin cases with an addition of a specials-counter. Also added more benchmarks.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
28 comments:
File src/runtime/cgocall.go:
Patch Set #5, Line 452:
// level, where Go pointers are allowed.I'd appreciate an update to the comment at the top here and in this function to indicate the new pinning behavior.
Patch Set #5, Line 568:
into Go memorynit: maybe "into unpinned Go memory"?
File src/runtime/cgocheck.go:
nit: maybe "an unpinned Go pointer into non-Go memory"?
Patch Set #5, Line 51:
it's OK.nit: maybe "it's safe store in C memory. The GC ensures it will not be moved or freed."
nit: maybe "an unpinned Go pointer"?
File src/runtime/mbitmap.go:
nit: unnecessary parens
File src/runtime/mheap.go:
Patch Set #4, Line 56:
maxObjsPerSpan = pageSize / smallSizeDivI'm not 100% sure `smallSizeDiv` is semantically coreect constant to use here. Please confirm.
I think the right thing to do here is to emit this from mksizeclasses.go into sizeclasses.go directly.
specifically, just take the max over all size classes.
File src/runtime/mheap.go:
if pinnerBits := s.getPinnerBits(); pinnerBits != nil {
s.setPinnerBits(nil)I think this races with something else checking the pinner bits. in theory a thread checking isPinned could load the pinner bits, get descheduled. then, the pinner bits get freed and subsequently reallocated for something else.
when the thread comes back, it's definitely looking at invalid bits.
I'm not certain there's a simple fix here that retains the same performance, since in general being able to free the pinner bits is conditioned on there being no outstanding readers.
the simple fix of course is to always access the pinner bits under the span's special lock, though I see that you're trying to avoid that.
there might be a different design that avoids this, but I have to think about it.
File src/runtime/pinner.go:
Patch Set #4, Line 124:
systemstack(func() {
I'm actually not sure if the systemstack is necessary here. I simply copied it from SetFinalizer().
the key question is whether a stack growth could cause a lock to be acquired a second time (resulting in a deadlock).
I don't think so? and I don't think addfinalizer even technically needs to be on the systemstack. but it seems all special-related code runs on the systemstack (including mem profile specials).
I think it might be a good idea to leave this on the system stack but it just leave a TODO to investigate this (you can give it to me, mknyszek).
Patch Set #4, Line 182:
func (span *mspan) incPinCounter(offset uintptr) {
The two methods incPinCounter() and decPinCounter() share a lot of code with addspecial() and remove […]
I think what you have here is OK.
File src/runtime/pinner.go:
high-level comment about documentation: please drop the parens in documentation. other Go std documentation doesn't do this. I tried to flag all the instances I could find.
nit: unnecessary parens
nit: unnecessary parens
Patch Set #5, Line 20:
touchedI don't think this is actually true. the object will still be scanned by the GC AFAICT, but we guarantee that it won't be moved or freed.
nit: unnecessary parens
Go memory, which excludes global pointer-free
// objectsreading your CL, I think this is even more specific. we only have spans for heap memory, so I think this should probably say what SetFinalizer says: "The argument obj must be a pointer to an object allocated by calling new, by taking the address of a composite literal, or by taking the address of a local variable."
nit: unnecessary parens
Patch Set #5, Line 28:
&pinnerInstance{}nit: in the runtime we generally try to make allocations explicit via "new", so I'd prefer this to be "new(pinnerInstance)".
Patch Set #5, Line 31:
i.pinnerUnpin()why unpin before the panic? I'm not sure it's possible to actually catch this panic, since it's going to happen on the finalizer goroutine.
nit: unnecessary parens
Unpin() must be called on the same Pinner, or the garbage
// collector will panic.
I'm not quite sure what this sentence is saying. you can't really call this on another Pinner. do you mean maybe something like: "If the Pinner holds at least one pinned object, Unpin must be called before the Pinner becomes unreachable or the runtime will panic."?
Patch Set #5, Line 74:
kind != kindUnsafePointer {nit: I have a preference for keeping this on the previous line
I think this should probably also defensively check the span state, to make sure this is a heap span. (span.state.get() == mSpanInUse)
same for setPinned.
I think you can make this an atomic.Uint8, which would be safer and cleaner overall.
// Ensure that the span is swept, b/c sweeping accesses the specials list
// w/o locks.
mp := acquirem()
span.ensureSwept()I think this should probably be lifted up (with releasem) to cover all of setPinned. the entire setPinned process should probably be non-preemptible, since its holding onto a span pointer. we want to avoid situations where a goroutine in setPinned is preempted, and when it comes back a GC happened and that span was freed. the current setPinned code is probably OK because holding any runtime lock will prevent preemption, *and* the span won't be freed since the setPinned path still tracks a live pointer to an object in the span, but it's better to be explicit.
I think this also needs a comment that ensureSwept is sufficient because we know we have at least one live pointer into the span, therefore sweeping the span won't release its resources.
nit: we don't use _ in new code. you can just leave this as pinnerGetCounter.
same here
Patch Set #5, Line 254:
Found leaking pinned pointer. Forgot to call Unpin()?the convention for error messages (it's not written down anywhere, just from experience) is to avoid capitalization for new sentences. I'd like to suggest something like:
"Pinner: found leaking pinned pointer; forgot to call Unpin()?"
WDYT?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
5 comments:
Patchset:
after thinking about it more, I think the implementation looks good overall. I think everything here ends up being about style and comments (there are a few suggestions for synchronization, but they're more for style than correctness.)
thanks for all your work on this!
File src/runtime/mheap.go:
if pinnerBits := s.getPinnerBits(); pinnerBits != nil {
s.setPinnerBits(nil)
I think this races with something else checking the pinner bits. […]
after a shower thought over the weekend, I think I may have spoken too soon.
isPinned is safe provided that you're always checking a valid live pointer, so the span will not take the free path. I believe this is true in every case it's called in.
I think that just means isPinned needs a comment explaining that it should only be passed a valid, live heap pointer.
File src/runtime/pinner.go:
Patch Set #5, Line 87:
isPinnedas mentioned elsewhere, I think this needs a comment that it's only safe to call on heap pointers that are known to be valid.
given that this is true, I would prefer if "p" here was an unsafe.Pointer to help indicate that.
I think this should probably also defensively check the span state, to make sure this is a heap span […]
(this is technically unnecessary, but it matches other code that looks up heap pointers in the span table, e.g. findObject.)
Patch Set #5, Line 102:
uintptrwe know this will always be passed a valid heap pointer, so I'd also prefer if this was an unsafe.Pointer to help make that more clear.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
Patchset:
I'm planning to use this API to improve delve's call injection so it would be nice if it received the attention it needs to be merged in 1.21 or early in 1.22.
Currently delve evaluates function calls in a weird way to always keep intermediate results in the target's memory, when evaluating `g(f())` we first start the call to g and then before actually letting g run we inject the call to f, so that when f finishes running we have a place (the argument frame of g) where we can copy the return value of f.
On top of being fairly convoluted code this causes a few problems:
1. Because of the weird evaluation order we have a bug where registerized variables are unavailable when we need them: https://github.com/go-delve/delve/issues/3310
2. Return values are no longer alive when we show them, so users can not, for example, load more of a returned string because it could be corrupted: https://github.com/go-delve/delve/issues/1599
3. supporting evaluation struct literals would be really annoying: https://github.com/go-delve/delve/issues/1465If we could instead keep intermediate results inside delve's memory things would be easier. With this pinning API we could do that and pin every pointer so that GC doesn't collect the objects they reference.
Besides lobbying for this CL to be reviewed and merged I wanted to also ask if we should ask for a `var debugPinner Pinner` to exist in the runtime so that we can use it, or if we can do the equivalent of:
pinner := new(runtime.Pinner)
pinner.Pin(&pinner)and expect pinner to not be garbage collected.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
6 comments:
Patchset:
@mkny...@google.com Thank you for the thorough review. If I didn't answer, it's an implicit ACK. Will push an updated patchset very soon.
File src/runtime/mheap.go:
if pinnerBits := s.getPinnerBits(); pinnerBits != nil {
s.setPinnerBits(nil)
after a shower thought over the weekend, I think I may have spoken too soon. […]
@mkny...@google.com I had the same thought, but wanted to double check before answering. So, I'm glad you came to the same conclusion, that's reassuring. Shower thought's are gold for me too. 😊 I'll make that precondition more explicit.
File src/runtime/pinner.go:
Patch Set #5, Line 20:
touchedI don't think this is actually true. […]
That's what I meant with "touch". 😊 I will make it explicit then.
Patch Set #5, Line 31:
i.pinnerUnpin()
why unpin before the panic? I'm not sure it's possible to actually catch this panic, since it's goin […]
@mkny...@google.com Defensive coding, because I was not sure if it's possible to recover from it, and it doesn't hurt. And maybe it helps with the tests. Of course I can remove it, if you prefer.
Unpin() must be called on the same Pinner, or the garbage
// collector will panic.
I'm not quite sure what this sentence is saying. you can't really call this on another Pinner. […]
TBO, I have no idea either. I guess that's a leftover from when the code looked different.
Patch Set #5, Line 87:
isPinned
as mentioned elsewhere, I think this needs a comment that it's only safe to call on heap pointers th […]
@mkny...@google.com Sure, if there is no overhead or catch-22 situations with the GC I can change that to a unsafe.Pointer.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #7 to this change.
M src/runtime/mksizeclasses.go
A src/runtime/pinner.go
A src/runtime/pinner_test.go
M src/runtime/sizeclasses.go
8 files changed, 862 insertions(+), 74 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
30 comments:
Patchset:
@mkny...@google.com I believe I addressed all of your comments. There is still one comment unresolved, because there are two possible options. Please mark as resolved, if appropriate. Thanks!
File src/runtime/cgocall.go:
Patch Set #5, Line 452:
// level, where Go pointers are allowed.
I'd appreciate an update to the comment at the top here and in this function to indicate the new pin […]
Done
Patch Set #5, Line 568:
into Go memorynit: maybe "into unpinned Go memory"?
Done
File src/runtime/cgocheck.go:
nit: maybe "an unpinned Go pointer into non-Go memory"?
Done
Patch Set #5, Line 51:
it's OK.
nit: maybe "it's safe store in C memory. The GC ensures it will not be moved or freed. […]
Done
nit: maybe "an unpinned Go pointer"?
Done
File src/runtime/mbitmap.go:
nit: unnecessary parens
Done
File src/runtime/mheap.go:
Patch Set #4, Line 56:
maxObjsPerSpan = pageSize / smallSizeDiv
I think the right thing to do here is to emit this from mksizeclasses.go into sizeclasses. […]
Done
File src/runtime/mheap.go:
if pinnerBits := s.getPinnerBits(); pinnerBits != nil {
s.setPinnerBits(nil)
@mkny...@google.com I had the same thought, but wanted to double check before answering. […]
Done
File src/runtime/pinner.go:
Patch Set #4, Line 112:
lock(&span.speciallock) // guard against concurrent calls of setPinned() on same spanIt might be necessary to aquirem() already here? I wonder what happens if the span is swept while ho […]
Done
Patch Set #4, Line 124:
systemstack(func() {
the key question is whether a stack growth could cause a lock to be acquired a second time (resultin […]
Done
Patch Set #4, Line 137:
systemstack(func() {dito
Done
File src/runtime/pinner.go:
high-level comment about documentation: please drop the parens in documentation. […]
Done
nit: unnecessary parens
Done
nit: unnecessary parens
Done
Patch Set #5, Line 20:
touched
That's what I meant with "touch". 😊 I will make it explicit then.
Done
nit: unnecessary parens
Done
Go memory, which excludes global pointer-free
// objects
reading your CL, I think this is even more specific. […]
I think originally my code worked also for global pointer-containing objects, where cgoIsGoPointer() returns true. But as you correctly point out, they have no span as they are linker allocated. SetFinalizer just ignores these cases, but I think it's better if Pinner explicitly fails in these cases, and sees it as a programming error. So, I will make it properly panicking in these cases (including a test). The problem I see with your quote is, that an "address of a composite literal" can still have no span. So we must be more strict. Since this is a very low level API, I would think it is ok to be a bit more technical in the documentation. So maybe we can just say "The object must be allocated in the Go heap memory, which excludes globally initialized objects"? That still allows the case, that you initialize a global pointer with an anonymous global function, because that is then initialized in the local scope of the function, which will end up in the heap.
The alternative would be to assume that, if cgoIsGoPointer() returns true and there is no span, the object is implicitly pinned. Then isPinned() could just always return true for these cases. But I'm not 100% sure, that this assumption is always correct.
nit: unnecessary parens
Done
Patch Set #5, Line 28:
&pinnerInstance{}
nit: in the runtime we generally try to make allocations explicit via "new", so I'd prefer this to b […]
Done
Patch Set #5, Line 31:
i.pinnerUnpin()
@mknyszek@google. […]
It doesn't affect the tests, so I removed it now anyway.
nit: unnecessary parens
Done
Unpin() must be called on the same Pinner, or the garbage
// collector will panic.
TBO, I have no idea either. I guess that's a leftover from when the code looked different.
Done
Patch Set #5, Line 74:
kind != kindUnsafePointer {nit: I have a preference for keeping this on the previous line
Done
(this is technically unnecessary, but it matches other code that looks up heap pointers in the span […]
spanOfHeap() is checking for (span.state.get() == mSpanInUse) already. (I think you mixed up with spanOf().)
I think you can make this an atomic.Uint8, which would be safer and cleaner overall.
Done
// Ensure that the span is swept, b/c sweeping accesses the specials list
// w/o locks.
mp := acquirem()
span.ensureSwept()
I think this should probably be lifted up (with releasem) to cover all of setPinned. […]
Done
nit: we don't use _ in new code. you can just leave this as pinnerGetCounter.
Done
same here
Done
Patch Set #5, Line 254:
Found leaking pinned pointer. Forgot to call Unpin()?
the convention for error messages (it's not written down anywhere, just from experience) is to avoid […]
Done
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
2 comments:
File src/runtime/pinner.go:
Patch Set #5, Line 87:
isPinned@mknyszek@google. […]
Done
Patch Set #5, Line 102:
uintptrwe know this will always be passed a valid heap pointer, so I'd also prefer if this was an unsafe. […]
Done
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
2 comments:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
"address of a composite literal" can still have no span.
I don't think this is true. to be clear, this isn't referring to:
```
// global
var x struct {
...
} = { ... }
...SetFinalizer(&x) // will panic
```it's referring to
```
// local
x := &myType{}
```or
```
// global
var x *myType = &myType{ ... }
```which is always going to be heap allocated when passed into SetFinalizer.
SetFinalizer forces an escape of any pointer you pass into it, and AFAICT Pinner does too, because of how the references are managed.
I think the current implementation _must_ do this anyway, because stack objects _can_ be moved. we don't have a way to actually stop that from happening. as a result, I believe Pinner and SetFinalizer have basically identical requirements.
(note also that values made with "new" are also not always heap-allocated; they can be stack-allocated as well.)
Patch Set #5, Line 87:
isPinned@mknyszek@google. […]
no, it should be fine in this circumstance. you can also immediately cast it to a uintptr, since a bunch of other APIs require it.
all I'm trying to suggest is to make it unsafe.Pointer in the API to make it clear to the caller that this should be a valid pointer.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Patch Set #5, Line 87:
isPinnedno, it should be fine in this circumstance. […]
Acknowledged
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Patch Set #7, Line 17:
pinnerInstancesmall nit I noticed: can you rename this type to just "pinner"? the "Instance" isn't really doing anything here.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
> "address of a composite literal" can still have no span. […]
Mmh, maybe we misunderstand each other, but that's not what I observe from looking at and stepping through the code. Please have a look at this snippet: https://go.dev/play/p/vggxiJhqoQM
Also look at this code here: https://github.com/golang/go/blob/ea8f03799692c5b6a3e0ef36aa95cf290db39b4c/src/runtime/mfinal.go#L388-L413
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner.go:
Patch Set #7, Line 17:
pinnerInstancesmall nit I noticed: can you rename this type to just "pinner"? the "Instance" isn't really doing an […]
@mkny...@google.com So, you mean you want that the two types are named "Pinner" and "pinner"? 😮 My thinking was, that the "Pinner" type is basically a pointer, and "pinnerInstance" is the counterpart, the actual instance, which is not copied when you copy the reference type "Pinner". You could also call it pinnerData or pinnerPIMPL or ... But I don't have strong feelings about it, I can change it, if you are serious. 😊
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
3 comments:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
Mmh, maybe we misunderstand each other, but that's not what I observe from looking at and stepping t […]
ah, sorry. thanks, you're right. I forgot we carved out that exception.
the broader point is that a global should be pinnable regardless of where the storage is. what I was thinking of, and what _will_ allocate, is assigning a new composite literal to a global. imagine trying to pin a global, but you don't know ahead of time whether it's going to be heap-allocated. that makes for a poor developer experience.
so my conclusion is that we should also allow pinning of the same kinds of pointers that SetFinalizer supports. in this particular case, I think it's fine to just have Pin perform the same check SetFinalizer does and ignore the pointer entirely. you only need to check this if it can't find a valid span, which should not be a common case.
File src/runtime/pinner.go:
Patch Set #7, Line 17:
pinnerInstance
@mknyszek@google. […]
yes, I'm serious. it's a common pattern to have the internal implementation of a type have the same name, just unexported.
I think this should defensively check the validity of the span beyond "nil," i.e. that it's specifically has the state mSpanInUse. same for isPinned.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
ah, sorry. thanks, you're right. I forgot we carved out that exception. […]
s/imagine trying to pin a global/imagine trying to pin a globally-stored object/
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
1 comment:
Patchset:
a couple of us discussed ways to make the pinner faster and allocate less frequently. up to you if you want to address them in this CL. I can implement them in a follow-up if not.
- replace pinnerBits with another gcBits, and copy them on each sweep (if present). the extra cost to sweeping is unlikely to be noticeable. meanwhile gcBits are much faster to allocate in general: just single atomic bump-pointer allocation. they also get zeroed much more efficiently since they get zeroed in bulk. they also have the advantage of only needing as many bits as there are objects (roughly) instead of having to make the bitmap sized to the maximum number of objects. the performance issue addressed by this switch is that most objects will not get pinned, so most pinning operations will have to allocate a new pinnerBits. this cost wouldn't show up in microbenchmarks that repeatedly pin the same object, and would probably appear insignificant if you're immediately pinning newly-allocated small objects in a loop (since objects of the same size will likely just be allocated in the same span on the same thread).
- cache a single *pinner in the P. have Pin try to get a *pinner from the P if it can, otherwise allocate. Unpin releases the *pinner back to the P, displacing anything that was already there. Ps are scanned just like any other heap object, so this should be fairly straightforward to do.
- add a fixed-sized array+length to the internal pinner to avoid an allocation for "refs" when the number of pinned objects is small (what I would assume to be common case, though I suppose experience will tell). perhaps something like an [8]unsafe.Pointer.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
1 comment:
Patchset:
cache a single *pinner in the P.
I just wanted to add: I recognize that it's possible for the user to effectively do this caching themselves by keeping their own Pinner. however, I suspect the typical use-case will be to just create the Pinner right when it's necessary (i.e. before a cgo call). it's more of a small usability win, I guess, but that's only because it more efficiently reuses *pinners by default.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner.go:
I think this should defensively check the validity of the span beyond "nil," i.e. […]
If span is not nil, it always has the state mSpanInUse, that's how spanOfHeap() works.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
s/imagine trying to pin a global/imagine trying to pin a globally-stored object/
Ok, so basically that means Pin() must support all globals, because I couldn't get SetFinalizer to bail out with globals, even if I use your first example (taking the address of a global struct), see here: https://go.dev/play/p/rE0vxmk3z56
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
2 comments:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objectss/imagine trying to pin a global/imagine trying to pin a globally-stored object/
cherryyz@ pointed out to me that locals can also get linker-allocated in the same way in certain situations. I think we need to be general here, like the SetFinalizer documentation suggests. critically, it's not possible for a stack-allocated pointer to make its way into SetFinalizer, and I think that just needs to be true for Pinner as well. but that detail definitely doesn't need to be exposed to users.
(a different stack management implementation, like segmented stacks, could for example prevent us stack objects from being moved. in that case it would be safe to pass a stack pointer in here and ignore it. we shouldn't preclude that by specifying that detail in the API.)
File src/runtime/pinner.go:
If span is not nil, it always has the state mSpanInUse, that's how spanOfHeap() works.
my bad, misread this as spanOf. yeah, that's fine.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
cherryyz@ pointed out to me that locals can also get linker-allocated in the same way in certain sit […]
yeah, that's right. the Pinner documentation should still follow SetFinalizer's lead here in not being specific about the underlying storage of what's passed to it to avoid restricting the implementation in the future.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
1 comment:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
yeah, that's right. […]
(I just reread this thread and I'm not sure what I was thinking; you definitely mentioned the SetFinalizer exception for globals earlier. 😞 sorry for the frustrating back-and-forth. my original comment here was definitely wrong.)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
(I just reread this thread and I'm not sure what I was thinking; you definitely mentioned the SetFin […]
No worries! Thanks for the deep review, I appreciate it.
I just finished implementing the link-allocated-check, and then realized that it is doing a more relaxed version of what cgoIsGoPointer() is doing anyway. And in both cases (isPinned and setPinned) that has been called already before. So, IINM this would _always_ evaluate to true. Therefore I would like to skip that check and explain that in a comment instead.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #8 to this change.
runtime: implement Pinner API for object pinning
Some C APIs require the use or structures that contain pointers to
buffers (iovec, io_uring, ...). The pointer passing rules would
require that these buffers are allocated in C memory and to process
this data with Go libraries it would need to be copied.
In order to provide a zero-copy way to use these C APIs, this CL
implements a Pinner API that allows to pin Go objects, which
guarantees that the garbage collector does not move these objects
while pinned. This allows to relax the pointer passing rules so that
pinned pointers can be stored in C allocated memory or can be
contained in Go memory that is passed to C functions.
The Pin() method accepts pointers to objects of any type and
unsafe.Pointer. Slices and arrays can be pinned by calling Pin()
with the pointer to the first element. Pinning of maps is not
supported.
If the GC collects unreachable Pinner holding pinned objects it
panics. If Pin() is called with the other non-pointer types it
panics as well.
Performance considerations: This change has no impact on execution
time on existing code, because checks are only done in code paths,
that would panic otherwise. The memory footprint on existing code is
one pointer per memory span.
Fixes: #46787
Signed-off-by: Sven Anderson <sv...@anderson.de>
Change-Id: I110031fe789b92277ae45a9455624687bd1c54f2
---
M src/runtime/cgocall.go
M src/runtime/cgocheck.go
M src/runtime/mbitmap.go
M src/runtime/mfinal.go
M src/runtime/mheap.go
M src/runtime/mksizeclasses.go
A src/runtime/pinner.go
A src/runtime/pinner_test.go
M src/runtime/sizeclasses.go
9 files changed, 902 insertions(+), 93 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #9 to this change.
9 files changed, 899 insertions(+), 93 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
3 comments:
Patchset:
> cache a single *pinner in the P. […]
Yeah, to be honest, I would prefer that is done in a separate CL. This attempt is clearly optimized to be minimal invasive for the cases where Pinner is not used, and that it is efficiently checked when it is used, but not for massive Pinning/Unpinning. I would maybe suggest to wait how it will actually be used, and if these cases would really benefit from it. Otherwise it might be premature optimization, that increases complexity and base-costs without real benefit.
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
No worries! Thanks for the deep review, I appreciate it. […]
Ok, another round of reading and thinking revealed, that we also need to cover pointers to global pointer-free objects, where cgoIsGoPointer() returns false, so I can't use it as a precondition in Pin(). (Actually cgoIsGoPointer() should be called cgoIsPointerToMemoryWhichMightContainGoPointer(), which would be less misleading.)
Now I extracted a isGoPointerWithoutSpan() from SetFinalizer() and use it in setPinned() directly, which is also more efficient. (No double iteration through activeModules.)
File src/runtime/pinner.go:
Patch Set #7, Line 17:
pinnerInstanceyes, I'm serious. […]
Done
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
Yeah, to be honest, I would prefer that is done in a separate CL. […]
That being said, I'm interested in implementing these as well. It seems straight forward after all. And I think I misunderstood the first point: so the gcBits would still only be allocated, if a Pinner is used? (Actually my first attempt was to use gcBits, but it crashed, because I didn't really understand how it works, and then gave up. Now I think I understand a bit better.)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Michael Knyszek.
Patch set 9:Run-TryBot +1
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #10 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Ian Lance Taylor, TryBot-Result-1 by Gopher Robot
A api/next/46787.txt
M src/runtime/cgocall.go
M src/runtime/cgocheck.go
M src/runtime/mbitmap.go
M src/runtime/mfinal.go
M src/runtime/mheap.go
M src/runtime/mksizeclasses.go
A src/runtime/pinner.go
A src/runtime/pinner_test.go
M src/runtime/sizeclasses.go
10 files changed, 902 insertions(+), 93 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Michael Knyszek.
Patch set 10:Run-TryBot +1
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Michael Knyszek.
1 comment:
Patchset:
Superseded.
@mkny...@google.com this flaky error is actually because of the missing "unpin()" that I removed from the finalizer. Now the test is not idempotent, and leaves objects pinned. So I need to add a possibility to unpin at least for the test case, which would add complexity. Or I just add the general unpin() back into the finalizer. WDYT?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Michael Knyszek.
1 comment:
Patchset:
That being said, I'm interested in implementing these as well. It seems straight forward after all. […]
@mkny...@google.com Actually I implemented the first point (gcBits) already. But I think I would still prefer to commit it in a follow up CL, because that needs maybe more rework and I still hope we can merge this API in 1.21. 😊 WDYT?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Sven Anderson.
2 comments:
Patchset:
Yeah, to be honest, I would prefer that is done in a separate CL.
sounds good.
I would maybe suggest to wait how it will actually be used, and if these cases would really benefit from it.
I totally agree your point here, but there's a bit of a chicken-and-egg problem in the ecosystem with Pinner.
pinning/unpinning does enable totally new behavior so for those situations you're totally right. but one of its use-cases is also potentially replacing unsafe code out there that relies on the GC not moving heap objects. if it's not fast enough at a baseline, adoption of the safer thing will be difficult. (it can of course be improved later, but the release timescales are fairly large, so I think it's worth trying to do the best we can up-front.)
And I think I misunderstood the first point: so the gcBits would still only be allocated, if a Pinner is used?
that's correct.
Patchset:
@mknyszek@google. […]
ah, ok yeah that makes sense. yeah please go ahead and add it back with a comment.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Sven Anderson.
Ansiwen uploaded patch set #11 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Ian Lance Taylor, TryBot-Result+1 by Gopher Robot
10 files changed, 931 insertions(+), 92 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
ah, ok yeah that makes sense. yeah please go ahead and add it back with a comment.
Done
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #12 to this change.
runtime: implement Pinner API for object pinning
10 files changed, 902 insertions(+), 93 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #13 to this change.
10 files changed, 939 insertions(+), 88 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #14 to this change.
10 files changed, 934 insertions(+), 88 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Sven Anderson (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner.go:
Go memory, which excludes global pointer-free
// objects
Ok, another round of reading and thinking revealed, that we also need to cover pointers to global po […]
I think I addressed this. Pin() behaves like SetFinalizer() (accepting but ignoring global vars without span) and the docs contain the same wording.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Alessandro Arzilli, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
I'm planning to use this API to improve delve's call injection so it would be nice if it received th […]
Happy to hear that even delve wants to use this API. 😊 However, I wonder if you could not just keep a reference in a list/map or use Keepalive() if you only need the GC from collecting them? Also there is cgo.Handle() which would keep objects alive as well, if I’m not mistaken. I probably didn’t really understand your use-case yet, because I guess while debugging, there are two Go runtimes involved (delve‘s and the debug target)?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Alessandro Arzilli, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
@mkny...@google.com Is there anything else I can do to make this CL progress, or should I just wait? (Im just a bit nervous because of the approaching code freeze, and I don’t want to miss a change to help merging it in time. 🤓 )
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Alessandro Arzilli, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
@mknyszek@google. […]
*chance
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Alessandro Arzilli, Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
Patchset:
*chance
I'm gonna give it one more round but it looks good to me overall. I'll make sure it gets in for the release.
thank you for your patience thus far!
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Alessandro Arzilli, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
I'm gonna give it one more round but it looks good to me overall. […]
That’s great news, thank you a lot!
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
Happy to hear that even delve wants to use this API. 😊 However, I wonder if you could not just keep a reference in a list/map or use Keepalive() if you only need the GC from collecting them?
runtime.KeepAlive doesn't do anything as an injected call, it needs to be seen by the compiler and to use a list or a map there would need to be one in the runtime.
Also there is cgo.Handle() which would keep objects alive as well, if I’m not mistaken. I probably didn’t really understand your use-case yet, because I guess while debugging, there are two Go runtimes involved (delve‘s and the debug target)?
Yes, there are two different processes which means Delve can't really just call its own cgo.Handle. It could inject a call to the one in the target process, however, but I didn't know it existed, it does not guarantee that the reference will not move, but currently it wouldn't matter.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
Patch set 14:Run-TryBot +1
4 comments:
Patchset:
LGTM overall! I took a closer look at the tests and realized we won't actually be running these anywhere in the current CI configuration.
I made a suggestion to make it work, but it will take a little bit of refactoring (hopefully not very much though). once that's resolved I think we're good to go, everything else looks good.
thanks for sticking with it!
File src/runtime/pinner_test.go:
//go:linkname cgoCheckPointer runtime.cgoCheckPointer
func cgoCheckPointer(any, any)
//go:linkname isPinned runtime.isPinned
func isPinned(unsafe.Pointer) bool
//go:linkname pinnerLeakPanic runtime.pinnerLeakPanic
var pinnerLeakPanic func()
//go:linkname getPinCounter runtime.pinnerGetPinCounter
func getPinCounter(unsafe.Pointer) *uintptrinstead of linkname, we use export_test.go for tests. it's an exported API from the runtime that's just used in testing.
please add these as wrapper functions (e.g. GetPinCounter) to what they linkname there.
Patch Set #14, Line 334:
cgoCheckPointersince this doesn't do anything if the GOEXPERIMENT isn't enabled, I think this and other tests that do this need to be folded into the test program suggestion I make below.
func TestPinnerCgoCheckWriteToNonGoMem(t *testing.T) {
// NOTE: we could also temporarily set cgocheck to 2 for this test, but
// it's global and might interfere with other parallel tests?
if !goexperiment.CgoCheck2 {
t.Skip("skipped: requires GOEXPERIMENT=cgocheck2")
}
var pinner runtime.Pinner
defer pinner.Unpin()
// this is a trick to simulate non-go memory, b/c global pointer-free
// objects are seen as non-go memory by cgoIsGoPointer.
nonGoPtr := (*unsafe.Pointer)(unsafe.Pointer(&globalUintptr))
o := new(obj)
pinner.Pin(o)
*nonGoPtr = unsafe.Pointer(o)
}because we don't have a builder for GOEXPERIMENT=cgocheck2, I think this needs to be a test program that's built with the requisite environment variable. once it's in a test program, you could even just write cgo code that generates a non-Go pointer instead of doing this trick; it's a more realistic scenario, in the end.
you can add the test program as src/runtime/testdata/testprogcgocheck2. then, you can build and test with buildTestProg and runBuiltTestProg. you may need to extend buildTestProg to support environment variables at build time.
(to avoid having to update every caller of buildTestProg, you can do something like this:
```
func buildTestProg(t *testing.T, binary string, flags ...string) (string, error) {
return buildTestProgWithEnv(t, binary, nil, flags)
}func buildTestProgWithEnv(t *testing.T, binary string, env, flags []string) (string, error) {
... // Old buildTestProg code here, with updates to use env.
}
```please also explicitly skip this test if testing.Short is true.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner_test.go:
Patch Set #14, Line 334:
cgoCheckPointersince this doesn't do anything if the GOEXPERIMENT isn't enabled, I think this and other tests that […]
I think you mix it up, cgoCheckPointer() doesn’t need GOEXPERIMENT, it’s enabled by default, and my tests test that it’s actually working (see assertCgoCheckPanics() before).
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
File src/runtime/pinner_test.go:
Patch Set #14, Line 334:
cgoCheckPointer
I think you mix it up, cgoCheckPointer() doesn’t need GOEXPERIMENT, it’s enabled by default, and my […]
I think I am wrong that it can be enabled even if the GOEXPERIMENT isn't, but AFAICT it's still disabled unless the GODEBUG setting is enabled.
am I missing something here?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
File src/runtime/pinner_test.go:
Patch Set #14, Line 334:
cgoCheckPointer
I think I am wrong that it can be enabled even if the GOEXPERIMENT isn't, but AFAICT it's still disa […]
'doh, cgocheck is enabled by default. yes, sorry, this is fine.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner_test.go:
Patch Set #14, Line 334:
cgoCheckPointer
'doh, cgocheck is enabled by default. yes, sorry, this is fine.
Right, and as I said before, my tests assert that it is enabled, so we would notice it, if the default changes.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
2 comments:
Patchset:
6 of 45 TryBots failed. […]
this looks like a real failure, since it happens on the next CL too.
File src/runtime/pinner_test.go:
Patch Set #14, Line 334:
cgoCheckPointer
Right, and as I said before, my tests assert that it is enabled, so we would notice it, if the defau […]
yep, totally right. wasn't sure the conditions you were running the tests in (but clearly the tests pass).
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
Patchset:
this looks like a real failure, since it happens on the next CL too.
(well, ok, the fact that it happens on the next CL doesn't necessarily implicate this change. :P in theory it could be that you're rebased on a bad change. however grepping through the CI logs, the last such failure I see is a one-off in August 2022, so I suspect this is indeed real.)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
(well, ok, the fact that it happens on the next CL doesn't necessarily implicate this change. […]
Hmm, I will have a look. But note that CI was happy with Patchset 10: https://go-review.googlesource.com/c/go/+/367296/14#message-1f7c071973decdee44c01cc35742e2a50831c70d
So it must be something that I introduced since then.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
Hmm, I will have a look. But note that CI was happy with Patchset 10: https://go-review. […]
@mkny...@google.com Hmm... ok, I think I know what the problem is:
1. I "fixed" the behavior of `cgoCheckPointer()` regarding nested pointers in the following way: If the checked pointer points to a pinned object A that contains other pointers to objects that are not pinned, it would say that is fine. Maybe that is not what we want? But my thinking was I can put these pointers in C memory, because they can never change, although the pinned object the pointer points to might contain unpinned pointers.
2. Because we now allow pinning of Go objects without Span, isPinned() always returns true for these. This seems to collide with the new semantics of nested pointers.I guess the way to go for now is to change back to the old behavior of `cgoCheckPointer()`, but that means all nested pointers need to be pinned as well, and more importantly `cgoCheckPointer()` would be significantly slower, because it would scan most pinned objects for nested pointers.
WDYT?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
Patchset:
If the checked pointer points to a pinned object A that contains other pointers to objects that are not pinned, it would say that is fine. Maybe that is not what we want? But my thinking was I can put these pointers in C memory, because they can never change, although the pinned object the pointer points to might contain unpinned pointers.
I think I agree. I believe the semantics that were proposed and approved are:
you can only store pointers to pinned objects in C memory. it's fine for C to access memory that pinned objects reference, but it would not be OK for C to pull out those pointers and store them elsewhere unless they were also pinned.
(almost the same as what you said, but just want to make sure we're on the same page.)
Because we now allow pinning of Go objects without Span, isPinned() always returns true for these. This seems to collide with the new semantics of nested pointers.
if I understand correctly, the problem is that linker-allocated objects are now always treated as pinned, which breaks some test of cgocheck. so I think the problem here is that cgocheck can't accurately catch this case anymore (passing an unpinned linker-allocated global, while technically safe, doesn't follow the pointer-passing rules).
I guess ideally we want to both allow linker-allocated objects *and* also track whether they're pinned for maximum cgocheck accuracy. this sounds perhaps more complex than it's worth, but I wonder to what degree users will end up relying on linker-allocated objects (an implementation detail) if cgocheck doesn't catch it.
so one possibility is to just say "cgocheck doesn't catch this case anymore" and delete the failing test. at the end of the day, it's just a checker; it's never going to catch every problem. I'm leaning toward this solution, but I'll check in with the team and circle back ASAP.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
Patchset:
this sounds perhaps more complex than it's worth
by "this" I meant tracking whether linker-allocated objects pinned. I do still think we need to support linker-allocated objects and that's not more trouble than it's worth. :)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
Patchset:
so one possibility is to just say "cgocheck doesn't catch this case anymore" and delete the failing test. at the end of the day, it's just a checker; it's never going to catch every problem. I'm leaning toward this solution, but I'll check in with the team and circle back ASAP.
checked with other team members; I think this is the preferred solution for this release. we might come back to it later, and managing these linker-allocated objects could use improvements in general. (e.g. go.dev/issue/29068)
once better infrastructure is in place, we can add the checking back in, but for now, it's OK to just say "cgocheck doesn't catch this case." we'll mention it in the release notes. we expect this case to be relatively rare.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
2 comments:
Patchset:
Another question came up to me today: if we ignore nested pointers in pinned objects, how should be the behavior for pinning pointers to interfaces? I guess it makes no sense to pin the interface struct itself, but payload data instead?
> this sounds perhaps more complex than it's worth […]
The failing test has these parameters:
```
// Passing the address of a static variable with
// pointers does matter.
name: "var1",
c: `void f13(char*** parg) {}`,
support: `var hello13 = [...]*C.char{new(C.char)}`,
body: `parg := [1]**C.char{&hello13[0]}; C.f13(&parg[0])`,
fail: true,
```
and I confirmed by stepping through with delve that it indeed does not fail, because the object is link-allocated. I think the underlying problem is, that cgoCheckPointer _does_ care about nested pointers. Also this test case would make no sense otherwise, because the code makes a difference if a link-allocated variable can contain pointers or not. The test right before that one says: "Passing the address of a static variable with no pointers doesn't matter."At the moment it looks to me that either cgoCheckPointer should in general not care about nested pointers anymore, or we need to check them also for being pinned, no?
Ok, while I typed this you answered already to disable the test. So that means we could also simplify `cgoIsGoPointer()` which at the moment would return true for some link-allocated objects, but `isPinned()` then always also returns true, so we iterate through `activeModules` twice for nothing. WDYT?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
2 comments:
Patchset:
I think the underlying problem is, that cgoCheckPointer does care about nested pointers.
right. I think the right behavior is to only care about referenced objects if the top-level object is not pinned. however for linker-allocated objects, we just say that cgocheck can't handle that.
Ok, while I typed this you answered already to disable the test. So that means we could also simplify cgoIsGoPointer() which at the moment would return true for some link-allocated objects, but isPinned() then always also returns true, so we iterate through activeModules twice for nothing. WDYT?
where do we iterate through activeModules the second time? isPinned bails out if the span is nil at all, so I think it's only checked once. (I could be missing something.) they do both return true, but I don't think the cost is very high, and I'd prefer the separation of "cgoIsGoPointer" and "isPinned" rather than having "cgoIsGoPointer" return false for a linker-allocated object which would be kind of surprising (it's still a Go value in Go-managed memory!).
I suppose it is unfortunate that we do the span lookup twice, but the span lookup is cheap (a little bit of arithmetic and then one load on linux/amd64). I suspect that anyone who cares about performance at the cgo boundary today already either disables cgocheck or makes the boundary crossing infrequent enough that it doesn't really matter. (we might want to reconsider if cgo calls are made faster, but certainly not for this release! :))
Another question came up to me today: if we ignore nested pointers in pinned objects, how should be […]
do you mean pinning `*any` for example? I think that should pin the interface-sized heap-allocated box that the `any` is placed in.
pinning an interface directly should definitely pin the underlying pointer but your CL already does that. an interface passed as the `any` arg to `Pin` will just get implicitly type-converted to `any` (i.e. it doesn't stuff the interface into another interface). (https://go.dev/ref/spec#Assignability)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
I think the underlying problem is, that cgoCheckPointer does care about nested pointers.
right. I think the right behavior is to only care about referenced objects if the top-level object is not pinned. however for linker-allocated objects, we just say that cgocheck can't handle that.
Ack.
Ok, while I typed this you answered already to disable the test. So that means we could also simplify cgoIsGoPointer() which at the moment would return true for some link-allocated objects, but isPinned() then always also returns true, so we iterate through activeModules twice for nothing. WDYT?
where do we iterate through activeModules the second time? isPinned bails out if the span is nil at all, so I think it's only checked once. (I could be missing something.)
Right, I mixed it up with `setPinned()`. So what i actually mean is, we don't have to iterate through `activeModules` at all, because `cgoIsGoPointer()` only iterates through it if the object is not in the heap or stack, and then `isPinned()` always returns true, so in the usual expression `cgoIsGoPointer(p) && !isPinned(p)` the iteration is always unnecessary when it happens. But since link-allocated objects are rare the impact is probably small.
they do both return true, but I don't think the cost is very high, and I'd prefer the separation of "cgoIsGoPointer" and "isPinned" rather than having "cgoIsGoPointer" return false for a linker-allocated object which would be kind of surprising (it's still a Go value in Go-managed memory!).
`cgoIsGoPointer()` already _does_ return `false` for a linker-allocated object, if it cannot contain a pointer, although it's a Go value in Go-managed memory. So the "surprise" is already the status-quo. ;-)
Probably best would be to directly use `inHeapOrStack()` instead of `cgoIsGoPointer()` and have something like `isLinkAllocPtrMem()` in the few cases where it is really necessary (if any).
I suppose it is unfortunate that we do the span lookup twice, but the span lookup is cheap (a little bit of arithmetic and then one load on linux/amd64). I suspect that anyone who cares about performance at the cgo boundary today already either disables cgocheck or makes the boundary crossing infrequent enough that it doesn't really matter. (we might want to reconsider if cgo calls are made faster, but certainly not for this release! :))
Ack.
do you mean pinning `*any` for example? I think that should pin the interface-sized heap-allocated box that the `any` is placed in.
Yes, that's what I meant. So, just to be clear,
```
buffer := new([512]byte)
var ifc any
ifc = buffer
pinner.Pin(&ifc)
```
would _not_ pin `buffer`, but _only_ the interface meta object. Right?
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #16 to this change.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
2 comments:
Patchset:
Right, I mixed it up with setPinned(). So what i actually mean is, we don't have to iterate through activeModules at all, because cgoIsGoPointer() only iterates through it if the object is not in the heap or stack, and then isPinned() always returns true, so in the usual expression cgoIsGoPointer(p) && !isPinned(p) the iteration is always unnecessary when it happens. But since link-allocated objects are rare the impact is probably small.
got it, thanks. that makes sense, and I think you're right that this case is a total no-op.
on top linker-allocated objects being rare for Pinner, I think `activeModules` might in general also only contain a single element. IIRC it's only longer for `-buildmode=shared` and for the `plugin` package, both of which have usability problems that prevent widespread use. (they're also not that widely applicable to begin with.) even then, I'd assume `activeModules` is very short.
cgoIsGoPointer() already does return false for a linker-allocated object, if it cannot contain a pointer, although it's a Go value in Go-managed memory. So the "surprise" is already the status-quo. ;-)
thanks for pointing that out. ugh. I don't like it, but it's a problem for another day. (that's something we'd eventually want to fix for better handling of linker-allocated objects in the GC in general, I think.)
Probably best would be to directly use inHeapOrStack() instead of cgoIsGoPointer() and have something like isLinkAllocPtrMem() in the few cases where it is really necessary (if any).
perhaps. for the sake of getting this in before the freeze, I think let's just lead with what you have. from my perspective, it's fairly straightforward to reason about, and we can always improve the performance of this case later if it becomes a problem. if you're inclined, feel free to leave a TODO, but it's up to you. I'm not too worried about this.
> do you mean pinning `*any` for example? I think that should pin the interface-sized heap-allocated […]
that's correct. just to be as clear as possible, because Pin basically forces ifc to the heap in this case (via set in pinner.ref, which will be heap-allocated via pinner, which will be heap-allocated due to SetFinalizer), there'll be a 16-byte allocation for ifc that &ifc is referring to. that allocation will be pinned.
in such a circumstance, I would expect users to pinner.Pin(ifc) if they wanted to pin "buffer".
also, AFAICT, your CL implements this behavior already.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
Ansiwen uploaded patch set #17 to this change.
10 files changed, 976 insertions(+), 87 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
2 comments:
Patchset:
that's correct. […]
Right, but I added a test case to assert that behavior.
File src/runtime/cgocall.go:
Patch Set #17, Line 489:
if isPinned(p) {@mkny...@google.com I added more test for coverage, but for this code I couldn't come up with a test. How is this reachable? And if it's too esoteric, should we rather remove the support for functions (which is not official anyway).
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
> Right, I mixed it up with setPinned(). […]
Ack
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
File src/runtime/pinner_test.go:
func TestPinnerCgoCheckWriteToNonGoMem(t *testing.T) {
// NOTE: we could also temporarily set cgocheck to 2 for this test, but
// it's global and might interfere with other parallel tests?
if !goexperiment.CgoCheck2 {
t.Skip("skipped: requires GOEXPERIMENT=cgocheck2")
}
var pinner runtime.Pinner
defer pinner.Unpin()
// this is a trick to simulate non-go memory, b/c global pointer-free
// objects are seen as non-go memory by cgoIsGoPointer.
nonGoPtr := (*unsafe.Pointer)(unsafe.Pointer(&globalUintptr))
o := new(obj)
pinner.Pin(o)
*nonGoPtr = unsafe.Pointer(o)
}
because we don't have a builder for GOEXPERIMENT=cgocheck2, I think this needs to be a test program […]
cmd/cgo/internal/testerrors/ptr_test.go already has support for cgocheck2, but that is not the right place to put it? I wonder what is the most sensible way, given that it is only a single test.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
it's fine for C to access memory that pinned objects reference, but it would not be OK for C to pull out those pointers and store them elsewhere unless they were also pinned.
I think I was wrong. thinking about pinning in the context of a moving garbage collector, I think that we need to require everything passed to C to be pinned. it would be prohibitively expensive for a moving garbage collector to figure out that an object is transitively pinned by another one when it's trying to relocate them. cgo already implicitly makes this true for top-level arguments (e.g. passing a struct).
I'm sorry for the back and forth on this, but it's tricky, and the consequences of getting it wrong are potentially severe.
Right, but I added a test case to assert that behavior.
got it, thanks!
you can only store pointers to pinned objects in C memory. it's fine for C to access memory that pinned objects reference, but it would not be OK for C to pull out those pointers and store them elsewhere unless they were also pinned.
just as a clarification, I wrote this earlier but it's wrong. but it's also not quite what your CL allows anyway. it's clear that all pointers passed through must either be pinned or top-level cgo arguments. it's not safe in general to allow passing pinned-pointer-to-memory-containing-an-unpinned-pointer because a moving GC could update the unpinned pointer at any time. (pinning could be defined as transitive, but that seems restrictive to the GC implementation.)
I knew this at some earlier point in the process, yet got confused. it's been a tough week before the freeze. apologies for all the mistakes.
I "fixed" the behavior of cgoCheckPointer() regarding nested pointers in the following way: If the checked pointer points to a pinned object A that contains other pointers to objects that are not pinned, it would say that is fine.
...
I guess the way to go for now is to change back to the old behavior of cgoCheckPointer(), but that means all nested pointers need to be pinned as well, and more importantly cgoCheckPointer() would be significantly slower, because it would scan most pinned objects for nested pointers.I think this needs to go back to the way it was, and it needs cgoCheckUnknownPointer to be transitive. this was subtle and I didn't fully grasp the change you made at the time.
I wouldn't be too worried about the performance of `cgoCheckPointer` in this case. yeah, deep trees will be expensive to check crossing the boundary, but they're also not cheap to pin either. *everything* crossing the boundary today is only one level deep. I expect with pinning the most we'll get is 2 levels deep, which shouldn't be substantially more expensive to check. :)
lastly, I also imagine performance-sensitive cgo users already disable cgocheck in production, and only use it when debugging.
again, I'm very sorry tug you around like this. this stuff is just subtle and it's important to get right, because we can't realistically change it once it goes out.
Patchset:
we should also update the pointer passing rules and the wiki at some point before release.
File src/runtime/pinner_test.go:
func TestPinnerCgoCheckWriteToNonGoMem(t *testing.T) {
// NOTE: we could also temporarily set cgocheck to 2 for this test, but
// it's global and might interfere with other parallel tests?
if !goexperiment.CgoCheck2 {
t.Skip("skipped: requires GOEXPERIMENT=cgocheck2")
}
var pinner runtime.Pinner
defer pinner.Unpin()
// this is a trick to simulate non-go memory, b/c global pointer-free
// objects are seen as non-go memory by cgoIsGoPointer.
nonGoPtr := (*unsafe.Pointer)(unsafe.Pointer(&globalUintptr))
o := new(obj)
pinner.Pin(o)
*nonGoPtr = unsafe.Pointer(o)
}
cmd/cgo/internal/testerrors/ptr_test. […]
no yeah, that works for me. I missed that. the cgo tests are unfortunately fairly scattered and clearly I don't have a good sense of where they are.
File src/runtime/pinner_test.go:
nit: I don't see a reason not to just spell out interface here.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
3 comments:
Patchset:
> you can only store pointers to pinned objects in C memory. […]
No worries, I totally understand. It _is_ confusing, and when I look at the code I would bet we are not the only one that got confused. ;-) The terminology is not very clear of what are Go pointers and what are valid pointers for cgo access.
File src/runtime/cgocall.go:
Patch Set #17, Line 489:
if isPinned(p) {
@mknyszek@google. […]
I think you oversaw this one.
File src/runtime/pinner_test.go:
nit: I don't see a reason not to just spell out interface here.
Laziness. 😜
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
1 comment:
File src/runtime/cgocall.go:
Patch Set #17, Line 489:
if isPinned(p) {
I think you oversaw this one.
yes, my apologies.
I think that you can skip the isPinned check here and also skip writing a test. I suspect, but am not 100% sure, that the cgoIsGoPointer check is trying to let through function values created from C functions. (e.g. `var x func() = C.myCFunc`). I will double-check.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
Patch Set #17, Line 489:
if isPinned(p) {
yes, my apologies. […]
Well, actually I couldn't come up with a function where `cgoIsGoPointer()` returns `true` in order to reach the `isPinned` call, that is heap-allocated or link-allocated in `data` or `bss`.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
Patch Set #17, Line 489:
if isPinned(p) {
Well, actually I couldn't come up with a function where `cgoIsGoPointer()` returns `true` in order t […]
right right, I figured. what I meant was I think this case is *only* trying to support passing C function pointers from Go (also, I think I confirmed that to be the case with https://pkg.go.dev/cmd/cgo#hdr-Go_references_to_C). passing any Go function to cgo in this way is just forbidden AFAICT because you can't actually do anything with it. the cgo tool needs to be aware of which functions C will call to generate wrappers for them. (and, passing a closure pointer to access closure data is way outside the bounds of safety.)
thus, my conclusion is to just delete the isPinned check here and skip writing a test. if you're so inclined you can leave a comment explaining it.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
1 comment:
Patchset:
it's not safe in general to allow passing pinned-pointer-to-memory-containing-an-unpinned-pointer because a moving GC could update the unpinned pointer at any time.
It's a matter of definition what means "safe" in a context where we use all kinds of unsafe stuff. 😊 But you are fight, there is no clear cut how many indirections C code would be allowed to follow pointers.
My thinking came more from the other case, where Go pointer are stored in C memory. There it is only important, that the pointers in the C memory are static, because the GC can't reach them in order to update them. But if you are thinking long enough about it, you realize that both problems are essentially identical.
So yeah, now I also agree that it's better to consistently follow the pointers also in pinned objects like it does for top-level objects.
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
Ansiwen uploaded patch set #18 to this change.
M src/cmd/cgo/internal/testerrors/ptr_test.go
M src/runtime/cgocall.go
M src/runtime/cgocheck.go
M src/runtime/export_test.go
M src/runtime/mbitmap.go
M src/runtime/mfinal.go
M src/runtime/mheap.go
M src/runtime/mksizeclasses.go
A src/runtime/pinner.go
A src/runtime/pinner_test.go
M src/runtime/sizeclasses.go
12 files changed, 982 insertions(+), 91 deletions(-)
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Ansiwen (Gerrit)
Attention is currently required from: Austin Clements, Ian Lance Taylor, Michael Knyszek.
5 comments:
Patchset:
> it's not safe in general to allow passing pinned-pointer-to-memory-containing-an-unpinned-pointer […]
Done
File src/runtime/cgocall.go:
Patch Set #17, Line 489:
if isPinned(p) {
right right, I figured. […]
Done
File src/runtime/pinner_test.go:
//go:linkname cgoCheckPointer runtime.cgoCheckPointer
func cgoCheckPointer(any, any)
//go:linkname isPinned runtime.isPinned
func isPinned(unsafe.Pointer) bool
//go:linkname pinnerLeakPanic runtime.pinnerLeakPanic
var pinnerLeakPanic func()
//go:linkname getPinCounter runtime.pinnerGetPinCounter
func getPinCounter(unsafe.Pointer) *uintptrinstead of linkname, we use export_test.go for tests. […]
Done
func TestPinnerCgoCheckWriteToNonGoMem(t *testing.T) {
// NOTE: we could also temporarily set cgocheck to 2 for this test, but
// it's global and might interfere with other parallel tests?
if !goexperiment.CgoCheck2 {
t.Skip("skipped: requires GOEXPERIMENT=cgocheck2")
}
var pinner runtime.Pinner
defer pinner.Unpin()
// this is a trick to simulate non-go memory, b/c global pointer-free
// objects are seen as non-go memory by cgoIsGoPointer.
nonGoPtr := (*unsafe.Pointer)(unsafe.Pointer(&globalUintptr))
o := new(obj)
pinner.Pin(o)
*nonGoPtr = unsafe.Pointer(o)
}
no yeah, that works for me. I missed that. […]
Done
File src/runtime/pinner_test.go:
Laziness. […]
Done
To view, visit change 367296. To unsubscribe, or for help writing mail filters, visit settings.
Michael Knyszek (Gerrit)
Attention is currently required from: Ansiwen, Austin Clements, Ian Lance Taylor.
Patch set 18:Run-TryBot +1