RFR (S) 8223657: Remove unused THREAD argument from SymbolTable functions

David Holmes david.holmes at oracle.com
Tue May 14 13:40:31 UTC 2019 

On 14/05/2019 10:27 pm, coleen.phillimore at oracle.com wrote:
> 
> 
> On 5/14/19 8:25 AM, David Holmes wrote:
>> On 14/05/2019 9:59 pm, coleen.phillimore at oracle.com wrote:
>>> On 5/14/19 2:35 AM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> On 14/05/2019 7:39 am, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> The functions SignatureStream::as_symbol and as_symbol_or_null() 
>>>>> don't need the TRAPS argument for the same reason, neither do 
>>>>> java_lang_Class::as_symbol, as_symbol_or_null or as_signature. This 
>>>>> propagated not needing TRAPS to a couple verifier functions as 
>>>>> well. These parameters and arguments to the calls are all removed. 
>>>>> This makes (S) into (M) I guess, but it's simple.
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/2019/8223657.02.incr/webrev/index.html 
>>>>>
>>>>
>>>> This change is incorrect:
>>>>
>>>> --- old/src/hotspot/share/classfile/stackMapFrame.cpp 2019-05-13 
>>>> 16:30:35.992633036 -0400
>>>> +++ new/src/hotspot/share/classfile/stackMapFrame.cpp 2019-05-13 
>>>> 16:30:35.512633016 -0400
>>>> @@ -92,8 +92,7 @@
>>>>    // local num may be greater than size of parameters because 
>>>> long/double occupies two slots
>>>>    while(!ss.at_return_type()) {
>>>>      init_local_num += _verifier->change_sig_to_verificationType(
>>>> -      &ss, &_locals[init_local_num],
>>>> -      CHECK_VERIFY_(verifier(), VerificationType::bogus_type()));
>>>> +      &ss, &_locals[init_local_num]);
>>>>      ss.next();
>>>>    }
>>>>
>>>> This gets rid of the exception checking _and_ the verifier checking. 
>>>> You need to add back:
>>>>
>>>>    while(!ss.at_return_type()) {
>>>>      init_local_num += _verifier->change_sig_to_verificationType(
>>>>        &ss, &_locals[init_local_num]);
>>>> +    if (_verifier->has_error()) return VerificationType::bogus_type();
>>>>      ss.next();
>>>>    }
>>>>
>>>
>>> I don't see it.   change_sig_to_verificationType doesn't have the 
>>> side effect of setting _exception_type so has_error() will always be 
>>> false here.
>>
>> That may be true in this case - I didn't verify that the verification 
>> was actually needed. The point is that the old code checks if there is 
>> a verify error and your new code does not.
> 
> Yes, the new code does not need to.  The old code only needed it because 
> SymbolTable functions passed a TRAPS argument.

Sorry but I don't see any connection between the two. The CHECK_VERIFY 
macros check for both a Java exception pending (like CHECK) and checks 
if there is a verifier error. Two separate and unrelated actions AFAICS

But as you note change_sig_to_verificationType doesn't do anything that 
would cause has_error() to be true, so it could have used plain CHECK 
instead of CHECK_VERIFY. The same for create_temporary_symbol. So it 
seems the VERIFY part was unnecessary in the first place, hence your 
change is okay afterall.

David
-----

> 
> Coleen
> 
>>
>> David
>>
>>>
>>> Coleen
>>>> Similarly in src/hotspot/share/classfile/verifier.cpp
>>>>
>>>> @@ -647,7 +647,7 @@
>>>> -    int n = change_sig_to_verificationType(&sig_stream, sig_type, 
>>>> CHECK_VERIFY(this));
>>>> +    int n = change_sig_to_verificationType(&sig_stream, sig_type);
>>>>
>>>> should be:
>>>>
>>>> -    int n = change_sig_to_verificationType(&sig_stream, sig_type, 
>>>> CHECK_VERIFY(this));
>>>> +    int n = change_sig_to_verificationType(&sig_stream, sig_type);
>>>> +    if (this->has_error()) return;
>>>>
>>>> ditto for the all the other CHECK_VERIFY changes in that file.
>>>>
>>>> Everything else appears fine.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> And full:
>>>>>
>>>>> open webrev at 
>>>>> http://cr.openjdk.java.net/~coleenp/2019/8223657.02/webrev
>>>>>
>>>>> This was tested with hs-tier1-3 on Oracle platforms. Non-oracle 
>>>>> platform changes are trivial.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 5/13/19 8:24 AM, David Holmes wrote:
>>>>>> On 13/05/2019 10:14 pm, coleen.phillimore at oracle.com wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/13/19 7:55 AM, coleen.phillimore at oracle.com wrote:
>>>>>>>>
>>>>>>>> Thanks David for reviewing.
>>>>>>>>
>>>>>>>> On 5/12/19 6:59 PM, David Holmes wrote:
>>>>>>>>> Hi Coleen,
>>>>>>>>>
>>>>>>>>> Generally this all looks fine. A couple of follow ups:
>>>>>>>>>
>>>>>>>>> src/hotspot/share/classfile/classListParser.cpp
>>>>>>>>>
>>>>>>>>> 329   TempNewSymbol class_name_symbol = 
>>>>>>>>> SymbolTable::new_symbol(_class_name);
>>>>>>>>>  330   guarantee(!HAS_PENDING_EXCEPTION, "Exception creating a 
>>>>>>>>> symbol.");
>>>>>>>>
>>>>>>>> Yup, I should have removed that.
>>>>>>>>>
>>>>>>>>> This guarantee seems wrong even before this change, but if no 
>>>>>>>>> exceptions can come from new_symbol then it serves no purpose now.
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> src/hotspot/share/classfile/javaClasses.cpp
>>>>>>>>>
>>>>>>>>> 572 Symbol* java_lang_String::as_symbol(oop java_string, TRAPS) {
>>>>>>>>>
>>>>>>>>> TRAPS is unused now. Further there seems to be no reason to 
>>>>>>>>> have both java_lang_String::as_symbol and 
>>>>>>>>> java_lang_String::as_symbol_or_null as the only distinction 
>>>>>>>>> appears to be that the former was expected to post an exception 
>>>>>>>>> if it would return NULL. But apparently it is impossible to 
>>>>>>>>> return NULL as we should have aborted the VM on allocation 
>>>>>>>>> failure - right?
>>>>>>>>
>>>>>>>> Yes, exactly.  I can remove as_symbol_or_null as part of this 
>>>>>>>> change. Let me see if it increases the change too much, if so, 
>>>>>>>> I'll file a follow-up RFE.  Thanks for finding these!
>>>>>>>
>>>>>>> Actually looking again, as_symbol_or_null is a probe but 
>>>>>>> as_symbol creates a new_symbol.  So there is a distinction. But 
>>>>>>> the as_symbol function doesn't require the TRAPS argument 
>>>>>>> anymore.  There's also the same in SignatureStream.
>>>>>>
>>>>>> Ah - sorry - yes read that too quick. I'm glad you've renamed the 
>>>>>> lookup functions to new_symbol to make it clearer.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Coleen
>>>>>>>
>>>>>>>>
>>>>>>>> Coleen
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>> On 11/05/2019 1:15 am, coleen.phillimore at oracle.com wrote:
>>>>>>>>>> Summary: also made lookup and lookup_only functions private to 
>>>>>>>>>> SymbolTable. External callers use new_symbol or probe.
>>>>>>>>>>
>>>>>>>>>> Ran hs-tier1-3.
>>>>>>>>>>
>>>>>>>>>> open webrev at 
>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8223657.01/webrev
>>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8223657
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Coleen
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>

More information about the hotspot-runtime-dev mailing list