Fwd: Re: [PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Sep 5 03:12:32 UTC 2016
Hi Egor,
Unfortunately, we are in the RDP1 phase from September 1-st.
Only P1-P3 bugs are allowed for fix.
We need to find out what is the process if we want to fix this bug in RDP1.
I'll check with colleagues what options exist.
Thanks,
Serguei
On 9/2/16 03:55, Egor Ushakov wrote:
> The resulting webrev with all fixes:
> http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.02/
>
> Thanks,
> Egor
>
> On 01.09.2016 20:20, serguei.spitsyn at oracle.com wrote:
>> Forwarding to the open serviceability mailing list.
>>
>> Thanks,
>> Serguei
>>
>> On 9/1/16 10:17, Dmitry Samersoff wrote:
>>> Egor,
>>>
>>> The fix looks good for me.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2016-09-01 20:01, serguei.spitsyn at oracle.com wrote:
>>>> I hope, we got a review from Dmitry.
>>>> Dmitry, please, confirm.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 9/1/16 10:00, serguei.spitsyn at oracle.com wrote:
>>>>> On 9/1/16 08:41, Egor Ushakov wrote:
>>>>>> I've found how to fix this!
>>>>>>
>>>>>> Add @modules section to the test:
>>>>>>
>>>>>> ->>> * @modules jdk.jdi/com.sun.tools.jdi
>>>>> Nice, thanks!
>>>>>
>>>>>
>>>>>> * @run build TestScaffold VMConnection * @run compile -g
>>>>>> ConstantPoolInfoGC.java * @run main ConstantPoolInfoGC
>>>>>> Please tell me if you need a new webrev.
>>>>> Yes, please.
>>>>> Also, please add the copyright comment:
>>>>>
>>>>>
>>>>> /*
>>>>> * Copyright (c) 2016, Oracle and/or its affiliates. All rights
>>>>> reserved.
>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>> *
>>>>> * This code is free software; you can redistribute it and/or
>>>>> modify it
>>>>> * under the terms of the GNU General Public License version 2
>>>>> only, as
>>>>> * published by the Free Software Foundation.
>>>>> *
>>>>> * This code is distributed in the hope that it will be useful, but
>>>>> WITHOUT
>>>>> * ANY WARRANTY; without even the implied warranty of
>>>>> MERCHANTABILITY or
>>>>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
>>>>> License
>>>>> * version 2 for more details (a copy is included in the LICENSE
>>>>> file that
>>>>> * accompanied this code).
>>>>> *
>>>>> * You should have received a copy of the GNU General Public License
>>>>> version
>>>>> * 2 along with this work; if not, write to the Free Software
>>>>> Foundation,
>>>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>>>> *
>>>>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA
>>>>> 94065 USA
>>>>> * or visit www.oracle.com if you need additional information or
>>>>> have any
>>>>> * questions.
>>>>> */
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Egor
>>>>>> On 01.09.2016 17:41, Egor Ushakov wrote:
>>>>>>> Serguei, it seems that jdk.jdi module in java 9 does not export
>>>>>>> com.sun.tools.jdi package. I have not found a way to get access to
>>>>>>> the constantPoolBytesRef field in ReferenceTypeImpl yet. Do you
>>>>>>> know
>>>>>>> if there's a way *in tests* to get the access to unexported
>>>>>>> packages? Thanks, Egor
>>>>>>> On 01.09.2016 13:35, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Egor, Sorry for the long silence. As we discussed before the
>>>>>>>> test needs to be in the jtreg form that works for jdk 9. Do you
>>>>>>>> have any progress with this? Otherwise, how would we be able to
>>>>>>>> ensure the fix works as expected? We could try to push your fix
>>>>>>>> without a unit test a couple of month ago. But it is too risky at
>>>>>>>> this stage of the development process - sorry. I'm still thinking
>>>>>>>> how to help and make your fix in. There might be some other tests
>>>>>>>> like this one that requires this kind of special handling. Thanks,
>>>>>>>> Serguei On 8/29/16 02:52, Egor Ushakov wrote:
>>>>>>>>> Hi guys! Any news on this, is there anything else needed from my
>>>>>>>>> side? Thanks! On 03.08.2016 12:24, Egor Ushakov wrote:
>>>>>>>>>> Hi Dmitry! I'm not sure what format is expected, the bug is:
>>>>>>>>>> http://bugs.java.com/view_bug.do?bug_id=6822627 JDK-6822627 :
>>>>>>>>>> java.lang.NullPointerException at
>>>>>>>>>> ReferenceTypeImpl.constantPool(ReferenceTypeImpl.java:1025)
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.01/
>>>>>>>>>> Reviewers were not yet assigned (if Serguei is not one of them).
>>>>>>>>>> Egor On 02.08.2016 22:47, Dmitry Samersoff wrote:
>>>>>>>>>>> Egor, I'll sponsor the fix. Could you send me commit
>>>>>>>>>>> comments in
>>>>>>>>>>> OJDK format NNNNN: Synopsys Summary: Reviewed-by:
>>>>>>>>>>> Contributed-by: egor.ushakov at jetbrains.com Webrev URL: -Dmitry
>>>>>>>>>>> On 2016-07-25 11:01, Egor Ushakov wrote:
>>>>>>>>>>>> Dmitry, could you please sponsor the fix? Thanks, Egor
>>>>>>>>>>>> --------
>>>>>>>>>>>> Forwarded Message -------- Subject: Re: [PATCH] 6822627:
>>>>>>>>>>>> NPE at ReferenceTypeImpl.constantPool Date: Fri, 22 Jul
>>>>>>>>>>>> 2016 22:58:59 -0700 From: serguei.spitsyn at oracle.com
>>>>>>>>>>>> <serguei.spitsyn at oracle.com> To: Egor Ushakov
>>>>>>>>>>>> <egor.ushakov at jetbrains.com> On 7/22/16 03:14, Egor Ushakov
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> Serguei, there is no hurry, however if you could recommend
>>>>>>>>>>>>> someone who can sponsor the fix, that would be great.
>>>>>>>>>>>> Egor, You can ask Dmitry Samersoff. But he is on vacation now.
>>>>>>>>>>>> Thanks, Serguei
>>>>>>>>>>>>> Thanks anyway, Egor On 22.07.2016 13:07,
>>>>>>>>>>>>> serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>> Hi Egor, Unfortunately for this bug I have a 4-week vacation
>>>>>>>>>>>>>> starting next Monday. It feels like there is not enough time
>>>>>>>>>>>>>> to resolve the testing issue and push the fix. I'd
>>>>>>>>>>>>>> suggest to
>>>>>>>>>>>>>> postpone it for a month. Does it work for you? We need to
>>>>>>>>>>>>>> find a solution for this test problem. Alternatively, you
>>>>>>>>>>>>>> can
>>>>>>>>>>>>>> try to find another sponsor for it. On 7/21/16 01:19, Egor
>>>>>>>>>>>>>> Ushakov wrote:
>>>>>>>>>>>>>>> Hi Serguei, You probably need jdk, not jre at ${JAVA_HOME}.
>>>>>>>>>>>>>>> I do not know how to simulate SoftReference ref being gced
>>>>>>>>>>>>>>> without accessing the internal field :(
>>>>>>>>>>>>>> I do not have a good idea either.
>>>>>>>>>>>>>>> We can try to run jvm with -XX:SoftRefLRUPolicyMSPerMB=1
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>> invoke gc between the requests, but this does not guarantee
>>>>>>>>>>>>>>> SoftReference clearance anyway.
>>>>>>>>>>>>>> There only way is to stress by eating a lot of memory.
>>>>>>>>>>>>>> However, the tests that use such approach are normally
>>>>>>>>>>>>>> unstable. Let's think a little bit more on this. Thanks,
>>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>>> Egor On 21.07.2016 8:31, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>>> Hi Egor, How do you run the test? I'm getting the errors:
>>>>>>>>>>>>>>>> /var/tmp/sspitsyn/dbg/jdk/test/com/sun/jdi/ConstantPoolInfoGC.java:38:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> error: package com.sun.tools.jdi does not exist import
>>>>>>>>>>>>>>>> com.sun.tools.jdi.ReferenceTypeImpl;
>>>>>>>>>>>>>>>> ^
>>>>>>>>>>>>>>>> /var/tmp/sspitsyn/dbg/jdk/test/com/sun/jdi/ConstantPoolInfoGC.java:74:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> error: cannot find symbol Field
>>>>>>>>>>>>>>>> constantPoolBytesRef =
>>>>>>>>>>>>>>>> ReferenceTypeImpl.class.getDeclaredField("constantPoolBytesRef");
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ^ symbol:
>>>>>>>>>>>>>>>> class ReferenceTypeImpl location: class
>>>>>>>>>>>>>>>> ConstantPoolInfoGC 2 errors My test run is:
>>>>>>>>>>>>>>>> /java/re/jtreg/4.2/promoted/latest/binaries/jtreg/bin/jtreg
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> \ -jdk ${JAVA_HOME} \ -J-Dtest.java.opts='-bits d64
>>>>>>>>>>>>>>>> -Xmixed -server' \ -Dtest.java.opts='-bits d64 -Xmixed
>>>>>>>>>>>>>>>> -server' \
>>>>>>>>>>>>>>>> /var/tmp/sspitsyn/dbg/jdk/test/com/sun/jdi/ConstantPoolInfoGC.java
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> It is normally enough for any jtreg test run. The test
>>>>>>>>>>>>>>>> should not use the internal knowledge of the
>>>>>>>>>>>>>>>> implementation
>>>>>>>>>>>>>>>> as it is in the ConstantPoolInfoGC.java. Thanks,
>>>>>>>>>>>>>>>> Serguei On
>>>>>>>>>>>>>>>> 7/20/16 02:53, Egor Ushakov wrote:
>>>>>>>>>>>>>>>>> Yes, all correct, thanks! On 20.07.2016 12:34,
>>>>>>>>>>>>>>>>> serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>>>>> Egor, If I understand correctly, you do not have an
>>>>>>>>>>>>>>>>>> openJdk user ID and an author status. Please, see the
>>>>>>>>>>>>>>>>>> link: http://openjdk.java.net/census So that, I'll
>>>>>>>>>>>>>>>>>> commit your fix with the comment: Contributed-by:
>>>>>>>>>>>>>>>>>> egor.ushakov at jetbrains.com and push it to the jdk9/hs. I
>>>>>>>>>>>>>>>>>> hope, somebody will correct me if it is not right.
>>>>>>>>>>>>>>>>>> Please, let me know if it works for you. Thanks, Serguei
>>>>>>>>>>>>>>>>>> On 7/20/16 02:10, Egor Ushakov wrote:
>>>>>>>>>>>>>>>>>>> Serguei, thanks for the review! Please sponsor the fix,
>>>>>>>>>>>>>>>>>>> I do not know how to proceed. Thanks! Egor On
>>>>>>>>>>>>>>>>>>> 20.07.2016
>>>>>>>>>>>>>>>>>>> 10:36, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>>>>>>> Hi Egor, Thank you for providing the test! A couple of
>>>>>>>>>>>>>>>>>>>> nits to the test: 56 if
>>>>>>>>>>>>>>>>>>>> (!Arrays.equals(cpbytes, cpbytes2)) {
>>>>>>>>>>>>>>>>>>>> 57 failure("Consequent constantPool
>>>>>>>>>>>>>>>>>>>> results vary, first was : " + cpbytes + ", now: " +
>>>>>>>>>>>>>>>>>>>> cpbytes2); 58 }; Last semicolon is
>>>>>>>>>>>>>>>>>>>> not need. 74 if (!testFailed) { 75
>>>>>>>>>>>>>>>>>>>> println("ConstantPoolInfoGC: passed"); 76 }
>>>>>>>>>>>>>>>>>>>> else { 77 throw new
>>>>>>>>>>>>>>>>>>>> Exception("ConstantPoolInfoGC: failed"); 78
>>>>>>>>>>>>>>>>>>>> } I'd suggest to rearrange the statement above to
>>>>>>>>>>>>>>>>>>>> something like this: 74 if (testFailed) {
>>>>>>>>>>>>>>>>>>>> 75 throw new
>>>>>>>>>>>>>>>>>>>> Exception("ConstantPoolInfoGC:
>>>>>>>>>>>>>>>>>>>> failed"); 76 } 77
>>>>>>>>>>>>>>>>>>>> println("ConstantPoolInfoGC: passed"); But I leave
>>>>>>>>>>>>>>>>>>>> it up to you. No need to see another webrev. I
>>>>>>>>>>>>>>>>>>>> can sponsor your fix, if needed. Thanks, Serguei On
>>>>>>>>>>>>>>>>>>>> 7/19/16 00:07, Egor Ushakov wrote:
>>>>>>>>>>>>>>>>>>>>> Hi Serguei, here's a new webrev with a test included:
>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.01/
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Egor On 15.07.2016 12:22, serguei.spitsyn at oracle.com
>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>> Hi Egor, The fix looks good modulo the
>>>>>>>>>>>>>>>>>>>>>> synchronization issue. Do you have a jtreg unit test
>>>>>>>>>>>>>>>>>>>>>> reproducing this failure mode? We have a rule to
>>>>>>>>>>>>>>>>>>>>>> provide a test coverage for the fixes. Thanks,
>>>>>>>>>>>>>>>>>>>>>> Serguei On 7/15/16 00:03, Egor Ushakov wrote:
>>>>>>>>>>>>>>>>>>>>>>> Thanks for your comments! I totally agree that the
>>>>>>>>>>>>>>>>>>>>>>> code there requires major refactoring. In the fix I
>>>>>>>>>>>>>>>>>>>>>>> tried not to make it worse and fix the NPE. On
>>>>>>>>>>>>>>>>>>>>>>> 14.07.2016 20:06, Martin Buchholz wrote:
>>>>>>>>>>>>>>>>>>>>>>>> The lack of volatile or synchronization, plus the
>>>>>>>>>>>>>>>>>>>>>>>> use of multiple mutable fields, suggests that a
>>>>>>>>>>>>>>>>>>>>>>>> deeper fix is necessary to be truly correct. For
>>>>>>>>>>>>>>>>>>>>>>>> comparison, much of the similar code implementing
>>>>>>>>>>>>>>>>>>>>>>>> reflection has been changed to use volatile. But
>>>>>>>>>>>>>>>>>>>>>>>> that's a big job, for the serviceability team. On
>>>>>>>>>>>>>>>>>>>>>>>> Thu, Jul 14, 2016 at 2:33 AM, Egor Ushakov
>>>>>>>>>>>>>>>>>>>>>>>> <egor.ushakov at jetbrains.com
>>>>>>>>>>>>>>>>>>>>>>>> <mailto:egor.ushakov at jetbrains.com>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>> Hi, I'm a developer from IDEA debugger (we have
>>>>>>>>>>>>>>>>>>>>>>>> company OCA). We've increased usage of
>>>>>>>>>>>>>>>>>>>>>>>> ReferenceTypeImpl.constantPool and now facing
>>>>>>>>>>>>>>>>>>>>>>>> JDK-6822627 more often. Please review and sponsor
>>>>>>>>>>>>>>>>>>>>>>>> the fix. The problem was that
>>>>>>>>>>>>>>>>>>>>>>>> constantPoolBytesRef SoftReference value coud
>>>>>>>>>>>>>>>>>>>>>>>> be GCed and there was no check for that. I've
>>>>>>>>>>>>>>>>>>>>>>>> added it to the check inside
>>>>>>>>>>>>>>>>>>>>>>>> getConstantPoolInfo to request the bytes again in
>>>>>>>>>>>>>>>>>>>>>>>> this case. BUGURL:
>>>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6822627
>>>>>>>>>>>>>>>>>>>>>>>> WEBREV:
>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.00/
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eavu/JDK-6822627/webrev.00/>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Thanks!-- Egor Ushakov Software Developer
>>>>>>>>>>>>>>>>>>>>>>>> JetBrains http://www.jetbrains.com The Drive
>>>>>>>>>>>>>>>>>>>>>>>> to Develop
>>>>>>>>>>>>>>>>>>>>>>> -- Egor Ushakov Software Developer JetBrains
>>>>>>>>>>>>>>>>>>>>>>> http://www.jetbrains.com The Drive to Develop
>>>>>>>>>>>>>>>>>>>>> -- Egor Ushakov Software Developer JetBrains
>>>>>>>>>>>>>>>>>>>>> http://www.jetbrains.com The Drive to Develop
>>>>>>>>>>>>>>>>>>> -- Egor Ushakov Software Developer JetBrains
>>>>>>>>>>>>>>>>>>> http://www.jetbrains.com The Drive to Develop
>>>>>>>>>>>>>>>>> -- Egor Ushakov Software Developer JetBrains
>>>>>>>>>>>>>>>>> http://www.jetbrains.com The Drive to Develop
>>>>>>>>>>>>>>> -- Egor Ushakov Software Developer JetBrains
>>>>>>>>>>>>>>> http://www.jetbrains.com The Drive to Develop
>>>>>>>>>>>>> -- Egor Ushakov Software Developer JetBrains
>>>>>>>>>>>>> http://www.jetbrains.com The Drive to Develop
>>>>>>> --
>>>>>>> Egor Ushakov
>>>>>>> Software Developer
>>>>>>> JetBrains
>>>>>>> http://www.jetbrains.com
>>>>>>> The Drive to Develop
>>>>>> --
>>>>>> Egor Ushakov
>>>>>> Software Developer
>>>>>> JetBrains
>>>>>> http://www.jetbrains.com
>>>>>> The Drive to Develop
>>>
>>
>
More information about the serviceability-dev
mailing list