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

[Tom Rodriguez]

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.