RFR: 8074895: os::getenv is inadequate

David Holmes david.holmes at oracle.com
Mon Mar 30 01:39:32 UTC 2015


On 27/03/2015 5:24 PM, Jeremy Manson wrote:
> I hate to see legacy cruft deliberately introduced into the codebase.  I
> guess it is too painful to turn it off in a makefile?  Stuff ignored by
> compilers in rarely touched code like this tends to turn crufty and
> become confusing, e.g., something I saw a month or two ago:
>
> http://hg.openjdk.java.net/jdk9/dev/hotspot/file/f68d656d1f5e/src/share/vm/oops/instanceKlass.cpp#l784
>
> Referring you to a page in what you have to think about for a second
> before you realize is JVMS v1, which has been obsolete since 2000, and
> is unavailable from the publisher.

But happens to be the version you would find sitting on the bookshelves 
of the Oracle VM team members :) A section reference would be better 
than a page number, but even they change over time.

> Doing it this way seems fine to me, but I don't know anything about
> suppressing warnings on Windows, so that's not a firm endorsement.  Not
> that my non-reviewer endorsement would do you any good.

Okay. Still need a second review - calling Coleen!

I'd really like to get this out of my repo and pushed :)

Thanks,
David

> Jeremy
>
> On Wed, Mar 25, 2015 at 11:41 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Okay I managed to fix this with:
>
>     --- old/src/share/vm/utilities/__growableArray.hpp        2015-03-26
>     02:34:35.715892476 -0400
>     +++ new/src/share/vm/utilities/__growableArray.hpp        2015-03-26
>     02:34:34.663833288 -0400
>     @@ -168,6 +168,8 @@
>         GrowableArray(int initial_size, bool C_heap = false, MEMFLAGS F
>     = mtInternal)
>           : GenericGrowableArray(initial___size, 0, C_heap, F) {
>           _data = (E*)raw_allocate(sizeof(E));
>     +// Needed for Visual Studio 2012 and older
>     +#pragma warning(suppress: 4345)
>           for (int i = 0; i < _max; i++) ::new ((void*)&_data[i]) E();
>         }
>
>     @@ -385,6 +387,8 @@
>           E* newData = (E*)raw_allocate(sizeof(E));
>           int i = 0;
>           for (     ; i < _len; i++) ::new ((void*)&newData[i]) E(_data[i]);
>     +// Needed for Visual Studio 2012 and older
>     +#pragma warning(suppress: 4345)
>           for (     ; i < _max; i++) ::new ((void*)&newData[i]) E();
>           for (i = 0; i < old_max; i++) _data[i].~E();
>           if (on_C_heap() && _data != NULL) {
>
>     So unless someone finds this totally objectionable it is what I
>     propose to go with. Full webrev at:
>
>     http://cr.openjdk.java.net/~__dholmes/8074895/webrev/
>     <http://cr.openjdk.java.net/~dholmes/8074895/webrev/>
>
>     Thanks,
>     David
>
>
>     On 25/03/2015 2:24 PM, David Holmes wrote:
>
>         On 24/03/2015 2:56 AM, Jeremy Manson wrote:
>
>             Thanks, Kim.  This is a pretty silly warning to have break
>             the build.
>             Does anyone have a problem with PODs being default
>             initialized?  That's
>             required by the standard, so if you do, then you are Doing
>             It Wrong.
>
>             I assume it is pretty easy to turn the warning off.  I'd do
>             it, but I
>             don't have the Windows build-fu necessary.  Also, do we
>             think it would
>             require another bug?
>
>
>         Unless someone else can already tell me how I will try to find the
>         cycles to either disable the warning in that file (if that
>         works) else
>         disable it in the build - which will need a new CR I think.
>
>         David
>
>             I'd hate to have to change my (or any) code for this.
>
>             Jeremy
>
>             On Mon, Mar 23, 2015 at 8:48 AM, Kim Barrett
>             <kim.barrett at oracle.com <mailto:kim.barrett at oracle.com>
>             <mailto:kim.barrett at oracle.com
>             <mailto:kim.barrett at oracle.com>__>> wrote:
>
>                  On Mar 23, 2015, at 3:45 AM, David Holmes
>             <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>                  <mailto:david.holmes at oracle.__com
>             <mailto:david.holmes at oracle.com>>> wrote:
>                  >
>                  > On 23/03/2015 4:12 PM, David Holmes wrote:
>                  >> On 21/03/2015 3:32 AM, Jeremy Manson wrote:
>                  >>> Argh.  Yes.  Martin told me not to get involved
>             with Windows,
>             but would
>                  >>> I listen?  Of course not...
>                  >>>
>
>              >>>http://cr.openjdk.java.net/__~jmanson/8074895/webrev.04/
>             <http://cr.openjdk.java.net/~jmanson/8074895/webrev.04/>
>                  >>
>                  >> Looks okay to me - running a test job now.
>                  >
>                  > <sigh> This just isn't meant to be :( It seems that:
>                  >
>                  > GrowableArray<JavaVMOption> options(2, true);
>                  >
>                  > in arguments.cpp is giving the windows compiler some
>             grief:
>                  >
>                  >
>             C:\jprt\T\P1\071814.daholme\s\__hotspot\src\share\vm\__utilities/growableArray.hpp(__171)
>             : error C2220: warning treated as error - no 'object' file
>             generated
>                  >
>             C:\jprt\T\P1\071814.daholme\s\__hotspot\src\share\vm\__utilities/growableArray.hpp(__168)
>             : while compiling class template member function
>             'GrowableArray<E>::__GrowableArray(int,bool,__MEMFLAGS)'
>                  >        with
>                  >        [
>                  >            E=JavaVMOption
>                  >        ]
>                  >
>             C:\jprt\T\P1\071814.daholme\s\__hotspot\src\share\vm\runtime\__arguments.cpp(3516)
>             : see reference to class template instantiation
>             'GrowableArray<E>'
>             being compiled
>                  >        with
>                  >        [
>                  >            E=JavaVMOption
>                  >        ]
>                  >
>             C:\jprt\T\P1\071814.daholme\s\__hotspot\src\share\vm\__utilities/growableArray.hpp(__171)
>             : warning C4345: behavior change: an object of POD type
>             constructed
>             with an initializer of the form () will be default-initialized
>                  >
>             C:\jprt\T\P1\071814.daholme\s\__hotspot\src\share\vm\__utilities/growableArray.hpp(__388)
>             : warning C4345: behavior change: an object of POD type
>             constructed
>             with an initializer of the form () will be default-initialized
>                  >
>             C:\jprt\T\P1\071814.daholme\s\__hotspot\src\share\vm\__utilities/growableArray.hpp(__379)
>             : while compiling class template member function 'void
>             GrowableArray<E>::grow(int)'
>                  >        with
>                  >        [
>                  >            E=JavaVMOption
>                  >        ]
>                  >
>                  > I'm guessing it doesn't like the enum as the generic
>             arg, but
>             don't know why given that it accepts plain int elsewhere. ???
>
>                  Just suppressing this warning (unconditionally
>             everywhere) would
>                  probably make sense.
>
>                  Microsoft describes it as an obsolete warning:
>
>             https://msdn.microsoft.com/en-__us/library/wewb47ee.aspx
>             <https://msdn.microsoft.com/en-us/library/wewb47ee.aspx>
>
>                  "This warning is obsolete. It is only generated in
>             Visual Studio
>                  2005 through Visual Studio 2012. It reports a behavior
>             change from
>                  the Visual C++ compiler that shipped in Visual Studio
>             .NET when
>                  initializing a POD (plain old data) object with (); the
>             compiler
>                  default-initializes the object.”
>
>                  It’s too bad the JDK9 supported build platform for
>             Windows is still
>                  lagging.
>
>
>


More information about the hotspot-runtime-dev mailing list