<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi David,</div><div dir="ltr"><br></div><div>(added net-dev, awt-dev, security-dev since part of these fixes are in their territory)</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 25, 2019 at 1:34 AM David Holmes <<a href="mailto:david.holmes@oracle.com">david.holmes@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Thomas,<br>
<br>
A few queries, comments and concerns ...<br>
<br>
On 25/03/2019 6:58 am, Thomas Stüfe wrote:<br>
> Hi all,<br>
> <br>
> After a long time I tried to build a Windows 32bit VM (VS2017) and failed;<br>
<br>
I'm somewhat surprised as I thought someone was actively doing Windows <br>
32-bit builds out there, plus there are shared code changes that should <br>
also have been caught by non-Windows 32-bit builds. :(<br>
<br></blockquote><div><br class="gmail-Apple-interchange-newline">Not sure what others do. I did a 32bit windows build, slowdebug, warning enabled, and it failed with those 5+ issues. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> multiple errors and warnings. Lets reverse the bitrot:<br>
> <br>
> cr:<br>
> <a href="http://cr.openjdk.java.net/~stuefe/webrevs/8221375--windows-32bit-build-(vs2017)-broken-in-many-places/webrev.00/webrev/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~stuefe/webrevs/8221375--windows-32bit-build-(vs2017)-broken-in-many-places/webrev.00/webrev/</a><br>
> <br>
> Issue: <a href="https://bugs.openjdk.java.net/browse/JDK-8221375" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8221375</a><br>
> <br>
> Most of the fixes are trivial: Calling convention mismatches (awt DTRACE<br>
> callbacks), printf specifiers etc.<br>
> <br>
> Had to supress a warning in os_windows_x86.cpp - I was surprised by this<br>
> since this did not look 32bit specifc, do we disable warnings on Windows<br>
> 64bit builds?<br>
<br>
What version of VS2017? We use VS2017 15.9.6 and we don't disable warnings.<br>
<br></blockquote><div><br></div><div>I use VS2017 CE. Not sure which version spcifically, but my compiler is at</div><div><br></div><div><div>Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26431 for x86<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> The error I had in vmStructs.cpp was a bit weird: compiler complained about<br>
> an assignment of an enum value defined like this:<br>
> <br>
> hash_mask_in_place = (address_word)hash_mask << hash_shift<br>
> <br>
> to an uint64_t variable, complaining about narrowing. I did not find out<br>
> what his problem was. In the end, I decided to add an explicit cast to<br>
> GENERATE_VM_LONG_CONSTANT_ENTRY(name) (see vmStructs.hpp).<br>
<br>
Not at all sure that's the right fix. In markOop.hpp we see that value <br>
gets special treatment on Windows-x64:<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
#ifndef _WIN64<br>
,hash_mask = right_n_bits(hash_bits),<br>
hash_mask_in_place = (address_word)hash_mask << hash_shift<br>
#endif<br>
};<br>
<br>
// Alignment of JavaThread pointers encoded in object header required <br>
by biased locking<br>
enum { biased_lock_alignment = 2 << (epoch_shift + epoch_bits)<br>
};<br>
<br>
#ifdef _WIN64<br>
// These values are too big for Win64<br>
const static uintptr_t hash_mask = right_n_bits(hash_bits);<br>
const static uintptr_t hash_mask_in_place =<br>
(address_word)hash_mask << hash_shift;<br>
#endif<br>
<br>
perhaps something special is needed for Windows-x86. I'm unclear how the <br>
values can be "too big" ??<br>
<br></blockquote><div><br></div><div>I banged my head against this for an hour or so and I think this is a compiler bug.</div><div><br></div><div>What I get is:</div><div><br></div><div>warning C4838: conversion from '' to 'uint64_t' requires a narrowing conversion<br></div><div><br></div><div>(Note the empty "from" string.)</div><div><br></div><div>Here are my tries to provoke the error:</div><div><br></div><div><div>VMLongConstantEntry iii[] = { { "hallo", markOopDesc::hash_mask_in_place }, {0,0}}; <<< this fails<br></div><div>VMLongConstantEntry iii = { "hallo", markOopDesc::hash_mask_in_place }; << but this succeeds<br></div><div>uint64_t iii = markOopDesc::hash_mask_in_place; << this succeeds too<br></div></div><div><br></div><div>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.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> With this patch we can build with warnings enabled on 32bit and 64bit<br>
> windows.<br>
> <br>
> Since patch fixes both hotspot and JDK parts, I'm sending it to hs-dev and<br>
> core-libs-dev.<br>
<br>
Don't see anything that actually comes under core-libs-dev. Looks like <br>
one net-dev, one awt-dev and one security-dev. Sorry.<br>
<br></blockquote><div><br></div><div>Okay, I will add them.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Cheers,<br>
David<br>
-----<br>
<br></blockquote><div><br></div><div>Thanks for reviewing,</div><div><br></div><div>Thomas</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Thanks, Thomas<br>
> <br>
</blockquote></div></div></div></div></div>