x/tools/go/analysis/passes/shift: allow full-width integer shifts

[rsc]

The shift pass rejects a uint64 shifted by a constant 64, and on 32 bit platforms a uint shifted by 32, both of which come up in correct code. Here are two examples:

From a recent x/sys/unix CL:

func offs2lohi(offs int64) (lo, hi uintptr) {
	const longBits = SizeofLong * 8
	return uintptr(offs), uintptr(uint64(offs) >> longBits)
}

On a 64-bit system, the goal is to return offs, 0, and that expression does that, using a uint64 >> 64 in the second expression. I am going to send a CL out replacing >> longBits with >> (longBits - 1) >> 1, to make older vets happy with this code.

From a recent math/big CL:

// bits.Len uses a lookup table for the low-order bits on some
// architectures. Neutralize any input-dependent behavior by setting all
// bits after the first one bit.
top := uint(x[i])
top |= top >> 1
top |= top >> 2
top |= top >> 4
top |= top >> 8
top |= top >> 16
top |= top >> 16 >> 16 // ">> 32" doesn't compile on 32-bit architectures
return i*_W + bits.Len(top)

Note the comment says "doesn't compile" but really the problem is that it doesn't vet.

Vet should not be redefining what is permitted in the language for valid code. Because these programs are valid, we should loosen the check in passes/shift/shift.go to be amt > maxSize, not amt >= maxSize.

[gopherbot]

Change https://go.dev/cl/463675 mentions this issue: unix: avoid false positive in vet shift check

[rsc]

Another possible loosening would be to (1) treat uint and uintptr as 64 bits on all platforms, and (2) do not warn about shifts by computed constant values. So given

var u64 uint64
var up uintptr
var u uint
var u32 uint32

u32>>32 and u64>>64 would be flagged, but u>>32 and up>>32 would not, even in a 32-bit build, and u64>>(computed expression == 64) would not either.

[timothy-king]

I think vet would still be helpful to warn on this variant:

func alt(offs int64) (lo, hi uintptr) {
	return uintptr(offs), uintptr(uint64(offs) >> 64)
}

Ambiguity about the machine word size seems to be involved in all of these problems. How about loosening to ">" when there is a possible size ambiguity like a possible arch size difference or a computed value for y? For "x << y" this would be loosened when either x or y are types int, uint or uintptr, or y is not a BasicLit.

Maybe vet could try to be slightly smarter and try to determine if the value of y relies on an identifier (either a type name or a constant value seems worth stopping on)? That might just be too clever for little benefit compared to *BasicLit. It might be unfortunate for x << (y+1-1) to be considered ambiguous. (but probably not a big deal.)

[ianlancetaylor]

This is an old vet warning (dates back to https://golang.org/cl/134780043) but it's the kind of warning that I'm never very happy about. In portable code I can have types and constants that are defined using build tags to be different on different systems. It's annoying when the tools block me from writing straightforward code that just happens to be odd on specific platforms. (I have similar concerns about the language rules that prohibit constant division by zero.)

So as I think you are saying, it's probably OK to warn about mixing a predeclared type and a literal constant that is out of range. But we should not warn when not using a predeclared type, and we should also not warn when not using a literal constant.

[rsc]

Thinking more about this, here is an example of a common mistake we do want to catch:

func make64(lo, hi uint32) uint64 {
    return uint64(lo) | uint64(hi<<32)
}

(The correct code is uint64(hi)<<32.)

This version is buggy too, but I am not sure we can catch it while also not catching the math/big CL:

func make64(lo, hi uint) uint64 {
    return uint64(lo) | uint64(hi<<32)
}

That's probably OK to miss, since most code along these lines is using sized ints.

Here is a potential rule:

1. The check only applies to numeric constant expressions. (7+1) yes, (N+1) no.
2. Treat int, uint, and uintptr as 64 bits on all systems for the purposes of the check.

(There is already a rule that exempts shifts with a constant on the left, like ^uint(0) >> 63. That would stay.)

Thoughts?

[randall77]

I feel like this is only tinkering around the edges of the problem, that code that looks baffling for one GOARCH is actually reasonable for another GOARCH.

The "right" solution is to run the check for all possible GOARCHes and report an error only if the check triggers in all of them. Sounds kinda crazy, but is there any reason we couldn't just do that?

[rsc]

@randall77 for the specific case of the shift check, running on all GOARCHes amounts to just triggering if the check would happen on both 32- and 64-bit systems, and since the check is only a >= or > comparison, assuming 64 bits has exactly that effect.

In general we need compiled dependencies to actually change the GOARCH, which in many cases would require changing the GOOS too. There's no darwin/ppc64le for example. The API check does run for all combinations of GOOS/GOARCH, and it's doable but definitely a factor of 10 or more.

[timothy-king]

The check only applies to numeric constant expressions. (7+1) yes, (N+1) no.
Treat int, uint, and uintptr as 64 bits on all systems for the purposes of the check.

We previously supported any constant expression like unsafe.Sizeof(i) as a side-effect go go/types. We may need to build a small ast constant evaluator to do the treat "as 64 bits" step. My guess is we probably do not want to lose support for unsafe.Sizeof on integers, and we are probably okay loosing support for Alignof, Offsetof, len and cap constant expression.

Thoughts?

The "right" solution is to run the check for all possible GOARCHes and report an error only if the check triggers in all of them. Sounds kinda crazy, but is there any reason we couldn't just do that?

This actually does sound like a principled answer. One complication is that vet assumes one consistently type checked package and other definitions might be in different files. We are currently getting the constant evaluation as a side-effect from go/types. That works on one set of files at a time. But I think we can try to simulate "all GOARCHes" for a big enough set of expressions.

[randall77]

@randall77 for the specific case of the shift check, running on all GOARCHes amounts to just triggering if the check would happen on both 32- and 64-bit systems, and since the check is only a >= or > comparison, assuming 64 bits has exactly that effect.

Sure, as long as the shift amount is an integer literal. We're giving up on cases where the shift amount is a buildtag-gated constant (that turns out to be wrong somehow).