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

[Kim Barrett]

> 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, 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.

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.

----- 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 -----