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

[Calvin Cheung]

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

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

thanks,
Calvin

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