review (S) for 6951190: assert(!klass_is_exact(), "only non-exact klass") while building JDK

[Vladimir Kozlov]

Looks good.

Vladimir

Tom Rodriguez wrote:
> I've corrected all the copies and updated the webrev.
> 
> tom
> 
> On May 10, 2010, at 1:36 PM, Vladimir Kozlov wrote:
> 
>> I agree to change them.
>>
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> Do you mind if I change them to TypeOopPtr::make_from_klass?  I'd added an assert to confirm that they produce the same thing when I was testing this.
>>> tom
>>> On May 10, 2010, at 12:44 PM, Vladimir Kozlov wrote:
>>>> It could be just historical.
>>>> I looked on the original code and it starts with TypeOopPtr::make(TypePtr::BotPTR, klass, NULL, 0) in inline_string_compareTo().
>>>> Then it was changed to TypeInstPtr::make(), added false for xk. And later it was copy/past into other intrinsic methods.
>>>>
>>>> I agree that we can use TypeOopPtr::make(klass) here.
>>>>
>>>> Your changes with CheckCastPPNode are good.
>>>>
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> I've updated it to just use a CheckCastPPNode as the checkcast logic would.  Do you know why all the logic for building the String type passes false for klass_is_exact?
>>>>> const TypeInstPtr* string_type =
>>>>>   TypeInstPtr::make(TypePtr::BotPTR, klass, false, NULL, 0);
>>>>> TypeInstPtr::make corrects it to true for final classes but it just seems weird.  gen_checkcast would simply use TypeOopPtr::make(klass) and let it take care of supplying the rest of the arguments.
>>>>> tom
>>>>> On May 10, 2010, at 11:58 AM, Tom Rodriguez wrote:
>>>>>> That's still not right.  I think my brain hasn't fully kicked in yet.  I'll just go with a CheckCastPP as you've suggested.
>>>>>>
>>>>>> tom
>>>>>>
>>>>>> On May 10, 2010, at 11:51 AM, Tom Rodriguez wrote:
>>>>>>
>>>>>>> On May 10, 2010, at 11:25 AM, Vladimir Kozlov wrote:
>>>>>>>
>>>>>>>> No. It will not handle situation when statically we don't know if argument is NULL.
>>>>>>>> And it will no work for NULL stopped() situation since 'failure' node will be NULL
>>>>>>>> so merge region will not have valid paths.
>>>>>>> You're right that I'm not handling the argument == null case properly.
>>>>>>>
>>>>>>>> Can we just use existing code and add CheckCastPPNode cast after gen_instanceof() ?
>>>>>>> I was trying to get rid of the use of gen_instanceof since it generates quite a rats nest of tests that can require split_if to simplify.  What about this:
>>>>>>>
>>>>>>> diff -r 359375cb7de6 src/share/vm/opto/library_call.cpp                                                                                               --- a/src/share/vm/opto/library_call.cpp        Fri May 07 15:13:00 2010 -0700                                                                       +++ b/src/share/vm/opto/library_call.cpp        Mon May 10 11:50:19 2010 -0700                                                                       @@ -945,16 +945,12 @@ bool LibraryCallKit::inline_string_equal
>>>>>>> ciInstanceKlass* klass = env()->String_klass();                                                                                                   if (!stopped()) {                                                                                                                                 -    Node* inst = gen_instanceof(argument, makecon(TypeKlassPtr::make(klass)));                                                                       -    Node* cmp  = _gvn.transform(new (C, 3) CmpINode(inst, intcon(1)));                                                                               -    Node* bol  = _gvn.transform(new (C, 2) BoolNode(cmp, BoolTest::ne));                                                                             -                                                                                                                                                     -    Node* inst_false = generate_guard(bol, NULL, PROB_MIN);                              
                                                           -    //instanceOf == true, fallthrough                                                   
>>                                                           -                                                                                                                                                     -    if (inst_false != NULL) {                                                                                                                       -      phi->init_req(3, intcon(0));                                                                                                                   -      region->init_req(3, inst_false);                                                                                                               +    Node* inst_false = NULL;                                                                                                                         +    argument = gen_checkcast(argument, makecon(TypeKlassPtr::make(klass)), &inst_false);                                                             +    phi->init_req(3, intcon(0
)
>> );                                                                                                                     +    region->init_req(3, inst_false ? inst_false : top());                                                                                           +    if (argument == null()) {                                                                                                                       +      set_control(top());                                                                                                                              }                                                                                                                                                }
>>>>>>> tom
>>>>>>>
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>> The !stopped() logic should handle that.
>>>>>>>>>
>>>>>>>>> tom
>>>>>>>>>
>>>>>>>>> On May 10, 2010, at 11:01 AM, Vladimir Kozlov wrote:
>>>>>>>>>
>>>>>>>>>> Tom,
>>>>>>>>>>
>>>>>>>>>> You need to add check for argument==null in inline_string_equal() after gen_checkcast() since it is false case.
>>>>>>>>>>
>>>>>>>>>> Vladimir
>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>> http://cr.openjdk.java.net/~never/6951190
>>>>>>>>> 6951190: assert(!klass_is_exact(),"only non-exact klass") while building JDK
>>>>>>>>> Reviewed-by:
>>>>>>>>> This looks to be a long standing issue with the String.equals
>>>>>>>>> intrinsic.  The code checks that the argument is a String but it never
>>>>>>>>> actually casts it to be a String.  Later on when we're processing
>>>>>>>>> types for the AddP for the String.count field we're looking for a
>>>>>>>>> field at +20 in Object which of course doesn't exist so we hit that
>>>>>>>>> assert.  The fix is to perform a real checkcast which includes to
>>>>>>>>> required cast to String.  I also added an assert to check the contract
>>>>>>>>> with gen_checkcast that it always returns non-null failure_control.
>>>>>>>>> Tested by hand with failing JDK build.
>