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

[David Holmes]

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