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

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.

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

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.

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

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.

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

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.

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.

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.

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

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