[PATCH resend2] 8157758: Use (~0u) instead of (-1) when left-shifting

[Kim Barrett]

> On Jun 13, 2016, at 11:51 AM, Alex Henrie <alexhenrie24 at gmail.com> wrote:
> 
> # HG changeset patch
> # User ahenrie
> # Date 1464130244 21600
> #      Tue May 24 16:50:44 2016 -0600
> # Node ID e3e2afa872c68a8fc7891fc25a9a11dab0704787
> # Parent  3f4173a750ac10fa4c22ed3d0e3d8d43bab3020b
> 8157758: Use (~0u) instead of (-1) when left-shifting
> 
> diff --git a/src/share/vm/code/dependencies.hpp b/src/share/vm/code/dependencies.hpp
> --- a/src/share/vm/code/dependencies.hpp
> +++ b/src/share/vm/code/dependencies.hpp
> @@ -163,17 +163,17 @@ class Dependencies: public ResourceObj {
>     call_site_target_value,
> 
>     TYPE_LIMIT
>   };
>   enum {
>     LG2_TYPE_LIMIT = 4,  // assert(TYPE_LIMIT <= (1<<LG2_TYPE_LIMIT))
> 
>     // handy categorizations of dependency types:
> -    all_types           = ((1 << TYPE_LIMIT) - 1) & ((-1) << FIRST_TYPE),
> +    all_types           = ((1 << TYPE_LIMIT) - 1) & ((~0u) << FIRST_TYPE),
> 
>     non_klass_types     = (1 << call_site_target_value),
>     klass_types         = all_types & ~non_klass_types,
> 
>     non_ctxk_types      = (1 << evol_method) | (1 << call_site_target_value),
>     implicit_ctxk_types = 0,
>     explicit_ctxk_types = all_types & ~(non_ctxk_types | implicit_ctxk_types),
> 
> diff --git a/src/share/vm/oops/cpCache.hpp b/src/share/vm/oops/cpCache.hpp
> --- a/src/share/vm/oops/cpCache.hpp
> +++ b/src/share/vm/oops/cpCache.hpp
> @@ -188,17 +188,17 @@ class ConstantPoolCacheEntry VALUE_OBJ_C
>     is_final_shift             = 22,  // (f) is the field or method final?
>     is_volatile_shift          = 21,  // (v) is the field volatile?
>     is_vfinal_shift            = 20,  // (vf) did the call resolve to a final method?
>     // low order bits give field index (for FieldInfo) or method parameter size:
>     field_index_bits           = 16,
>     field_index_mask           = right_n_bits(field_index_bits),
>     parameter_size_bits        = 8,  // subset of field_index_mask, range is 0..255
>     parameter_size_mask        = right_n_bits(parameter_size_bits),
> -    option_bits_mask           = ~(((-1) << tos_state_shift) | (field_index_mask | parameter_size_mask))
> +    option_bits_mask           = ~(((~0u) << tos_state_shift) | (field_index_mask | parameter_size_mask))
>   };
> 
>   // specific bit definitions for the indices field:
>   enum {
>     cp_index_bits              = 2*BitsPerByte,
>     cp_index_mask              = right_n_bits(cp_index_bits),
>     bytecode_1_shift           = cp_index_bits,
>     bytecode_1_mask            = right_n_bits(BitsPerByte), // == (u1)0xFF

At first glance, the proposed changes seem obviously correct, since
they replace left shifts of negative signed values, which is undefined
behavior, with left shifts of unsigned values, which are fine.

However, this patch may change the underlying type of the enum,
affecting the types of all the enumerators.  That could have
widespread ripple effects that are hard to track and examine.

Better would be to extract these problematic values out of their
respective enums and instead declare them as static consts of an
appropriate type.  That they are presently enumerators isn't actually
interesting.

Probably many / all of the nearby enumerators would be better treated
as static const declarations with appropriate types, but that's less
urgent than eliminating the obvious undefined behavior.