@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").

@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.

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

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

I think that the go:nocheckptr escape hatch is decent. For public libraries, _amd64.go files can use that directly while the generic files can Do The Right Thing?

@twmb, my concern is that if we allow checkptr to have false-positives, folks will add go:nocheckptr annotations assuming amd64 semantics even on non-amd64-specific functions — or, worse, will add annotations thinking they are working around a spurious alignment crash, and also end up suppressing a true-positive that was masked behind it.

I don't see why this should be architecture specific. The point of -d=checkptr is to check for bad code. That shouldn't be architecture specific. Bad code can run correctly on some architectures and not others, but we should always warn for it.

So I think the question is: should -d=checkptr report conversions from unsafe.Pointer to an unaligned pointer? Clearly we should report conversions to an unaligned pointer to pointer type. It's less clear for a pointer to integer type.

I don't have a good sense of the philosophy of -d=checkptr so I don't know what the answer is.

Marking as release-blocker per #37298 (comment). (FYI @toothrot @cagedmantis)

We might need to just get some compiler & runtime folks on a videoconference to figure this out...

@ianlancetaylor, the question is, are unaligned integer reads “bad code” when you know (by build constraint) that the architecture supports them, or are they always bad regardless for some reason (such as a current or potential-future detail of the garbage collector)?

If they're always bad, then checkptr should stand and the go:nocheckptr annotation in x/crypto/sha3 should be removed and the code fixed. But if they're only bad on specific architectures, then -d=checkptr should only flag them in source files that would be included for those architectures (or only when running on those architectures).

I think in a sense this comes down to a question of whether unsafe.Alignof indicates "required alignment" or only "preferred alignment."

My rationale behind -d=checkptr is based on interpreting unsafe.Alignof as required alignment. That is, (*T)(unsafe.Pointer(u)) is only valid if u % unsafe.Alignof(*new(T)) == 0. And we define unsafe.Alignof(*new(int)) to 8 even on amd64, hence -d=checkptr raising an error.

The rationale of code like sha3's xor_unaligned though seems to be that unsafe.Alignof is merely preferred alignment. That is, that the Go runtime will guarantee variables have that alignment because it's fastest, but (depending on the target CPU/OS) users may be able to still access variables with weaker alignment. And the question then is whether -d=checkptr should recognize these CPU/OS-specific cases.

One option would be: on x86, the compilers could define unsafe.Alignof(*new(T)) == 1 for all numeric types T (i.e., required alignment), but then still layout variables in memory as we do today (i.e., with preferred alignment). The Go spec only requires that variables be at least aligned according to unsafe.Alignof; it doesn't preclude a compiler/runtime from consistently aligning them more generally.

It sounds like three of our options are:

1. Declare unaligned pointers to be always bad on all architectures; delete the go:nocheckptr annotation; update any flagged code
2. Declare unaligned pointers okay on x86; remove the alignment check on these architectures; possibly update unsafe.Alignof as @mdempsky mentioned in the preceding comment
3. Declare unaligned pointers to be bad unless annotated with go:nocheckptr; update code to use this annotation where necessary

(3) seems to be our de facto state in Go 1.14, but I don't think it's workable. [Edit: deleted some incorrect stuff here.] In my case, the flagged conversion occurs in reflect code called via fmt (demo code) or go-cmp (real code). There's no place I can stick a go:nocheckptr annotation to make the problem go away. In general, the check may not be be triggered by the code that constructs the unaligned pointer, but instead by the code that reads it, which may be in any (possibly third-party) package.

The annotation indicates that a function is expected to issue an unaligned read (correct?)

No, checkptr only checks unsafe.Pointer conversions. It doesn't instrument every possible read/write using a pointer.

I don't think we should change the unsafe.Alignof values at this point.

I would still like to more clearly understand the philosophy of -d=checkptr: should it only report cases that never work, or should it report cases that can work but likely will not? It seems to me, based on the work done to rewrite expressions that convert pointers into slices to support -d=checkptr, that it reports cases that can work but may not.

The annotation indicates that a function is expected to issue an unaligned read (correct?)

No, checkptr only checks unsafe.Pointer conversions. It doesn't instrument every possible read/write using a pointer.

Thanks -- I updated my comment.

I think the general idea still stands, though. The code that triggers the check may not be under the control of the person who created the unaligned pointer.

The code that triggers the check may not be under the control of the person who created the unaligned pointer.

I'd argue the code that created the unaligned pointer is here:

sh.Data = uintptr(unsafe.Pointer(&b[offs]))

This is nominally just a uintptr-typed assignment, but it's actually overwriting a pointer in memory, so it's there's really an implied pointer conversion / assignment.

The issue here is that the compiler doesn't emit checks for this code pattern, because (in general) it doesn't know the type of slice pointed to by sh. We could perhaps implement some basic points-to analysis here to know that sh points to a slice of type []T, and then check the value written to sh.Data for T-alignment.

It's certainly unfortunate though that this false negative manifests as triggering a failure in very distant code though.

I would still like to more clearly understand the philosophy of -d=checkptr: should it only report cases that never work, or should it report cases that can work but likely will not?

I think deciding this is the essence of the issue here.

My rationale in implementing checkptr is based on wanting code to strictly conform to the specifications guaranteed to users (e.g., the Go language spec and unsafe.Pointer safety rules). We don't make any guarantees that pointers to unaligned integers are safe on any platforms, so I think it's appropriate to warn (by default) when that happens, even on platforms where it happens to be safe to do so. In that sense, I disagree with @bcmills's characterization of these as false positives.

Analogously, the race detector warns about all memory accesses that aren't synchronized according to the Go memory model. It doesn't rule out non-synchronized accesses that are impossible on certain CPUs/OSes.

For example, this program is race free, but the race detector still reports about it:

package main

import (
	"os"
)

func main() {
	r, w, err := os.Pipe()
	if err != nil {
		panic(err)
	}

	var x int

	go func() {
		x++
		w.Close()
	}()

	r.Read(make([]byte, 1))
	println(x)
}

But that said, I don't think other positions are inconsistent, and I'm not opposed to if we overall prefer a more lenient philosophy.

@mdempsky

My rationale behind -d=checkptr is based on interpreting unsafe.Alignof as required alignment.

I think that interpretation is significantly stronger than what the documentation for unsafe.Alignof actually says, which is (emphasis mine):

Alignof takes an expression x of any type and returns the required alignment of a hypothetical variable v as if v was declared via var v = x.

And the spec itself says:

The following minimal alignment properties are guaranteed:

1. For a variable x of any type: unsafe.Alignof(x) is at least 1.
2. For a variable x of struct type: unsafe.Alignof(x) is the largest of all the values unsafe.Alignof(x.f) for each field f of x, but at least 1.
3. For a variable x of array type: unsafe.Alignof(x) is the same as the alignment of a variable of the array's element type.

Note that both of those are expressed in terms of compiler-allocated variables, not backing stores of pointers. The documentation for unsafe.Pointer itself says nothing about alignment, except that “[i]t is also valid to use &^ to round pointers, usually for alignment.”

@cespare

3. Declare unaligned pointers to be bad unless annotated with go:nocheckptr; update code to use this annotation where necessary

(3) seems to be our de facto state in Go 1.14, but I don't think it's workable.

I agree: this state seems untenable to me, because in the limit it requires that nearly every function that uses unsafe.Pointer in even a trivial way must be annotated go:nocheckptr, and that completely undermines the value of the check.

Consider the function:

func Identity(b *uint64) *uint64 {
	p := unsafe.Pointer(b)
	return (*uint64)(p)
}

If a go:nocheckptr annotation is correct, then a caller could construct an unaligned pointer using go:nocheckptr in their own code, and pass that in to Identity, which would then fail the alignment check in its own (completely trivial) conversions and itself require a go:nocheckptr annotation.

We don't make any guarantees that pointers to unaligned integers are safe on any platforms, so I think it's appropriate to warn (by default) when that happens, even on platforms where it happens to be safe to do so.
…

Analogously, the race detector warns about all memory accesses that aren't synchronized according to the Go memory model. It doesn't rule out non-synchronized accesses that are impossible on certain CPUs/OSes.

Maybe? But these days we have a formal memory model for data races.

In contrast, reads through unaligned pointers are not clearly defined one way or the other, and my impression is that the Go project prefers to bias toward providing “intuitive” semantics for not-explicitly-defined programs rather than defining them to be erroneous (as in, say, C). Empirically, even parts of the Go project intuitively expect unaligned integers to work, for better or for worse.

I wouldn't object to a spec change to clearly define unaligned integer pointers as an error, but I would expect such a clarification to have its own release note and perhaps proposal, and to arrive in a major release (like 1.15) rather than a minor one (like 1.14.1 or 1.14.2).

In contrast, reads through unaligned pointers are not clearly defined one way or the other,

Ack, it could certainly be clearer. I'm arguing my position here, but I'm not dogmatic about it. I'm open to whatever the group consensus is. (And so I'm definitely hoping more folks will weigh in with their thoughts/interpretations.)

Note that both of those are expressed in terms of compiler-allocated variables, not backing stores of pointers.

Note that the Go spec uses "variable" to refer to any memory location used for storing a value. It's not used exclusively in reference to declared variables such as introduced by the "var" keyword. E.g., new(T) is defined as returning a pointer to a newly allocated variable; structs and arrays are defined as variables having sub-variables for each field/element.

Moreover, by definition, a pointer of type *T is either nil or points to a variable of type T. There's nothing else it can point to.

I think that interpretation is significantly stronger than what the documentation for unsafe.Alignof actually says, which is (emphasis mine):

Alignof takes an expression x of any type and returns the required alignment of a hypothetical variable v as if v was declared via var v = x.

Note that the Go spec defines alignment as a property of variables, whereas x is an arbitrary expression (i.e., possibly non-addressable or even untyped). In that context, I interpret this wording as explaining how to handle things like unsafe.Alignof(1), not that it only guarantees the alignment of declared variables.

I think we should declare unaligned pointers to be ok on at least x86. Maybe everywhere.

There are going to be too many people using unaligned pointers. I think we could break those people's programs if we needed to, but I just don't see a commensurate benefit in doing so.

The main benefit of the check is to catch people making alignment assumptions on x86, where their program would then fail on another architecture. That just doesn't seem that much of a pain point to me. Even if it fails on another architecture, at least it does so loudly, without silently corrupting data (I hope: it would do so on early Alpha chips, but I don't know any other arch that just ignores low bits of addresses when accessing memory).

I guess another benefit is just to catch people doing bad pointer arithmetic. That is a nice feature (and could potentially detect otherwise silent memory corruption bugs), but I don't think that's the intended purpose of this check.

We discussed this at the runtime meeting today and decided that we'd disable the alignment check if the base type of the pointer is pointer-free. So we'd still catch unaligned pointer accesses, but would allow other unaligned accesses to happen (on all architectures, even if that means they might fault).

Change https://golang.org/cl/223781 mentions this issue: runtime: don't report a pointer alignment error for pointer-free base type

We discussed this at the runtime meeting today and decided that we'd disable the alignment check if the base type of the pointer is pointer-free.

Was the decision to guarantee support for non-aligned integer accesses on x86, or simply not to proactively warn against it?

E.g., if we implement points-to optimizations in the compiler in the future, do we have to worry about partially overlapping variables?

@gopherbot please open a backport issue to 1.14.1

Backport issue(s) opened: #37905 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

We only discussed not warning about unaligned pointers to integer values. We didn't discuss whether they should be supported in any real sense.

There is a more general discussion to have about the interaction of unsafe pointers and points-to analysis, such as whether we should consider the possibility that a *int64 and a *float64 might point to the same memory even in a package that does not import unsafe.

I wanted to add that the key observation for me, at least, was that unaligned access will either work or crash on all architectures we support (at least, we're pretty sure; Cherry is confirming). That puts it in a different category from other things checkptr checks for.

ARMv5 does a rotated load depending on the lower two bits of the address. See https://heyrick.eu/armwiki/Unaligned_data_access.

FWIW, I would be fine with keeping the integer-alignment check on architectures where unaligned loads are not known to be safe, such as arm.

(And I'd be fine with requiring proper alignment everywhere, including amd64, in Go 1.15 or later with advance notice in the release notes.)

Thanks for pointing that out, @zeebo. After a bit of discussion about whether we should keep the check just for ARM, we decided that we don't really want to do an ARCH-specific checkptr check, especially since it only affects a small minority of deployments even on that architecture.

Change https://golang.org/cl/249378 mentions this issue: sha3: remove go:nocheckptr annotation