RFR (L) JDK-8230199: consolidate signature parsing code in HotSpot sources
Harold Seigel
harold.seigel at oracle.com
Thu Jan 30 20:25:59 UTC 2020
Hi Lois,
The latest webrev looks good. Thanks for responding to my small
off-line comments.
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
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list