RFR (L) JDK-8230199: consolidate signature parsing code in HotSpot sources

[coleen.phillimore at oracle.com]

On 1/28/20 3:50 PM, Lois Foltan wrote:
> On 1/27/2020 4:36 PM, coleen.phillimore at oracle.com wrote:
>>
>> Lois,  So many nice cleanups in this patch!
>
> Thanks for your review Coleen!  Responses interspersed below.  New 
> webrev is at: 
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8230199.2/webrev/index.html
>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8230199.0/webrev/src/hotspot/cpu/sparc/sharedRuntime_sparc.cpp.frames.html 
>>
>>
>> 1910 ss.skip_array_prefix(1); // skip one '['
>> 1911 if (ss.is_primitive())
>> 1912 in_elem_bt[i] = ss.type();
>> 1913 // else what is in_elem_bt[i]?
>>
>> This code is for the not-well kept secret JavaCritical, where the 
>> native function can only take an array of primitives (checked 
>> elsewhere that I can't find right now).  So I believe this code can 
>> just assert ss.is_primitive() for all these cases.  Only code on 
>> solaris sparc uses this feature, so you can run tests on sparc to 
>> verify.
>
> Done. Changed all to:
> out_sig_bt[argc++] = T_INT; out_sig_bt[argc++] = T_ADDRESS; 
> ss.skip_array_prefix(1); // skip one '[' assert(ss.is_primitive(), 
> "primitive type expected"); in_elem_bt[i] = ss.type(); // else what is 
> in_elem_bt[i]?
> Will run a sparc build & test.

I don't think you need this comment anymore.

+ // else what is in_elem_bt[i]?


>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8230199.0/webrev/src/hotspot/share/classfile/systemDictionary.cpp.frames.html 
>>
>>
>> 2207 if (!Signature::is_array(class_name)) {
>>
>>
>> Did you try adding SignatureStream before this, to avoid the Symbol 
>> refcounting madness?
>
> Yes I did try that.  However, the issue with that approach is that you 
> may have a non signature string in the case where it is not an array.  
> This is the condition of the first part of the if statement on line 
> #2207.  Here you can just simply use the Symbol* as the constraint 
> name.  If you try to construct a SignatureStream with this 
> non-signature Symbol* a crash occurs immediately. SignatureStream 
> assumes the Symbol* is a field or method signature not just a plain 
> name for example something like "java/lang/String" without a leading 
> 'L' or a leading '('.

Oh too bad.  It looks good then.
>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8230199.0/webrev/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp.frames.html 
>>
>>
>> hpp files shouldn't include an inline.hpp file.  Can this include 
>> oop.hpp instead?
>
> I removed the inclusion of oop.inline.hpp and removed the following 
> assert in JavaArgumentUnboxer::do_type().
> 161     assert(arg->klass() == SystemDictionary::box_klass(type), "already checked");
> It turns out that in the statement prior to this assert, a call is 
> made to JavaArgumentUnboxer::next_arg(type) which already does the 
> check for us.  Thus the "already checked" comment.  No need for a 
> duplicate assert.

Perfect.

>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8230199.0/webrev/src/hotspot/share/oops/symbol.cpp.frames.html 
>>
>>
>> Should SignatureStream arguments be const references?  Or should they 
>> be written to?
>
> I was able to add a const to the SignatureStream& parameter for the 
> static method print_class().  For print_array(), however, I was not 
> able to since ss.skip_array_prefix() does write to 
> SignatureStream::_type field the type after skipping the array brackets.

Ok. thanks.

Latest version looks good to me.

Coleen

>
>>
>> This looks really good!
>
> Thanks again!
> Lois
>
>> Coleen
>>
>>
>> On 1/24/20 4:41 PM, Lois Foltan wrote:
>>> Please review the following enhancement to consolidate signature 
>>> parsing code in Hotspot sources. This change removes duplicate 
>>> blocks of code that parse field or method signatures, provides a new 
>>> Signature class to support basic signature queries on Symbol 
>>> operands and enhances the SignatureStream class to parse field 
>>> signatures in addition to methods.
>>>
>>> open webrev 
>>> at:http://cr.openjdk.java.net/~lfoltan/bug_jdk8230199.0/webrev/ 
>>> <http://cr.openjdk.java.net/~lfoltan/bug_jdk8230199.0/webrev/>
>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8230199
>>> contributed-by: Lois Foltan, John Rose
>>>
>>> Testing: hs-tier1-8, jdk-tier1-2
>>>
>>> Thanks,
>>> Lois
>>
>