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
>
