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

[David Holmes]

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

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