[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR: 8074895: os::getenv is inadequate
From: Jeremy Manson <jeremymanson () google ! com>
Date: 2015-03-31 3:34:01
Message-ID: CAPYFHW3gv0kNf3gfLB+M=y4_iEAtddSkO42V0oWPxR6C_MF=Zg () mail ! gmail ! com
[Download RAW message or body]
Thanks, David! I'm sorry it was so much work. Hopefully, I will be able to
avoid breaking things on Windows in the future.
Jeremy
On Mon, Mar 30, 2015 at 2:27 PM, David Holmes <david.holmes@oracle.com>
wrote:
> 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@oracle.com
>>>>>> <mailto:david.holmes@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@oracle.com <mailto:kim.barrett@oracle.com>
>>>>>> <mailto:kim.barrett@oracle.com
>>>>>> <mailto:kim.barrett@oracle.com>__>> wrote:
>>>>>>
>>>>>> On Mar 23, 2015, at 3:45 AM, David Holmes
>>>>>> <david.holmes@oracle.com <mailto:david.holmes@oracle.com>
>>>>>> <mailto:david.holmes@oracle.__com
>>>>>> <mailto:david.holmes@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.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic