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

>
> 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().

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

Thanks,
Lois

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