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

[Chris Plummer]

Hi Jini,

Ok, that's all I needed to hear. Just wanted to make sure the disruption 
was being considered.

thanks,

Chris

On 11/28/17 10: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.
>
> 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 
>>>>>>
>>>>>>
>>>>>>
>>