RFR(xs): 8221375: Windows 32bit build (VS2017) broken

[Kim Barrett]

> On Mar 24, 2019, at 4:58 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 
> Hi all,
> 
> After a long time I tried to build a Windows 32bit VM (VS2017) and failed;
> multiple errors and warnings. Lets reverse the bitrot:
> 
> cr:
> http://cr.openjdk.java.net/~stuefe/webrevs/8221375--windows-32bit-build-(vs2017)-broken-in-many-places/webrev.00/webrev/
> 
> Issue: https://bugs.openjdk.java.net/browse/JDK-8221375
> 
> Most of the fixes are trivial: Calling convention mismatches (awt DTRACE
> callbacks), printf specifiers etc.
> 
> Had to supress a warning in os_windows_x86.cpp - I was surprised by this
> since this did not look 32bit specifc, do we disable warnings on Windows
> 64bit builds?
> 
> The error I had in vmStructs.cpp was a bit weird: compiler complained about
> an assignment of an enum value defined like this:
> 
> hash_mask_in_place       = (address_word)hash_mask << hash_shift
> 
> to an uint64_t variable, complaining about narrowing. I did not find out
> what his problem was. In the end, I decided to add an explicit cast to
> GENERATE_VM_LONG_CONSTANT_ENTRY(name) (see vmStructs.hpp).
> 
> With this patch we can build with warnings enabled on 32bit and 64bit
> windows.
> 
> Since patch fixes both hotspot and JDK parts, I'm sending it to hs-dev and
> core-libs-dev.
> 
> Thanks, Thomas

I started looking at the HotSpot parts before you retracted with the intent of opening
separate issues.  I’ll give you my comments now, in case I miss some of those new
issues and might be able to affect the associated changes.

------------------------------------------------------------------------------ 
src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp
 468 #pragma warning(push)
 469 // Ignore "C4172: returning address of local variable or temporary"
 470 #pragma warning(disable : 4172)

Although this is Windows-specific code, consider using
PRAGMA_DIAG_PUSH/POP and PRAGMA_DISABLE_MSVC_WARNING.

------------------------------------------------------------------------------ 
src/hotspot/share/classfile/classFileParser.cpp
 315           utf8_length = (u2) strlen(str);

This looked really familiar; I remember DavidH and I looking at this
before and being puzzled because the warning only showed up in 32bit
builds, even though the exact same issue applies to 64bit builds.

Unfortunately, the person who reported it (in an internal channel)
doesn't seem to have followed up with a patch or even a bug report.
So thanks for this.

------------------------------------------------------------------------------
src/hotspot/share/oops/markOop.hpp
  29 #include "utilities/globalDefinitions.hpp"

I don't object to this change, but I'm surprised this is needed in
order to build.  oops/oop.hpp indirectly includes globalDefinitions.hpp.

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

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