RFR (2nd round) 8071627: Code refactoring to override == operator of Symbol*

[Calvin Cheung]

On 4/15/2015 11:13 PM, Ioi Lam wrote:
> On 4/15/15 11:02 PM, Stefan Karlsson wrote:
>> On 2015-04-16 01:25, Calvin Cheung wrote:
>>> Hi Stefan,
>>>
>>> Thanks for your review.
>>>
>>> On 4/15/2015 2:50 PM, Stefan Karlsson wrote:
>>>> Hi Calvin,
>>>>
>>>> On 2015-04-15 21:56, Calvin Cheung wrote:
>>>>> Please review this second version of the fix.
>>>>>
>>>>> This version has 2 new functions (equals() and not_equals()) in 
>>>>> the Symbol class.
>>>>> It replaces the Symbol* == and != comparisons with those 2 
>>>>> function calls.
>>>>>
>>>>> Pro:
>>>>>
>>>>> It has a much smaller changeset than the first version.
>>>>>
>>>>> Con:
>>>>>
>>>>> Someone may by mistake introduce a new line of (Symbol* == Symbol*).
>>>>>
>>>>> We will mitigate this by enhancing our internal static analysis 
>>>>> tool to flag the
>>>>> above code in the future.
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8071627
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/
>>>>
>>>> This is a much less intrusive change than the previous patch. Thanks.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/classFileParser.cpp.patch 
>>>>
>>>>
>>>> Is there a reason why you added not_equals:
>>>> -              if (name != vmSymbols::object_initializer_name()) {
>>>> +              if 
>>>> (name->not_equals(vmSymbols::object_initializer_name())) {
>>>>
>>>> instead of just:
>>>> +              if 
>>>> (!name->equals(vmSymbols::object_initializer_name())) {
>>> We think that it's clearer to have not_equals() than using the ! as 
>>> in the above.
>>
>> OK. IMHO, negating boolean expressions with ! is a common construct 
>> so I'm not sure not_equals is clearer.
>>
>>>>
>>>> http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/systemDictionary.cpp.udiff.html 
>>>>
>>>> !     assert(parsed_name->not_equals(NULL), "Sanity");
>>>>
>>>>
>>>> You use symbol == NULL but not symbol != NULL, which seems 
>>>> inconsistent to me.
>>> I'm not sure I understand this comment.
>>> Are you referring to the following section of the udiff?
>>> *** 1104,1115 ****
>>> --- 1104,1115 ----
>>>       Exceptions::_throw_msg(THREAD_AND_LOCATION,
>>>         vmSymbols::java_lang_SecurityException(), message);
>>>     }
>>>
>>>     if (!HAS_PENDING_EXCEPTION) {
>>> !     assert(parsed_name != NULL, "Sanity");
>>> !     assert(class_name == NULL || class_name == parsed_name, "name 
>>> mismatch");
>>> !     assert(parsed_name->not_equals(NULL), "Sanity");
>>> !     assert(class_name == NULL || class_name->equals(parsed_name), 
>>> "name mismatch");
>>>       // Verification prevents us from creating names with dots in 
>>> them, this
>>>       // asserts that that's the case.
>>>       assert(is_internal_format(parsed_name),
>>>              "external class name format used internally");
>>>
>>> I don't see anything incorrect there.
>>
>> I'm not talking about being correct or incorrect. I would have 
>> expected to see either:
>>
>> 1) Use pointer (in)equality when checking for NULL:
>> class_name == NULL
>> parsed_name != NULL
>>
>> or
>>
>> 2) strictly use the (not_)equals function when checking for NULL:
>> class_name->equals(NULL)
>> parsed_name->not_equals(NULL)
>> or even:
>> class_name->equals(NULL)
>> !parsed_name->equals(NULL))
>>
>> but not what you have today, where you mix the two ways to NULL check
>> class_name == NULL
>> parsed_name->not_equals(NULL)
>>
>
> I think it's better to use (symbol == NULL) everywhere. Calling 
> symbol->equals(NULL) while symbol itself is NULL is confusing -- it 
> raises questions like "will this crash in debug mode", so we'd better 
> avoid it.
I've fixed all those NULL comparison back to (symbol == NULL) or (symbol 
!= NULL).

>
> In our static analysis tool, we need to flags checks for (symbol1 == 
> symbol2), but should allow (symbol == NULL).
>
> - Ioi
>
>>
>>>> http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/oops/symbol.hpp.udiff.html 
>>>>
>>>>
>>>> *+   inline bool equals(const Symbol* other) const {*
>>>> *+     if (this && other) {
>>>> *
>>>> First, pointers should be checked null checked with == or !=. See:
>>>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-Miscellaneous 
>>>>
>>>
>>> I will fix it.
>>>
>>>>
>>>> Second, I recall some discussion that null checking the 'this' 
>>>> pointer is undefined behavior. Though, we do it in other places so 
>>>> this isn't worse, I think.
>>>>
>>>> Did you use your previous patch to find all Symbol* compares?
>>>
>>> Yes. Essentially comment out the bodies of the == and != operators 
>>> in SymbolRef.
>>> Rebuilding hotspot resulting a lot of "undefined reference to 
>>> SymbolRef::operator==" link errors.
>>> Then go through those error and change a == b to a->equals(b) and a 
>>> != b to a->not_equals(b).
>>> Obviously change SymbolRef back to Symbol*.
>>
>> Great.
>>
>> One more thing:
>> http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/oops/symbol.hpp.udiff.html 
>>
>>
>> *+       if (this->_identity_hash != other->_identity_hash) {*
>> *+         return false;*
>> *+       }*
>>
>>
>> Is this correct? The current code sets up _identity_hash with 
>> os::random, so it seems like to Symbols with equal contents would 
>> most likely going to get different _identity_hash codes. Or will this 
>> be changed later?
I removed that comparison for now.

Updated webrev:
     http://cr.openjdk.java.net/~ccheung/8071627/webrev.02/

Files modified since last time:
src/share/vm/classfile/classFileParser.cpp
src/share/vm/classfile/systemDictionary.cpp
src/share/vm/oops/symbol.hpp
src/share/vm/prims/methodHandles.cpp

thanks,
Calvin
>>
>> Thanks,
>> StefanK
>>>
>>> thanks,
>>> Calvin
>>>
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>>>
>>>>> Tests:
>>>>>     JPRT (almost done)
>>>>>     Will do more perf testing after JPRT
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>
>>>
>>
>