all: get standard library building with -d=checkptr

[mdempsky]

As a prerequisite for #34964, the standard library's tests need to all pass when -d=checkptr is enabled:

$ go test -a -short -gcflags=all=-d=checkptr std cmd

Currently, that's not the case.

1. There are also some panics because of (*[Big]T)(p)[:n] expressions, like in package reflect. This code pattern should be recognized by cmd/compile.

2. There are a few tests that fail because CL 201781 leads them to heap allocate when before things would only stack allocate; e.g., context.TestAllocs or database/sql.TestRawBytesAllocs. Not sure how to handle this; maybe for now the escape analysis change should only be enabled for -d=checkptr=2, and -race/-msan only enable -d=checkptr=1.

3. In sync/atomic.hammerStoreLoadPointer, there's (in effect):

    new := uintptr(LoadPointer(addr))
    new += offset
    StorePointer(addr, unsafe.Pointer(new))
   

   This violates package unsafe's pointer rules: pointers have to be converted to uintptr and back to unsafe.Pointer in a single expression without being stored in a uintptr-typed variable, as is being done with new. (This just needs someone to grok exactly what the test is doing, and fix it to follow pointer rules correctly.)

There seem to be other failures still that need to be diagnosed. If folks want to try running the command above, claiming a failure, and then investigating what's going on, that would be great.

[mdempsky]

/cc @odeke-em @cuonglm

[mdempsky]

This code in package reflect is currently failing in checkptrAlignment, and seems to violate the safety rules because of the conversion to *[16]byte (sometimes gcdata points to <16 bytes):

go/src/reflect/type.go

Lines 2184 to 2192 in 8c6876e

	kmask := (*[16]byte)(unsafe.Pointer(ktyp.gcdata))
	for i := uintptr(0); i < ktyp.ptrdata/ptrSize; i++ {
	if (kmask[i/8]>>(i%8))&1 != 0 {
	for j := uintptr(0); j < bucketSize; j++ {
	word := base + j*ktyp.size/ptrSize + i
	mask[word/8] |= 1 << (word % 8)
	}
	}
	}

(There's similar code for etype just below.)

[odeke-em]

I'll cover the sync/atomic.hammer* tests fixup :)

[mdempsky]

This failure looks genuinely suspicious to me:

$ go test -a -short -gcflags=all=-d=checkptr testing/quick
runtime: pointer 0xc00022a3e0 to unallocated span span.base()=0xc000222000 span.limit=0xc00023a000 span.state=3
fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)

[gopherbot]

Change https://golang.org/cl/201841 mentions this issue: all: disable tests that fail under -d=checkptr

[gopherbot]

Change https://golang.org/cl/201839 mentions this issue: cmd/compile: recognize (*[Big]T)(ptr)[:n:m] pattern for -d=checkptr

[mdempsky]

Uploaded a couple CLs to improve -d=checkptr, and address some issues.

CL 201841 enumerates all of the remaining std cmd test failures that I'm aware of, but haven't investigated yet.

[odeke-em]

Hey @mdempsky in regards to the sync/atomic.HammerLoad* tests that are flagged, what they do is that for 8 goroutines, run pairwise increments of the high and low parts of an int* or *uint and panic if high != low. To save the value at the end of the function, we invoke

StorePointer(addr, newValue)

where newValue is a raw uint64 value such as 0xc0000000c0 and that value is being written to addr.

checkptr tries to find the object associated with newValue but unfortunately that's a raw value that's not attached to any object and perhaps beyond Go's address space. Writing such code is a valid usecase and here is a minimal repro to test that edge case

package main

import (
	"fmt"
	"sync/atomic"
	"unsafe"
)

func main() {
	val := uint64(820338753727)
	addr := (*unsafe.Pointer)(unsafe.Pointer(&val))
	v := uintptr(atomic.LoadPointer(addr))
	atomic.StorePointer(addr, unsafe.Pointer(v+1+1<<32))

	xs := uintptr(atomic.LoadPointer(addr))
	fmt.Printf("xs: %#x\n", xs)
}

$ go run -gcflags=all=-d=checkptr main.go 
panic: (runtime.ptrArith) (0x10c09c0,0xc00000c080)

goroutine 1 [running]:
main.main()
	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/34972/main.go:13 +0x91
exit status 2

Trying to recognize cases that do plain arithmetic is going to be a tricky one :)

[gopherbot]

Change https://golang.org/cl/201877 mentions this issue: syscall: fix wrong unsafe.Pointer align in anyToSockaddr

[cuonglm]

Another instance in reflect:

go/src/reflect/type.go

Line 1011 in 8c6876e

return (*[1 << 20]*rtype)(add(unsafe.Pointer(t), uadd, "t.inCount > 0"))[:t.inCount]

and some lines below:

go/src/reflect/type.go

Line 1023 in 8c6876e

return (*[1 << 20]*rtype)(add(unsafe.Pointer(t), uadd, "outCount > 0"))[t.inCount : t.inCount+outCount]

[mdempsky]

@odeke-em Thanks for looking into it.

Writing such code is a valid usecase

Regardless of the merit of the use case, if the example violates package unsafe's safety rules, then it's not valid Go.

Your repro case can be simplified to just:

package main

import (
	"fmt"
	"unsafe"
)

func main() {
	fmt.Println(unsafe.Pointer(uintptr(0xc000000000)))
}

This is failing because 0xc000000000 is an offset into the Go heap (at least where Go tries to allocate it by default on 64-bit OSes), and package unsafe's safety rules don't allow you to fabricate a pointer into the heap out of nothing.

To fix the test, it should either:

1. Exclusively use pointers outside of the Go heap. (This is challenging, because you don't know where the Go heap is, and allocating non-Go-heap memory in a portable way is tricky.)

2. Alternatively, allocate a large chunk of memory, find an adequately aligned pointer within that chunk of memory, and then restrict to modifying the lower bits of the address. For example:

    buf := make([]byte, 1 << 21) // allocate 2MB
    p := unsafe.Pointer(&buf[0])
    p = unsafe.Pointer((uintptr(p) + 1<<20 - 1) &^ (1<<20 - 1))  // align p to 1MB boundary
   

   p is now 20-bit aligned, and you can change any of the lower 20 bits and be guaranteed it still points into buf.

[bcmills]

There are also some panics because of (*[Big]T)(p)[:n] expressions, like in package reflect. This code pattern should be recognized by cmd/compile.

I saw that one coming in #13656 (comment)!

One could easily envision a compiler that, say, sanity-checks that all pointers to arrays point to addresses that are mapped in the program's address space.