RFR (L) JDK-8230199: consolidate signature parsing code in HotSpot sources
David Holmes
david.holmes at oracle.com
Mon Feb 3 05:18:29 UTC 2020
Hi Lois,
Sorry for the delay in looking at this. This is a nice set of
consolidations and simplifications. I few mostly minor comments/queries
below.
On 29/01/2020 6:50 am, 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
In the sharedRuntime_<cpu> changes I'm curious about the change to the
assertion:
- assert(in_sig_bt[i] == ss.type(), "must match");
+ assert(in_sig_bt[i] == ss.type() ||
+ in_sig_bt[i] == T_ARRAY, "must match");
That would seem to be an existing bug - no? Given the code is:
2001 if (in_sig_bt[i] == T_ARRAY) {
...
2009 } else {
2010 out_sig_bt[argc++] = in_sig_bt[i];
2011 in_elem_bt[i] = T_VOID;
2012 }
2013 if (in_sig_bt[i] != T_VOID) {
2014 assert(in_sig_bt[i] == ss.type() ||
2015 in_sig_bt[i] == T_ARRAY, "must match");
then we mustn't have any tests that actually exercise this otherwise we
would fail the assert! Or does ss.type() now behave differently to before?
2008 // else what is in_elem_bt[i]?
This comment/question doesn't seem to serve any purpose. If we think
there couldn't possibly be anything other than an array of primitives
then we should probably add some form of guarantee - just as the
original code caught that case with ShouldNotReachHere.
---
src/hotspot/share/classfile/systemDictionary.cpp
Minor nit:
2181 Symbol* obj_class = ss.as_symbol();
2182 klass = constraints()->find_constrained_klass(obj_class,
class_loader);
You introduced the local obj_class but it is only used once and so it
not necessary. (In other places you removed use-once locals.)
---
src/hotspot/share/classfile/verificationType.cpp
Observation: The switch in get_component seems to indicate a missing
type2name style conversion helper.
134 case T_ARRAY:
135 case T_OBJECT: {
136 guarantee(ss.is_reference(), "unchecked verifier input?");
137 Symbol* component = ss.as_symbol();
Is it that case as_symbol returns a class name without envelope, but an
array type name with envelop? I'm trying to reconcile the old and new
code where T_ARRAY and T_OBJECT were handled distinctly.
---
src/hotspot/share/oops/constMethod.cpp
! set_result_type((BasicType)0);
This change seems a little odd. Why 0? What about T_ILLEGAL if not
intended to be a real type?
---
src/hotspot/share/prims/methodHandles.cpp
549 case T_VOID:
550 case T_INT: case T_LONG: case T_FLOAT: case T_DOUBLE:
This is an inconsistent and unusual style. One case per line is the
common norm.
---
src/hotspot/share/runtime/signature.hpp
57 // The primitive field types (like 'I') correspond 1-1 with type codes
58 // (like T_INT) which part of the specfication of the 'newarray'
59 // instruction (JVMS 6.5, section on newarray).
There is a word missing - perhaps "which form part of ..." ?
73 // "envelope" syntax which starts with 'L' (or 'Q') and ends with ';'.
Can we add 'Q' when/if we get Q-types please :)
101 // determine if it (in fact) cannot be a class name.
The use of "(in fact)" seems a little odd, and elsewhere is used without
parentheses.
389 case T_BYTE: case T_SHORT: case T_INT:
390 case T_BOOLEAN: case T_CHAR:
Odd case style again.
397 #ifdef _LP64
398 pass_double(); _jni_offset++; _offset += 2;
399 #else
400 pass_double(); _jni_offset += 2; _offset += 2;
401 #endif
How about:
int jni_offset = LP_64_ONLY(1) NOT_LP64(2)
pass_double(); _jni_offset += jni_offset; _offset += 2;
Ditto for T_LONG case.
528 Symbol* as_symbol() {
529 return find_symbol();
530 }
Is the new as_symbol equivalent to the old as_symbol_or_null() ? It
doesn't appear to allow NULL return yet there are callsites that still
check the result for NULL (ie nmethod.cpp, method.cpp, signature.cpp).
Do we no longer need an allocation constrained version of as_symbol() ?
---
Thanks,
David
-----
>>
>> 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