RFR: 8074895: os::getenv is inadequate

Jeremy Manson jeremymanson at google.com
Fri Mar 27 07:25:37 UTC 2015 

Oh, and thank you for doing it!  I didn't think my piddling little P4
change was going to cause so much trouble!

Jeremy

On Fri, Mar 27, 2015 at 12:24 AM, Jeremy Manson <jeremymanson at google.com>
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.
>
> 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.
>
> Jeremy
>
> On Wed, Mar 25, 2015 at 11:41 PM, David Holmes <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/
>>
>> 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>> wrote:
>>>>
>>>>     On Mar 23, 2015, at 3:45 AM, David Holmes <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/
>>>>     >>
>>>>     >> 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
>>>>
>>>>     "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