RFR: 8074895: os::getenv is inadequate
David Holmes
david.holmes at oracle.com
Mon Mar 30 21:27:31 UTC 2015
On 31/03/2015 6:39 AM, Coleen Phillimore wrote:
>
> On 3/30/15, 4:32 PM, David Holmes wrote:
>> On 31/03/2015 12:51 AM, Coleen Phillimore wrote:
>>>
>>> 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.
>>
>> This one is, it applies only to the next line:
>>
>> https://msdn.microsoft.com/en-us/library/2c8f766e.aspx
>>
>> I was attempting to minimize the impact by only disabling the warning
>> where it was occurring. But I can broaden the scope to cover the whole
>> function with a push/pop instead if people really think that would be
>> better.
>
> Oh, bizarre. Okay, leave it then.
I will leave it and push in its current form.
Thanks,
David
> Coleen
>
>>
>> Thanks,
>> David
>>
>>> 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