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

Tue May 14 12:27:17 UTC 2019

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.

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