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

[Thomas Stüfe]

Hi all,

Following David's suggestion, I withdraw this bug and will open issues for
each area separately.

Cheers, Thomas



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

> 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/c6690e25/attachment-0001.html>