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

[David Holmes]

Hi Thomas,

On 25/03/2019 5:01 pm, Thomas Stüfe wrote:
> Hi David,
> 
> (added net-dev, awt-dev, security-dev since part of these fixes are in 
> their territory)

May be better to split up the reviews, cross-posting that many groups 
gets very messy given most people won't be subscribed to them all - 
myself included.

> On Mon, Mar 25, 2019 at 1:34 AM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     Hi Thomas,
> 
>     A few queries, comments and concerns ...
> 
>     On 25/03/2019 6:58 am, Thomas Stüfe wrote:
>      > Hi all,
>      >
>      > After a long time I tried to build a Windows 32bit VM (VS2017)
>     and failed;
> 
>     I'm somewhat surprised as I thought someone was actively doing Windows
>     32-bit builds out there, plus there are shared code changes that should
>     also have been caught by non-Windows 32-bit builds. :(
> 
> 
> Not sure what others do. I did a 32bit windows build, slowdebug, warning 
> enabled, and it failed with those 5+ issues.
> 
>      > 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?
> 
>     What version of VS2017? We use VS2017 15.9.6 and we don't disable
>     warnings.
> 
> 
> I use VS2017 CE. Not sure which version spcifically, but my compiler is at
> 
> Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26431 for x86

I think that would equate to an older version - 15.7

MSVC++ 14.14 _MSC_VER == 1914 (Visual Studio 2017 version 15.7)

Any chance you can upgrade to latest version? (Especially in light of 
the apparent compiler bug you cite below.)

Thanks,
David
-----

>      > 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).
> 
>     Not at all sure that's the right fix. In markOop.hpp we see that value
>     gets special treatment on Windows-x64:
> 
> 
>     #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
> 
>     perhaps something special is needed for Windows-x86. I'm unclear how
>     the
>     values can be "too big" ??
> 
> 
> I banged my head against this for an hour or so and I think this is a 
> compiler bug.
> 
> What I get is:
> 
> warning C4838: conversion from '' to 'uint64_t' requires a narrowing 
> conversion
> 
> (Note the empty "from" string.)
> 
> Here are my tries to provoke the error:
> 
> VMLongConstantEntry iii[] = {  { "hallo", 
> markOopDesc::hash_mask_in_place }, {0,0}};  <<< this fails
> VMLongConstantEntry iii = { "hallo", markOopDesc::hash_mask_in_place };  
>   << but this succeeds
> uint64_t iii = markOopDesc::hash_mask_in_place;   << this succeeds  too
> 
> I have no clue what this means. It is difficult to fix since the 
> expression is hidden in such a macro pile. But I think either we go with 
> my change or we disable the warning for win32 for the whole section.
> 
>      >
>      > 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.
> 
>     Don't see anything that actually comes under core-libs-dev. Looks like
>     one net-dev, one awt-dev and one security-dev. Sorry.
> 
> 
> Okay, I will add them.
> 
>     Cheers,
>     David
>     -----
> 
> 
> Thanks for reviewing,
> 
> Thomas
> 
>      > Thanks, Thomas
>      >
>