X tracing: Fix bug when probe detach did not work (its .prev was nil so no way...

X tracing: Fix bug when probe detach did not work (its .prev was nil so no way to modify original probe list)

go/xcommon/tracing/runtime.go

@@ -27,14 +27,15 @@ import _ "unsafe"
 //go:linkname runtime_stopTheWorld runtime.stopTheWorld
 //go:linkname runtime_startTheWorld runtime.startTheWorld
 
-// runtime_stopTheWorld returns with the world stopped
+// runtime_stopTheWorld returns with the world stopped.
+//
 // Current goroutine remains the only one who is running, with others
 // goroutines stopped at safe GC points.
 // It requires careful programming as many things that normally work lead to
-// fatal errors when the world is stoppped - for example using timers would be
+// fatal errors when the world is stopped - for example using timers would be
 // invalid, but adjusting plain values in memory is ok.
 func runtime_stopTheWorld(reason string)
 
-// StartTheWorld restarts the world after it was stopped by runtime_stopTheWorld
+// runtime_startTheWorld restarts the world after it was stopped by runtime_stopTheWorld
 func runtime_startTheWorld()
 

go/xcommon/tracing/tracing.go

@@ -200,6 +200,7 @@ package tracing
 import (
 	"sync"
 	"sync/atomic"
+	"unsafe"
 
 	"fmt"
 )
@@ -245,7 +246,9 @@ func verifyUnlocked() {
 
 // Probe describes one probe attached to a tracepoint
 type Probe struct {
-	prev, next *Probe
+	// NOTE .next must come first as probe list header is only 1 word and
+	// is treated as *Probe on probe attach/detach - accessing/modifying its .next
+	next, prev *Probe
 
 	// implicitly:
 	// probefunc  func(some arguments)
@@ -270,17 +273,12 @@ func AttachProbe(pg *ProbeGroup, listp **Probe, probe *Probe) {
 		panic("attach probe: probe is not newly created")
 	}
 
-	var last *Probe
-	for p := *listp; p != nil; p = p.next {
-		last = p
+	last := (*Probe)(unsafe.Pointer(listp))
+	for p := *listp; p != nil; last, p = p, p.next {
 	}
 
-	if last != nil {
-		last.next = probe
-		probe.prev = last
-	} else {
-		*listp = probe
-	}
+	last.next = probe
+	probe.prev = last
 
 	if pg != nil {
 		pg.Add(probe)
@@ -294,7 +292,7 @@ func (p *Probe) Detach() {
 	verifyLocked()
 
 	// protection: already detached
-	if p.prev == p {
+	if p.prev == nil {
 		return
 	}
 
@@ -302,9 +300,7 @@ func (p *Probe) Detach() {
 	// - no reader is currently reading it
 	// - either a reader already read prev.next, and will proceed with our probe entry, or
 	// - it will read updated prev.next and will proceed with p.next probe entry
-	if p.prev != nil {
-		p.prev.next = p.next
-	}
+	p.prev.next = p.next
 
 	// we can safely change next.prev pointer:
 	// - readers only go through list forward
@@ -315,7 +311,8 @@ func (p *Probe) Detach() {
 
 	// mark us detached so that if Detach is erroneously called the second
 	// time it does not do harm
-	p.prev = p
+	p.prev = nil
+	p.next = nil
 }
 
 // ProbeGroup is a group of probes attached to tracepoints.

go/xcommon/tracing/tracing_test.go

+// Copyright (C) 2017  Nexedi SA and Contributors.
+//                     Kirill Smelkov <kirr@nexedi.com>
+//
+// This program is free software: you can Use, Study, Modify and Redistribute
+// it under the terms of the GNU General Public License version 3, or (at your
+// option) any later version, as published by the Free Software Foundation.
+//
+// You can also Link and Combine this program with other software covered by
+// the terms of any of the Free Software licenses or any of the Open Source
+// Initiative approved licenses and Convey the resulting work. Corresponding
+// source of such a combination shall include the source code for all other
+// software used.
+//
+// This program is distributed WITHOUT ANY WARRANTY; without even the implied
+// warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+//
+// See COPYING file for full licensing terms.
+// See https://www.nexedi.com/licensing for rationale and options.
+
+package tracing
+
+import (
+	"reflect"
+	"testing"
+	"unsafe"
+
+	"github.com/kylelemons/godebug/pretty"
+)
+
+func TestAttachDetach(t *testing.T) {
+	var traceX *Probe	// list head of a tracing event
+
+	// check that traceX probe list has such and such content and also that .prev
+	// pointers in all elements are right
+	checkX := func(probev ...*Probe) {
+		t.Helper()
+		var pv []*Probe
+
+		pp := (*Probe)(unsafe.Pointer(&traceX))
+		for p := traceX; p != nil; pp, p = p, p.next {
+			if p.prev != pp {
+				t.Fatalf("probe list: %#v: .prev is wrong", p)
+			}
+			pv = append(pv, p)
+		}
+
+		if !reflect.DeepEqual(pv, probev) {
+			t.Fatalf("probe list:\n%s\n", pretty.Compare(probev, pv))
+		}
+	}
+
+	checkX()
+
+	// attach probe to traceX
+	attachX := func(probe *Probe) {
+		Lock()
+		AttachProbe(nil, &traceX, probe)
+		Unlock()
+	}
+
+	// detach probe
+	detach := func(probe *Probe) {
+		Lock()
+		probe.Detach()
+		Unlock()
+	}
+
+	p1 := &Probe{}
+	attachX(p1)
+	checkX(p1)
+
+	detach(p1)
+	checkX()
+	detach(p1)
+	checkX()
+	attachX(p1)
+	checkX(p1)
+
+	p2 := &Probe{}
+	attachX(p2)
+	checkX(p1, p2)
+
+	p3 := &Probe{}
+	attachX(p3)
+	checkX(p1, p2, p3)
+
+	detach(p2)
+	checkX(p1, p3)
+
+	detach(p1)
+	checkX(p3)
+
+	detach(p3)
+	checkX()
+}