Hi David, Updating vs2017 did not help :/
Cheers, Thomas On Mon, Mar 25, 2019 at 8:17 AM David Holmes <david.hol...@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.hol...@oracle.com > > <mailto:david.hol...@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 > > > > > >
