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

[Ioi Lam]

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.

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?
>
> Thanks,
> StefanK
>>
>> thanks,
>> Calvin
>>
>>>
>>> Thanks,
>>> StefanK
>>>
>>>>
>>>> Tests:
>>>>     JPRT (almost done)
>>>>     Will do more perf testing after JPRT
>>>>
>>>> thanks,
>>>> Calvin
>>>
>>
>