RFR(s): 8221408: Windows 32bit build build errors/warnings in hotspot

[Thomas Stüfe]

Hi Kim,

thanks for looking a this. Pls see my remarks inline.

On Wed, Mar 27, 2019 at 5:59 AM Kim Barrett <kim.barrett at oracle.com> wrote:

> > On Mar 26, 2019, at 7:46 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> >
> >> On Mar 26, 2019, at 2:18 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> >>
> >> Kim Barret wrote:
> >>
> >>> src/hotspot/share/runtime/vmStructs.hpp
> >>> 275   { QUOTE(name), (uint64_t)name },
> >>>
> >>> I'd like to get a better understanding of this change, as it doesn't
> >>> make sense to me.  Can you provide the exact error message?  I can't
> >>> see why there would be a problem here.  If I haven't messed up, the
> >>> case you mentioned should be
> >>>
> >>> 64bit
> >>> hash_mask_in_place = (uintptr_t)((intptr_t(1) << 31) - 1) << 8
> >>>
> >>> 32bit
> >>> hash_mask_in_place = (uintptr_t)((intptr_t(1) << (31 - 7)) - 1) << 7
> >>>
> >>> and I don't see anything problematic about either of those.
> >>>
> >>
> >> Completely agree. I am at a loss here.
> >>
> >> Warning from older VS2017:
> >> warning C4838: conversion from '' to 'uint64_t' requires a narrowing
> conversion
> >> (Note the empty "from" string.)
> >>
> >> David suggested updating the compiler. Same warning, but more elaborate
> text:
> >> warning C4838: conversion from 'unnamed-enum-lock_mask' to 'uint64_t'
> requires a narrowing conversion
> >>
> >> I tried to isolate the error, away from those massive macros.
> >>
> >> This expression causes the error:
> >> VMLongConstantEntry iii[] = {  { "hallo",
> markOopDesc::hash_mask_in_place }, {0,0}};
> >>
> >> But not this:
> >> VMLongConstantEntry iii = { "hallo", markOopDesc::hash_mask_in_place };
> >>
> >> And neither this:
> >> uint64_t iii = markOopDesc::hash_mask_in_place;
> >>
> >> As I said, completely at a loss here. My fix works though.
> >
> > The warning suggests an implicit narrowing in an initializer list,
> > which is forbidden except under certain circumstances where the source
> > is a constant expression whose value will fit in the target type; see
> > C++14 8.5.4/7.
> >
> > But I don't see why that warning is being triggered here.
> > hash_mask_in_place is (for Win32) an enum value,
>
> Actually, it’s an enum for everything *except* Win64.  I got confused by
> the
> conditionalization, but I think the discussion below is still mostly okay.
>
>
Yes, that is why I shied away from such a change and rather fixed the
assignment itself.

That and the feeling that the code was perfectly fine as it is; changing it
by replacing the enum value with an uintptr_t felt like cargo-cult
programming. To satisfy one broken compiler.

The more I think about this the more I think I would rather manually switch
the warning off on WIN32 with a pragma. Difficult to do this though for
just one value in the macro list. However, that would be the clearest
expression of "we have a broken compiler on one platform but the code is
actually fine".

Note that one alternative to your suggestion would be to make the static
refrain from using the uintptr_t windows only (both 32 and 64bit).


> > so clearly a constant
> > expression, despite being of signed type (because that's the way VS
> > rolls). It's especially confusing if this is the only one that is
> > failing. This seems like a compiler bug.
> >
> > I'm not in a position to test it, but below is a possible fix.
> >
>
> Throughout below, s/_WIN32/_WIN64/
>
> > I noticed that the value expressions for hash_mask and
> > hash_mask_in_place are the same for !_WIN32 and _WIN64, with only the
> > declarations being different (enum values vs static consts with
> > appropriate type). The _WIN64 declarations would actually work fine
> > for !_WIN64 too. So I'm removing the !_WIN32 declarations and
> > unconditionally using the _WIN64 declarations. (I didn't fix the
> > indentation of the retained declarations to minimize the changes in
> > the patch.) It seems unlikely that any code is dependent on these
> > being of int type rather than uintptr_t on Win32.
>
> I think this will still work, despite my confusion over the
> conditionalization.
> But it affects all non-Win64 platforms, and I haven’t tested it at all.
> I’ll run some tests on the platforms we (Oracle) support tomorrow.
>
>
Thank you,

Thomas