Repository: go
Issue: proposal: runtime: add a pointer check code to gcWriteBarrier for helping debugging zombie pointers
Labels: Proposal

<!--  
Our proposal process is documented here:  
[https://go.dev/s/proposal-process](https://go.dev/s/proposal-process)  
-->  
  
## Background  
Zombie pointer problem is very difficult to debug. Now GoGC will only report this issue in scanning objects, even though Go GC will panic here, information is not enough for debugging (reportZombies will print this span, but won't tell developer how this pointer collected to gcw, developer will have to find out where this memory address comes from.).  
  
Currently there are two ways that could put pointers to gcw: `greyobject` and `gcWriteBarrier` (and its cx, dx... derivation). `greyobject` already has `debug.gccheckmark` flag to validate if this pointer is ok, however `gcWriteBarrier` has no corresponding way to do the validation.  
  
For example, a misuse `atomic.StorePointer` won't have any errors reported, if developers don't pay attention to the correctness of this value. (Espcially when this field is an ID or some other int64 fields have no real world meaning) [code snippet](https://go.dev/play/p/J-HJCiZvlFg) 
  
```go  
package main

import (
	"fmt"
	"sync/atomic"
	"unsafe"
)

type struct1 struct {
	Field int64
}

type struct1ptr *struct1

var s1 struct1ptr = &struct1{
	1,
}

func main() {
	f1 := struct1{
		Field: 0,
	}
	atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(s1)), unsafe.Pointer(&f1))
	fmt.Println(s1.Field)
}

```  
  
Moreover, if this function is working under heavy load, this wrong assign may leads to a zombie pointer which is hard to debug, [link](https://go.dev/play/p/tUVc81fYRAo) As this panic will only tell developers which `mspan` contains zombie pointer, but no information about where this pointer is allocated.

```go
package main

import (
	"fmt"
	"runtime"
	"sync"
	"sync/atomic"
	"time"
	"unsafe"
)

var current time.Time

func main() {
	var wg sync.WaitGroup
	for i := 0; i < 1000; i++ {
		wg.Add(2)
		go func() {
			defer wg.Done()
			rc := time.Now()
			atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&current)), unsafe.Pointer(&rc))
		}()
		go func() {
			defer wg.Done()
			fmt.Printf("%v\n", current.Unix())
		}()
		runtime.GC()
	}
	wg.Wait()
	fmt.Println("===over===")
}
```

Another case is unsafe type conversion. [playground](https://go.dev/play/p/4DGlXgSj0YZ) Same `UserInfo` from different RPCs may leads developer to use unsafe.Pointer to elimate new allocation performance loss and avoid writing too long code for mapping field. But after IDL files updated, `UserInfoFromARPC` and `UserInfoFromBRPC` may be different, and there is a high probability that the newly added fields will be of pointer type, since new fields are often optional.

```go
package main

import (
	"fmt"
	"runtime"
	"sync"
	"unsafe"
)

type UserInfoFromARPC struct {
	ID  int64
	Age int64
}

type UserInfoFromBRPC struct {
	ID    int64
	Age   int64
	Extra *string // this field was introduced after UnsafeQueryB is finished, and generated by some idl generator
}

func Query() *UserInfoFromARPC {
	return &UserInfoFromARPC{
		ID:  100,
		Age: 100,
	}
}

func UnsafeQueryB() *UserInfoFromBRPC {
	return (*UserInfoFromBRPC)(unsafe.Pointer(Query()))
}
func main() {
	var wg sync.WaitGroup
	for i := 0; i < 1000; i++ {
		wg.Add(1)
		go func() {
			fmt.Println(UnsafeQueryB())
			wg.Done()
		}()
		runtime.GC()
	}
	wg.Wait()
}

```
## Proposal  
  
### Idea in brief  
  
Add a macro-controlled code section in gcWriteBarrier, gcWriteBarrier will try to check pointer validation before store it. If the pointer is illegal, panic here. In general, the panic stack will have the frame of the wrong "allocation" happened function. Developer could use this to find their bugs easier.
  
### Why choose to modify gcWriteBarrier  
  
ZombiePointers will only happened when this wrong pointer has been esacped to heap. If a pointer doesn't escaped to heap, it won't cause zombie pointer at least.
  
### Why choose to using a macro instead of a flag  
  
As `gcWriteBarrier` is a frequently called function, using macro will avoid changing regular code. Only when developers find there is a zombie pointer in their program and it's reproducible, they could compile codes with `-+` and this flag to recompile runtime and enable these tracking code, and waiting for panic to find out where the bug is.

### Cons
1. Controlling compiled code by c macro isn't a common way in go
> but maybe least performance overhead?

PS: should we modify `reportZombies` to make it not panic when `invalidptr=0` is set?