RFR: 8190837: BasicType and BasicTypeSize should refer to HotSpot values

Wed Nov 29 08:42:46 UTC 2017

Hi David,

> That can be fixed using a no-arg
> constructor for static initialization and adding a private setType method
> used for the real initialization.

I uploaded new webrev. Is it okay?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.01/

> I'm not at all clear why we need the tXxx and T_XXX forms? The former can be
> obtained from the latter.

I agree with you, but it is difficult.
For example, [1] has a lot of lines which use BasicType.

I had a lot of compile errors when I removed getTXxx methods from BasicType...

Thanks,

Yasumasa

[1] http://hg.openjdk.java.net/jdk/hs/file/5a449dbca6d0/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/Bytecodes.java#l560

2017-11-29 16:01 GMT+09:00 David Holmes <david.holmes at oracle.com>:
> On 29/11/2017 4:19 PM, Jini George wrote:
>>
>> Hi Chris,
>>
>> Thank you for raising this. I agree it is disruptive, but we are trying to
>> address the issue of frequent SA breakages with hotspot changes, and the
>> causes of these breakages. One of the reasons is the redefinition of
>> constants, which is extremely error prone. There have been multiple cases
>> where the changes get done in hotspot and the corresponding changes get
>> inadvertently missed out in SA. And this does not get caught, sometimes, for
>> months. I believe that the switch case statements conversion to if-else
>> statements is not a heavy price to pay for avoiding errors like these.
>
>
> I agree it is good to ensure this always matches the VM. I also agree it is
> unfortunate we lost the ability to keep the switch statements - so be it.
> I'm more concerned that the BasicType static fields are no longer final
> (that may raise parfait warnings!). That can be fixed using a no-arg
> constructor for static initialization and adding a private setType method
> used for the real initialization.
>
> I'm not at all clear why we need the tXxx and T_XXX forms? The former can be
> obtained from the latter. And with the change to use the getTXxx() functions
> I think we can actually do away with all the tXxx static fields. The
> getTXxx() functions can be implemented as "return T_XXX.getType(); and the
> intToBasicType() function can also examine getType(). The name could be
> stored as a field, set at construction time. Just a thought. :)
>
> Thanks,
> David
>
>
>> Thanks!
>> - Jini.
>>
>> On 11/29/2017 7:56 AM, Chris Plummer wrote:
>>>
>>> On 11/28/17 5:23 PM, Yasumasa Suenaga wrote:
>>>>
>>>> Hi Chris,
>>>>
>>>>> I understood the reason for getting rid of the case statements. I was
>>>>> just
>>>>> wondering if you weighed this code disruption vs. the value of what you
>>>>> are
>>>>> fixing.
>>>>
>>>> Jini has pointed it as below and I agree with him:
>>>>
>>>>
>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
>>>> -------------
>>>> 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.
>>>> -------------
>>>
>>> Hi Yasumasa,
>>>
>>> Yes, I had read that and understand the point being made. What I'm asking
>>> is now that you've implemented it and seen the disruption to the switch
>>> statements (which I assume you and Jini were not aware of before embarking
>>> on this), is it still worth doing? It's not really that big of a deal to me.
>>> I just want to make sure it's been taken into consideration.
>>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> 2017-11-29 10:09 GMT+09:00 Chris Plummer <chris.plummer at oracle.com>:
>>>>>
>>>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>>>>
>>>>>> Hi Chris,
>>>>>>
>>>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <chris.plummer at oracle.com>:
>>>>>>>
>>>>>>> Hi Yasumasa,
>>>>>>>
>>>>>>> This isn't code I know very well, and I'm not a Reviewer. Just a
>>>>>>> couple
>>>>>>> of
>>>>>>> observations.
>>>>>>>
>>>>>>> I wonder if the person who originally suggested this change realized
>>>>>>> the
>>>>>>> disruption it would have to existing switch statements. I'm not
>>>>>>> saying
>>>>>>> the
>>>>>>> change shouldn't be done for this reason, but it is something to at
>>>>>>> least
>>>>>>> consider.
>>>>>>
>>>>>> According to JLS, `case` label need to have constant expression.
>>>>>> We cannot set `static final` to the field which is set at
>>>>>> initialize().
>>>>>>
>>>>>>
>>>>>> https://docs.oracle.com/javase/specs/jls/se9/html/jls-14.html#jls-14.11
>>>>>
>>>>> I understood the reason for getting rid of the case statements. I was
>>>>> just
>>>>> wondering if you weighed this code disruption vs. the value of what you
>>>>> are
>>>>> fixing.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Your coding pattern for the following differs from the existing 200+
>>>>>>> instances of VM.registerVMInitializedObserver() calls already in
>>>>>>> place. I
>>>>>>> suggest you be consistent with existing code.
>>>>>>>
>>>>>>>     71   static {
>>>>>>>     72     VM.registerVMInitializedObserver(
>>>>>>>     73         (o, d) -> initialize(VM.getVM().getTypeDataBase()));
>>>>>>>     74   }
>>>>>>
>>>>>> This style has been used in JavaThreadsPanel.java .
>>>>>
>>>>> Ah, I missed that one case, but then it's one that you added. :)
>>>>>>
>>>>>> I like it because it is simple.
>>>>>>
>>>>>> I will change it to traditional style if other reviewer(s) suggest it.
>>>>>
>>>>> I think consistency is important.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>>
>>>>>>> On 11/27/17 11:49 PM, Yasumasa Suenaga wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Enum values in BasicType and BasicTypeSize are declared as const
>>>>>>> values. However, it makes error prone when existing enum values
>>>>>>> change.
>>>>>>> They should refer to HotSpot values via VMStructs.
>>>>>>>
>>>>>>> This issue has been pointed by Jini [1].
>>>>>>>
>>>>>>> I uploaded webrev for this issue. Could you review it?
>>>>>>>
>>>>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.00/
>>>>>>>
>>>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>>
>>>>>>>
>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
>>>>>>>
>>>>>>>
>>>
>