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.stu...@gmail.com> wrote: > 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 >> > > >> > >> >
