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

[David Holmes]

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.

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