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

[Thomas Stüfe]

On Wed, Mar 27, 2019 at 6:41 AM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> 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).
>

That sentence is not parsable :)

What I meant was to make your change only for Windows, both variants.

..Thomas



>
>> > 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
>
>