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

[Thomas Stüfe]

Hi David,

Updating vs2017 did not help :/

Cheers, Thomas

On Mon, Mar 25, 2019 at 8:17 AM David Holmes <david.holmes at oracle.com>
wrote:

> 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
> >      >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20190325/b23f0f6d/attachment.html>