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

[Jini George]

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 
>>>>>
>>>>>
>>>>>
>