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

[Kim Barrett]

> On Jun 14, 2016, at 6:14 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> 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.

After examining the specific values involved, I don't think a change
of the underlying type for the enums in question is likely after all.

And my suggestion of moving the affected values out of the enum has a
similar opportunity to change the underlying type of the enum.

I still think it would be better in the long term to eliminate the
enums in favor of constants with specified types (or make the
underlying type explicit using C++11 syntax, but we don't have that
option yet), but that isn't needed right now.

So I've changed my mind about this patch.  Looks good.