RFR: 8331201: UBSAN enabled build reports on Linux x86_64 runtime error: shift exponent 65 is too large for 64-bit type 'long unsigned int' [v2]

[Axel Boldt-Christmas]

On Thu, 19 Dec 2024 08:46:57 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> The issue existed in making Fingerprints of method names. Each parameter in the methods' arguments is decoded as a 4-bits value. The 64-bits `fingertprint_t` can hold up to 14 parameters plus return type and static bit. To make the Fingerprint, the signature is iterated one parameter at a time and the corresponding code is accumulated after shifting the bits up.
>> Some compilers do not mask the shift value to the base size and UBSAN catches the case. 
>> In this PR, the number of parameters (`_param_count`) is used and compared with the max (14) to do the shift operation safely. The pre-existing `_param_size` is not reflecting the number of parameters, since it is incremented by 2 for `T_DOUBLE` and `T_LONG` types.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   removed extra blank lines

I think the best way to fix this specific issue is the way I proposed in the JBS entry. 

-   _accumulator |= ((fingerprint_t)type << _shift_count);
-   _shift_count += fp_parameter_feature_size;
+   if (_param_size < fp_max_size_of_parameters) {
+     _accumulator |= ((fingerprint_t)type << _shift_count);
+.    _shift_count += fp_parameter_feature_size;
+   }


My reasoning for using the `_param_size < fp_max_size_of_parameters` condition instead of some `_shift_count < 64` is that the later makes it more confusing IMO. The current implementation of fingerprinting uses the `_param_size < fp_max_size_of_parameters`  condition to determine if an overflow fingerprint should be used. (The only way we would currently install a accumulated fingerprint that when this condition is false is if there is a mismatch between `_method->size_of_parameters()` and `_param_size`. But I assume that `!_method || _method->size_of_parameters() == _param_size` is a post condition here).

Generally UB in the implementation is because we have not through through the design enough. I feel it is rarely the best solution to create a `if (!ub) { do_the_thing(); }` rather than look at the algorithm and find what are we actually trying to do and then adapt the implementation in terms of the algorithm. Which in this case is if  `_param_size < fp_max_size_of_parameters` we use an overflow fingerprint so stop accumulating a value which will never be read.


I then think we should create a RFE for investigating if there are any `_param_size < fp_max_size_of_parameters` implies overflow fingerprint dependencies or assumptions anywhere else. And with that information in mind, change the algorithm such that the condition for using a overflow fingerprint is not tied to the size of the parameters, but the number of parameters. So we can fingerprint any method with 14 or less parameters, regardless of their types.

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

PR Comment: https://git.openjdk.org/jdk/pull/22807#issuecomment-2579267432