cmd/compile: -d=checkptr should not reject unaligned pointers to non-pointer data

[cespare]

I'm testing some code with Go 1.14 and found that some tests are failing which passed in Go 1.13 while running under -race.

This is due to the new checkptr checks that are enabled by -race. Here's a minimized repro:

// +build ignore

package main

import (
	"fmt"
	"reflect"
	"unsafe"
)

func main() {
	b := make([]byte, 20)
	s := unsafeInts(b, 1) // with 0 or 8, no failure
	fmt.Println(s)
}

func unsafeInts(b []byte, offs int) []int64 {
	var s []int64
	sh := (*reflect.SliceHeader)(unsafe.Pointer(&s))
	sh.Data = uintptr(unsafe.Pointer(&b[offs]))
	sh.Cap = (len(b) - offs) / 8
	sh.Len = sh.Cap
	return s
}

$ go1.13.4 run -race checkptr.go
[0 0]
$ go1.14beta1 run -race checkptr.go
panic: runtime error: unsafe pointer conversion

goroutine 1 [running]:
reflect.Value.Int(...)
        /home/caleb/sdk/go1.14beta1/src/reflect/value.go:974
fmt.(*pp).printValue(0xc0000a08f0, 0x5150a0, 0xc0000bc001, 0x186, 0x76, 0x1)
        /home/caleb/sdk/go1.14beta1/src/fmt/print.go:749 +0x3694
fmt.(*pp).printValue(0xc0000a08f0, 0x5136e0, 0xc0000aa020, 0x97, 0x76, 0x0)
        /home/caleb/sdk/go1.14beta1/src/fmt/print.go:869 +0xfd3
fmt.(*pp).printArg(0xc0000a08f0, 0x5136e0, 0xc0000aa020, 0x76)
        /home/caleb/sdk/go1.14beta1/src/fmt/print.go:716 +0x25b
fmt.(*pp).doPrintln(0xc0000a08f0, 0xc00005af50, 0x1, 0x1)
        /home/caleb/sdk/go1.14beta1/src/fmt/print.go:1173 +0xae
fmt.Fprintln(0x55d520, 0xc0000b8008, 0xc00005af50, 0x1, 0x1, 0x432142, 0xc000092058, 0x0)
        /home/caleb/sdk/go1.14beta1/src/fmt/print.go:264 +0x66
fmt.Println(...)
        /home/caleb/sdk/go1.14beta1/src/fmt/print.go:274
main.main()
        /home/caleb/p/misc/checkptr/checkptr.go:14 +0x18f
exit status 2
$

This error isn't particularly helpful (why is it unsafe?) and when I looked at the source, it still wasn't clear to me what the problem is.

	// Check that (*[n]elem)(p) is appropriately aligned.
	// TODO(mdempsky): What about fieldAlign?
	if uintptr(p)&(uintptr(elem.align)-1) != 0 {
		throw("checkptr: unsafe pointer conversion")
	}

Even if there's some problem with unaligned pointers into the Go heap, in my application the backing data doesn't come from make([]byte) but rather from unix.Mmap. And my CPU (amd64) should have no problem with reading unaligned data, right?

It's also strange that the crash didn't happen in my code, but inside the reflect-y fmt code that looked at my data. (Not that it matters, but in my real code the crash isn't triggered by fmt but rather by go-cmp, also via use of reflect.Value.Int.)

/cc @mdempsky

[cespare]

@bcmills thanks for the title update. Besides answering the "should it be allowed" question, it would be nice if the error message could say what the problem is (e.g., it would be good to include the word "unaligned").

[bcmills]

@mdempsky, @randall77, @aclements, @ianlancetaylor, @griesemer: I think this issue needs a decision before Go 1.14.1. The spec and unsafe documentation are currently unclear as to whether unaligned non-pointer data should be allowed.

As a result, users may perceive -d=checkptr (and, by extension, -race mode) as having false-positives that should be ignored or otherwise suppressed. If I understand correctly, the default settings for -race mode should have no false-positives, precisely to avoid this sort of perception.

If unaligned integer reads on these architectures should not be treated as an error, then I think it is important for Go 1.14.1 to no longer flag them in -race mode, perhaps by dropping all or part of the alignment check.

On the other hand, if unaligned reads really should be disallowed, then we need to know that sooner rather than later so that we can fix violations (such as #37644) appropriately.

[bcmills]

@cespare, the clarity of the error messages is #37488. Let's leave that as a separate issue.

[gopherbot]

Change https://golang.org/cl/222855 mentions this issue: sha3: mark xorInUnaligned with go:nocheckptr