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

[Thomas Stüfe]

Hi Kim, David,

new version:

http://cr.openjdk.java.net/~stuefe/webrevs/8221408-win32-hotspot-buildfixes/webrev.01/webrev/

Worked in feedback:

- using PRAGMA_ macros instead of raw pragmas on os::current_stack_pointer()
- redefined the offending enum values in vmStruct.hpp to be static
constants, for all platforms.

Good?

Did build successfully on 32bit windows. Submit tests are running but I do
not expect any surprises.

Thanks, Thomas




On Wed, Mar 27, 2019 at 9:54 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > On Mar 27, 2019, at 1: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:
> > > 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.
>
> I suspect we wouldn't even be discussing this if we were using static
> consts
> with known types rather than enum values with implicit platform-dependent
> types. And the driver for much of our enum usage is ancient broken
> compilers
> that haven't been usable by us for *many* years (JDK-8153116).
>
> Anyway, a cleaned up version of my patch passed tier1 on Oracle-supported
> platforms.
>
> > 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”.
>
> My preference is to make the change to static consts.  Indeed, as a
> cleanup I’d get replace
> that enum (and probably those nearby) with static consts with explicit
> types, rather than the
> platform-dependent implicit enum types.
>
> Before going any further though, I’d want to know whether my patch
> actually does fix the
> win32 build failure, or if there is something else lurking there.
>
>