Skip to content

Commit

Permalink
runtime: fix conflict between lfstack and checkptr
Browse files Browse the repository at this point in the history
lfstack does very unsafe things. In particular, it will not
work with nodes that live on the heap. In normal use by the runtime,
that is the case (it is only used for gc work bufs). But the lfstack
test does use heap objects. It goes through some hoops to prevent
premature deallocation, but those hoops are not enough to convince
-d=checkptr that everything is ok.

Instead, allocate the test objects outside the heap, like the runtime
does for all of its lfstack usage. Remove the lifetime workaround
from the test.

Reported in https://groups.google.com/g/golang-nuts/c/psjrUV2ZKyI

Change-Id: If611105eab6c823a4d6c105938ce145ed731781d
Reviewed-on: https://go-review.googlesource.com/c/go/+/448899
Reviewed-by: Austin Clements <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Keith Randall <[email protected]>
  • Loading branch information
randall77 committed Nov 17, 2022
1 parent 3e5c2c1 commit d6171c9
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 19 deletions.
9 changes: 9 additions & 0 deletions src/runtime/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ func LFStackPush(head *uint64, node *LFNode) {
func LFStackPop(head *uint64) *LFNode {
return (*LFNode)(unsafe.Pointer((*lfstack)(head).pop()))
}
func LFNodeValidate(node *LFNode) {
lfnodeValidate((*lfnode)(unsafe.Pointer(node)))
}

func Netpoll(delta int64) {
systemstack(func() {
Expand Down Expand Up @@ -1709,3 +1712,9 @@ func BlockUntilEmptyFinalizerQueue(timeout int64) bool {
func FrameStartLine(f *Frame) int {
return f.startLine
}

// PersistentAlloc allocates some memory that lives outside the Go heap.
// This memory will never be freed; use sparingly.
func PersistentAlloc(n uintptr) unsafe.Pointer {
return persistentalloc(n, 0, &memstats.other_sys)
}
6 changes: 4 additions & 2 deletions src/runtime/lfstack.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ import (
// This stack is intrusive. Nodes must embed lfnode as the first field.
//
// The stack does not keep GC-visible pointers to nodes, so the caller
// is responsible for ensuring the nodes are not garbage collected
// (typically by allocating them from manually-managed memory).
// must ensure the nodes are allocated outside the Go heap.
type lfstack uint64

func (head *lfstack) push(node *lfnode) {
Expand Down Expand Up @@ -59,6 +58,9 @@ func (head *lfstack) empty() bool {
// lfnodeValidate panics if node is not a valid address for use with
// lfstack.push. This only needs to be called when node is allocated.
func lfnodeValidate(node *lfnode) {
if base, _, _ := findObject(uintptr(unsafe.Pointer(node)), 0, 0); base != 0 {
throw("lfstack node allocated from the heap")
}
if lfstackUnpack(lfstackPack(node, ^uintptr(0))) != node {
printlock()
println("runtime: bad lfnode address", hex(uintptr(unsafe.Pointer(node))))
Expand Down
31 changes: 14 additions & 17 deletions src/runtime/lfstack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ type MyNode struct {
data int
}

// allocMyNode allocates nodes that are stored in an lfstack
// outside the Go heap.
// We require lfstack objects to live outside the heap so that
// checkptr passes on the unsafe shenanigans used.
func allocMyNode(data int) *MyNode {
n := (*MyNode)(PersistentAlloc(unsafe.Sizeof(MyNode{})))
LFNodeValidate(&n.LFNode)
n.data = data
return n
}

func fromMyNode(node *MyNode) *LFNode {
return (*LFNode)(unsafe.Pointer(node))
}
Expand All @@ -30,22 +41,17 @@ func TestLFStack(t *testing.T) {
stack := new(uint64)
global = stack // force heap allocation

// Need to keep additional references to nodes, the stack is not all that type-safe.
var nodes []*MyNode

// Check the stack is initially empty.
if LFStackPop(stack) != nil {
t.Fatalf("stack is not empty")
}

// Push one element.
node := &MyNode{data: 42}
nodes = append(nodes, node)
node := allocMyNode(42)
LFStackPush(stack, fromMyNode(node))

// Push another.
node = &MyNode{data: 43}
nodes = append(nodes, node)
node = allocMyNode(43)
LFStackPush(stack, fromMyNode(node))

// Pop one element.
Expand Down Expand Up @@ -75,8 +81,6 @@ func TestLFStack(t *testing.T) {
}
}

var stress []*MyNode

func TestLFStackStress(t *testing.T) {
const K = 100
P := 4 * GOMAXPROCS(-1)
Expand All @@ -86,15 +90,11 @@ func TestLFStackStress(t *testing.T) {
}
// Create 2 stacks.
stacks := [2]*uint64{new(uint64), new(uint64)}
// Need to keep additional references to nodes,
// the lock-free stack is not type-safe.
stress = nil
// Push K elements randomly onto the stacks.
sum := 0
for i := 0; i < K; i++ {
sum += i
node := &MyNode{data: i}
stress = append(stress, node)
node := allocMyNode(i)
LFStackPush(stacks[i%2], fromMyNode(node))
}
c := make(chan bool, P)
Expand Down Expand Up @@ -134,7 +134,4 @@ func TestLFStackStress(t *testing.T) {
if sum2 != sum {
t.Fatalf("Wrong sum %d/%d", sum2, sum)
}

// Let nodes be collected now.
stress = nil
}

0 comments on commit d6171c9

Please sign in to comment.