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

[David Holmes]

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();
    }

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