RFR(s): 8221408: Windows 32bit build build errors/warnings in hotspot

[Kim Barrett]

> On Mar 26, 2019, at 7:46 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Mar 26, 2019, at 2:18 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>> 
>> Kim Barret wrote:
>> 
>>> src/hotspot/share/runtime/vmStructs.hpp
>>> 275   { QUOTE(name), (uint64_t)name },
>>> 
>>> I'd like to get a better understanding of this change, as it doesn't
>>> make sense to me.  Can you provide the exact error message?  I can't
>>> see why there would be a problem here.  If I haven't messed up, the
>>> case you mentioned should be
>>> 
>>> 64bit
>>> hash_mask_in_place = (uintptr_t)((intptr_t(1) << 31) - 1) << 8
>>> 
>>> 32bit
>>> hash_mask_in_place = (uintptr_t)((intptr_t(1) << (31 - 7)) - 1) << 7
>>> 
>>> and I don't see anything problematic about either of those.
>>> 
>> 
>> Completely agree. I am at a loss here. 
>> 
>> Warning from older VS2017:
>> warning C4838: conversion from '' to 'uint64_t' requires a narrowing conversion
>> (Note the empty "from" string.)
>> 
>> David suggested updating the compiler. Same warning, but more elaborate text:
>> warning C4838: conversion from 'unnamed-enum-lock_mask' to 'uint64_t' requires a narrowing conversion
>> 
>> I tried to isolate the error, away from those massive macros.
>> 
>> This expression causes the error:
>> VMLongConstantEntry iii[] = {  { "hallo", markOopDesc::hash_mask_in_place }, {0,0}};
>> 
>> But not this:
>> VMLongConstantEntry iii = { "hallo", markOopDesc::hash_mask_in_place };
>> 
>> And neither this:
>> uint64_t iii = markOopDesc::hash_mask_in_place;
>> 
>> As I said, completely at a loss here. My fix works though.
> 
> The warning suggests an implicit narrowing in an initializer list,
> which is forbidden except under certain circumstances where the source
> is a constant expression whose value will fit in the target type; see
> C++14 8.5.4/7.
> 
> But I don't see why that warning is being triggered here.
> hash_mask_in_place is (for Win32) an enum value,

Actually, it’s an enum for everything *except* Win64.  I got confused by the
conditionalization, but I think the discussion below is still mostly okay.

> so clearly a constant
> expression, despite being of signed type (because that's the way VS
> rolls). It's especially confusing if this is the only one that is
> failing. This seems like a compiler bug.
> 
> I'm not in a position to test it, but below is a possible fix.
> 

Throughout below, s/_WIN32/_WIN64/

> I noticed that the value expressions for hash_mask and
> hash_mask_in_place are the same for !_WIN32 and _WIN64, with only the
> declarations being different (enum values vs static consts with
> appropriate type). The _WIN64 declarations would actually work fine
> for !_WIN64 too. So I'm removing the !_WIN32 declarations and
> unconditionally using the _WIN64 declarations. (I didn't fix the
> indentation of the retained declarations to minimize the changes in
> the patch.) It seems unlikely that any code is dependent on these
> being of int type rather than uintptr_t on Win32.

I think this will still work, despite my confusion over the conditionalization.
But it affects all non-Win64 platforms, and I haven’t tested it at all.
I’ll run some tests on the platforms we (Oracle) support tomorrow.

> ----- begin patch -----
> diff -r fc8f0a122310 src/hotspot/share/oops/markOop.hpp
> --- a/src/hotspot/share/oops/markOop.hpp	Tue Mar 26 19:26:30 2019 -0400
> +++ b/src/hotspot/share/oops/markOop.hpp	Tue Mar 26 19:27:59 2019 -0400
> @@ -138,22 +138,16 @@
>          epoch_mask_in_place      = epoch_mask << epoch_shift,
>          cms_mask                 = right_n_bits(cms_bits),
>          cms_mask_in_place        = cms_mask << cms_shift
> -#ifndef _WIN64
> -         ,hash_mask               = right_n_bits(hash_bits),
> -         hash_mask_in_place       = (address_word)hash_mask << hash_shift
> -#endif
>   };
> 
>   // Alignment of JavaThread pointers encoded in object header required by biased locking
>   enum { biased_lock_alignment    = 2 << (epoch_shift + epoch_bits)
>   };
> 
> -#ifdef _WIN64
>     // These values are too big for Win64
>     const static uintptr_t hash_mask = right_n_bits(hash_bits);
>     const static uintptr_t hash_mask_in_place  =
>                             (address_word)hash_mask << hash_shift;
> -#endif
> 
>   enum { locked_value             = 0,
>          unlocked_value           = 1,
> ----- end patch -----