Hi Jini,
Your changes look good.
Thanks!
One point I want to make is that we have the enum BasicTypeSize redefined in SA as public static final values,
I will update BasicTypeSize in new webrev.
An ideal solution would be to include this in vmStructs.cpp as a declare_constant() macro, and read this in SA with the db.lookupIntConstant() method.
I agree with you, but I think it should be another issue (enhancement). I've proposed to change of BasicType in 8185796 [1]. I can file it to JBS now, but I want to create a patch after 8185796 and 8187401 (They are waiting for reviewer(s)). Thanks, Yasumasa [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021926.html On 2017/10/06 14:31, Jini George wrote:
Hi Yasumasa, Your changes look good. One point I want to make is that we have the enum BasicTypeSize redefined in SA as public static final values, and this makes it error prone when existing enum values change, just as in this case. An ideal solution would be to include this in vmStructs.cpp as a declare_constant() macro, and read this in SA with the db.lookupIntConstant() method. This would insulate SA from enum value changes in hotspot to some extent -- but this is just a suggestion, and you can chose to ignore this, since this would mean changing all the following values in BasicType. public static final int tBoolean = 4; public static final int tChar = 5; public static final int tFloat = 6; public static final int tDouble = 7; public static final int tByte = 8; public static final int tShort = 9; public static final int tInt = 10; public static final int tLong = 11; public static final int tObject = 12; public static final int tArray = 13; public static final int tVoid = 14; public static final int tAddress = 15; public static final int tNarrowOop = 16; public static final int tMetadata = 17; public static final int tNarrowKlass = 18; public static final int tConflict = 19; public static final int tIllegal = 99; Thank you, Jini (Not a Reviewer). On 9/29/2017 2:58 PM, Yasumasa Suenaga wrote:Hi all, This change has been reviewed by Serguei. I'm waiting for another reviewer. Thanks, Yasumasa 2017/09/27 ??9:49 "Yasumasa Suenaga" <yasuenag at gmail.com <mailto:yasuenag at gmail.com>>: Hi David, Serguei, I added noreg-hard label and how to reproduce to JBS: https://bugs.openjdk.java.net/browse/JDK-8187401 <https://bugs.openjdk.java.net/browse/JDK-8187401> Also I uploaded new webrev for jdk10/hs: http://cr.openjdk.java.net/~ysuenaga/JDK-8187401/webrev.01/ <http://cr.openjdk.java.net/~ysuenaga/JDK-8187401/webrev.01/> Thanks, Yasumasa 2017-09-27 8:25 GMT+09:00 serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com>>: > On 9/26/17 16:22, David Holmes wrote: >> >> On 27/09/2017 8:52 AM, serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> wrote: >>> >>> Hi David, >>> >>> >>> On 9/26/17 15:09, David Holmes wrote: >>>> >>>> Hi Sergeui, >>>> >>>> On 27/09/2017 3:51 AM, serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> wrote: >>>>> >>>>> Hi Yasumasa, >>>>> >>>>> >>>>> On 9/26/17 02:41, Yasumasa Suenaga wrote: >>>>>> >>>>>> Hi Serguei, >>>>>> >>>>>> Thank you for your comment! >>>>>> >>>>>>> This fix looks Ok to me but you need to add a unit test. >>>>>> >>>>>>? ?I guess it is caused by inlined method which is generated by JIT >>>>>> compiler. I don't know how to reproduce it on jtreg test. >>>>>> Do you have any idea for it? >>>>> >>>>> >>>>> I'm not sure what exact problem you have with jtreg. >>>>> You may want to try to use other jtreg tests as examples. >>>> >>>> >>>> I see two problems: >>>> >>>> 1. hsdb is an interactive GUI tool >>> >>> >>> There is already at least one jtreg hsdb test: >>> open/test/hotspot/jtreg/serviceability/sa/JhsdbThreadInfoTest.java >>> >>> Not sure, if this example would help in this case though. >>> >>>> 2. The problem seems related to JIT inlining - so how do you force that >>>> in a test? >>> >>> >>> Then I wonder how was it forced in the manual reproducer? >>> The fact it is fixed has to be verified anyway. >> >> >> Well the reproducer happens to hit the issue, so we can use it to manually >> verify. >> >>>> I would think this is a noreg-hard situation. As long as there is a >>>> manual reproducer that can be used to verify the fix - as per the bug report >>>> - that should be okay IMHO. >>> >>> >>> I'm Ok with adding noreg-hard label if it is hard to develop. >> >> >> Sounds good to me. The manual verification steps should be very clearly >> spelt out in the bug report so that even someone unfamiliar with hsdb (like >> me!) can follow them easily. > > > Sounds good, thanks. > > Serguei > >> >> Cheers, >> David >> >>> Thanks, >>> Serguei >>> >>>> Cheers, >>>> David >>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>>> Yasumasa >>>>>> >>>>>> >>>>>> 2017-09-26 18:15 GMT+09:00 serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com> >>>>>> <serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com>>: >>>>>>> >>>>>>> Hi Yasumasa, >>>>>>> >>>>>>> This fix looks Ok to me but you need to add a unit test. >>>>>>> >>>>>>> Thanks, >>>>>>> Serguei >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 9/20/17 15:47, Yasumasa Suenaga wrote: >>>>>>>> >>>>>>>> PING: >>>>>>>> >>>>>>>> Have you checked this issue? >>>>>>>> >>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187401/webrev.00/ <http://cr.openjdk.java.net/~ysuenaga/JDK-8187401/webrev.00/> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Yasumasa >>>>>>>> >>>>>>>> >>>>>>>> On 2017/09/11 11:16, Yasumasa Suenaga wrote: >>>>>>>>> >>>>>>>>> Hi all, >>>>>>>>> >>>>>>>>> This review request is a part of [1]. >>>>>>>>> >>>>>>>>> >>>>>>>>> JBS: >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8187401 <https://bugs.openjdk.java.net/browse/JDK-8187401> >>>>>>>>> >>>>>>>>> webrev: >>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187401/webrev.00/ <http://cr.openjdk.java.net/~ysuenaga/JDK-8187401/webrev.00/> >>>>>>>>> >>>>>>>>> >>>>>>>>> I cannot access JPRT. So I need a sponsor. >>>>>>>>> >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> >>>>>>>>> Yasumasa >>>>>>>>> >>>>>>>>> >>>>>>>>> [1] >>>>>>>>> >>>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html <http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html> >>>>>>>>> >>>>> >>> >
