RFR: 5038439: Warning message for literal shift amounts outside the canonical domain

[Jan Lahoda]

On Thu, 4 Sep 2025 15:22:57 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

> When bit shifting an `int` or `long` value by an amount `X`, all but the last 5 or 6 (respectively) bits of `X` are ignored.
> 
> This can create a trap for the unwary, as in this example:
> 
> public long readLongBigEndian(byte[] buf, int offset) {
>     return ((buf[offset + 0] & 0xff) << 56)   // BUG HERE
>          | ((buf[offset + 1] & 0xff) << 48)   // BUG HERE
>          | ((buf[offset + 2] & 0xff) << 40)   // BUG HERE
>          | ((buf[offset + 3] & 0xff) << 32)   // BUG HERE
>          | ((buf[offset + 4] & 0xff) << 24)
>          | ((buf[offset + 5] & 0xff) << 16)
>          | ((buf[offset + 6] & 0xff) << 8)
>          | ((buf[offset + 7] & 0xff);
> }
> 
> This PR adds a new warning when the compiler detects an out-of-range bit shift, i.e., an `int` bit shift not in the range `[0...31]` or a `long` bit shift not in the range `[0...63]`.

I think overall seems sensible - some questions/nits inline.

make/jdk/src/classes/build/tools/intpoly/FieldGen.java line 631:

> 629:                 + (params.getBitsPerLimb() - 1) + ";");
> 630:         if (params.getBitsPerLimb() * params.getNumLimbs() != params.getPower()) {
> 631:             result.appendLine("@SuppressWarnings(\"lossy-conversions\")");

Do you know what is the specific problematic code generated here?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 4055:

> 4053:             int maximumShift;
> 4054:             switch (((OperatorSymbol)operator).opcode) {
> 4055:             case ByteCodes.ishl:

Nit: use "modernized" switch - multiple case labels and `->`?

test/langtools/tools/javac/lint/ShiftOutOfRange.java line 14:

> 12: 
> 13:         // These should generate warnings
> 14:         a = a << (byte)-1;

Looking at the patch, I am not completely convinced disallowing `-1` is beneficial. On one hand, it looks weird. On the other hand, it makes some sense (as far as I can tell, it is universal for both `int` and `long`). And, unlike `128`, it is not clearly wrong. I am not sure if it wouldn't be better to simply permit `-1` as a special case.

I see there are `<< -5` in the microbenchmark tests, but I think I am less inclined to accommodate negative values other than `-1`, as the semantics of those is much more cryptic, for me at least.

Please let me know what you think.

-------------

PR Review: https://git.openjdk.org/jdk/pull/27102#pullrequestreview-3411950069
PR Review Comment: https://git.openjdk.org/jdk/pull/27102#discussion_r2487162366
PR Review Comment: https://git.openjdk.org/jdk/pull/27102#discussion_r2487161036
PR Review Comment: https://git.openjdk.org/jdk/pull/27102#discussion_r2487176078