octobered
commented
2 years ago
## 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(¤t)), 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?
ianlancetaylor
ericlagergren
cherrymui
guodongli-google
randall77