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

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.

---

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?

---

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?

---

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

Thanks,
David
-----

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