RFR: 8074895: os::getenv is inadequate
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Mar 30 14:51:36 UTC 2015
On 3/29/15, 9:39 PM, David Holmes wrote:
> 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!
This seems fine although I think I'd prefer the #pragma nowarnings out
of the middle of the functions to not interrupt reading of these
functions. I don't think #pragmas are scoped.
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