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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Tue May 14 14:53:37 UTC 2019 


On 5/14/19 9:40 AM, David Holmes wrote:
> 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.

Great, thank you.  That's what I found when I read the code.

Coleen

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