RFR: 8332368: ubsan aarch64: immediate_aarch64.cpp:298:31: runtime error: shift exponent 32 is too large for 32-bit type 'int'

[Andrew Dinn]

On Wed, 16 Apr 2025 18:32:16 GMT, Boris Ulasevich <bulasevich at openjdk.org> wrote:

> Running a linux-aarch64-server-fastdebug build with UBSAN (--enable-ubsan configure option) gives the following runtime error immediately on JVM start:
> 
> 
> ad_aarch64.hpp:7114:11: runtime error: shift exponent 100 is too large for 32-bit type 'unsigned int'
> # Pipeline_Use_Cycle_Mask::operator<<=(int) make/hotspot/ad_aarch64.hpp:7114
> # Pipeline_Use_Element::step(unsigned int) make/hotspot/ad_aarch64.hpp:7168
> # Pipeline_Use::step(unsigned int) make/hotspot/ad_aarch64.hpp:7216
> # Scheduling::step(unsigned int) src/hotspot/share/opto/output.cpp:2116
> # Scheduling::AddNodeToBundle(Node*, Block const*) src/hotspot/share/opto/output.cpp:2553
> 
> 
> The value of 100 comes from fixed_latency, defined in AD files:
> 
> // Pipeline class for call.
> pipe_class pipe_class_call()
> %{
>   single_instruction;
>   fixed_latency(100);
> %}
> 
> 
> The fixed_latency value is used by the scheduler to model the occupancy of functional units over time. The occupancy is tracked using a uint mask value:
> 
>   fprintf(fp_hpp, "class Pipeline_Use_Element {\n");
>   fprintf(fp_hpp, "protected:\n");
>   fprintf(fp_hpp, "  // Mask of used functional units\n");
>   fprintf(fp_hpp, "  uint _used;\n\n");
> 
> When the scheduler virtually steps over the instruction, it shifts the masks left by the instruction's latency. The problem is that 100 is greater than sizeof(uint), and left-shifting by 100 effectively zeroes the _mask, but, according to the C++ standard, this is undefined behavior.
> 
> We can find a number of fixed_latency(100) expressions in aarch64.ad, arm.ad, ppc.ad, riscv.ad, x86_64.ad files. Perhaps all of them deserve correction. I suggest leaving the AD files as they are, but limiting the shift value in case it exceeds the allowed maximum in a generated code:
> 
>  void step(uint cycles) {
>    _used = 0;
> -  _mask <<= cycles;
> +  uint max_shift = 8 * sizeof(_mask) - 1;
> +  _mask <<= (cycles < max_shift) ? cycles : max_shift;
>  }
> 
> In fact, this change does not affect the current behavior; we just eliminate the undefined behavior while preserving the intended semantics.

I believe this is the right solution.

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

Marked as reviewed by adinn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24696#pullrequestreview-2775910433