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

[Thomas Stüfe]

On Mon, Mar 25, 2019 at 2:17 PM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Hi all,
>
> may I please have reviews for the following build fixes for Windows 32bit:
>
> Issue:https://bugs.openjdk.java.net/browse/JDK-8221408
> cr:
> http://cr.openjdk.java.net/~stuefe/webrevs/8221408-win32-hotspot-buildfixes/webrev.00/webrev/
>
> Thank you,
>
> Thomas
>

Kim Barret wrote:

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

Okay, I'll change this.

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

This was when I checked the weird vmStructs.cpp error. I wanted to make
sure that address_word is defined in vmStructs.hpp. But it is not necessary
and I will remove it.

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

>------------------------------------------------------------------------------

Note that I will have no opportunity to play around with Win32 compilers
for the rest of the week - fixing win32 was a weekend project and I'm back
in the office for serious work :)

..Thomas