RFR (L) JDK-8231844: Enhance type signature characters in classfile_constants.h and improve the JVM to use type signature characters more consistently

[Lois Foltan]

On 10/10/2019 9:15 PM, David Holmes wrote:
> Hi Lois,
>
> On 11/10/2019 3:30 am, Lois Foltan wrote:
>> On 10/9/2019 8:14 PM, David Holmes wrote:
>>> Hi Lois,
>>>
>>> On 10/10/2019 7:18 am, Lois Foltan wrote:
>>>> Please review this enhancement to consistently use the 
>>>> JVM_SIGNATURE_* constants for type signature processing.  This 
>>>> change is a precursor to JDK-8230199: consolidate signature parsing 
>>>> in Hotspot sources.
>>>>
>>>> open webrev 
>>>> at:http://cr.openjdk.java.net/~lfoltan/bug_jdk8231844.0/webrev/ 
>>>> <http://cr.openjdk.java.net/~lfoltan/bug_jdk8231844.0/webrev/>
>>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8231844
>>>> contributed-by: Lois Foltan, John Rose
>>>
>>> In general the change is good - special characters should be 
>>> referred to by symbolic names to give more semantic context where 
>>> practical.
>>
>> Thanks David for the review!  See my comments below.  New webrev at:
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8231844.1/webrev/
>
> Updated look good - thanks.
>
> Follow up comments way below ...
>
>>>
>>> However I do have a few comments.
>>>
>>> It seems redundant and somewhat counter to the fix to change a 
>>> character in the code to a symbolic constant but then have a comment 
>>> still use the original character. Sometimes this seems acceptable as 
>>> it helps clarity e.g.
>>>
>>>     // Ignore wrapping L and ;
>>> !   if (name[0] == JVM_SIGNATURE_CLASS) {
>>>
>>> but in particular I found the comments here unnecessary:
>>>
>>> src/hotspot/share/utilities/globalDefinitions.cpp
>>>
>>>  // Map BasicType to signature character
>>> ! char type2char_tab[T_CONFLICT+1] = {
>>> !   0, 0, 0, 0,
>>> !   JVM_SIGNATURE_BOOLEAN, JVM_SIGNATURE_CHAR,    // 4:  'Z', 'C'
>>> !   JVM_SIGNATURE_FLOAT,   JVM_SIGNATURE_DOUBLE,  //     'F', 'D'
>>> !   JVM_SIGNATURE_BYTE,    JVM_SIGNATURE_SHORT,   // 8:  'B', 'S'
>>> !   JVM_SIGNATURE_INT,     JVM_SIGNATURE_LONG,    //     'I', 'J'
>>> !   JVM_SIGNATURE_CLASS,   JVM_SIGNATURE_ARRAY,   // 12: 'L', '['
>>> !   JVM_SIGNATURE_VOID,    0,                     //     'V'
>>> !   0, 0, 0, 0
>>> ! };
>>>
>>> other cases could go either way.
>>
>> Fixed this in utilities/globalDefinitions.cpp and another instance in 
>> src/hotspot/share/runtime/fieldType.cpp where the comments seemed 
>> redundant.
>>
>>>
>>> ---
>>>
>>> src/java.base/share/native/include/classfile_constants.h.template
>>>
>>> +     JVM_SIGNATURE_ENDPACKAGE    = '/',
>>> +     JVM_SIGNATURE_ENDPACKAGE_DOT = '.',
>>>
>>> These constants bothered me both because of their very long names 
>>> and because the names aren't completely accurate. They both indicate 
>>> separators between identifiers: one for internal form, and one for 
>>> external form. The / doesn't "end a package" and . can be part of a 
>>> nested type name, unrelated to packages. As these characters do not 
>>> have a unique semantic usage that is easily expressed, perhaps 
>>> referring to them as just:
>>>
>>> JVM_SIGNATURE_SLASH
>>> JVM_SIGNATURE_DOT
>>>
>>> would suffice?
>>
>> I agree, changed.
>>
>>>
>>> ---
>>>
>>> src/hotspot/share/c1/c1_InstructionPrinter.cpp
>>>
>>> Should type2name really handle all cases and assert if an invalid 
>>> type, and return "???" (or perhaps "<unknown>") in product mode?
>>
>> A new RFE should be created for addressing how type2name should 
>> handle an invalid type since it is used quite frequently throughout 
>> the JVM. It is not in the scope of this RFE to change the behavior of 
>> type2name. It was the goal to reduce the use of hard coded characters 
>> and/or strings when dealing with types and type signatures which is 
>> what the use of type2name does in InstructionPrinter::basic_type_name().
>
> Agree this is out of scope.
>
>>>
>>> ---
>>>
>>> src/hotspot/share/utilities/globalDefinitions.hpp
>>>
>>> I do not like this new method:
>>>
>>> +inline BasicType primitive_or_object(BasicType t) {
>>> +  assert(is_java_type(t), "%d", (int)t);
>>> +  return (is_reference_type(t) ? T_OBJECT : t);
>>> +}
>>>
>>> I found it's use in the code was far less clear than the original 
>>> code. I like to explicitly see where arrays and objects are being 
>>> treated the same (or differently).
>>
>> This is in line with the change that I recently completed for 
>> JDK-8230505: Replace JVM type comparisons to T_OBJECT and T_ARRAY 
>> with call to is_reference_type.  Please refer to John's response to 
>> Coleen's review comment that I believe addresses your concern as 
>> well. 
>> https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-September/039486.html 
>
>
> I can't say that I agree. A simple query like is_reference_type() is 
> perfectly fine. But reducing all reference types to just T_OBJECT is 
> something that, to me, needs to be consciously chosen when a new 
> reference type is added. This approach could hide bugs as it will be 
> easy to miss locations that might new to handle the new reference type 
> differently. Just my 2c.

Thanks David for your 2c.  Based on your and Fred's feedback I have 
posted a new webrev without the change to primitive_or_object() since 
more discussion is needed.  See new webrev link in my reply to Fred.

Lois

>
> Thanks,
> David
>
>>
>> Thanks,
>> Lois
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Testing: hs-tier1-3, jdk-tier1-2 (all platforms).  hs-tier4-6 
>>>> (linux only)
>>>>
>>>> Thanks,
>>>> Lois
>>