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

Lois Foltan lois.foltan at oracle.com
Tue Jan 28 20:50:42 UTC 2020


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.

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

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

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

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



More information about the hotspot-runtime-dev mailing list