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

[Lois Foltan]

On 1/30/2020 3:25 PM, Harold Seigel wrote:
> Hi Lois,
>
> The latest webrev looks good.  Thanks for responding to my small 
> off-line comments.

Thank you Harold for the review!
Lois

>
> Harold
>
> On 1/28/2020 6:26 PM, Lois Foltan wrote:
>> On 1/28/2020 5:03 PM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> 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]?
>>
>> Yes, I agree.  Will remove those before commit.  Thanks for the final 
>> review!
>> Lois
>>
>>>
>>>>
>>>>>
>>>>> 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
>>>>>
>>>>
>>>
>>