review (S) for 7032162: assert(flat != TypePtr::BOTTOM) failed: cannot alias-analyze an untyped ptr

[Tom Rodriguez]

The push for this failed just before my vacation and I didn't have time to investigate then.  It turns out that the klass() call needs to guard against the null object so it now looks like this:

diff -r 149bb459be66 src/share/vm/ci/ciObject.cpp                                                                                            
--- a/src/share/vm/ci/ciObject.cpp                                                                                                          
+++ b/src/share/vm/ci/ciObject.cpp                                                                                                          
@@ -194,6 +194,16 @@
 // ciObject::should_be_constant()                                                                                                          
 bool ciObject::should_be_constant() {                                                                                                      
   if (ScavengeRootsInCode >= 2)  return true;  // force everybody to be a constant                                                          
+  if (!JavaObjectsInPerm && !is_null_object()) {                                                                                            
+    // We want Strings and Classes to be embeddable by default since                                                                        
+    // they used to be in the perm world.  Not all Strings used to be                                                                      
+    // embeddable but there's no easy way to distinguish the interned                                                                      
+    // from the regulars ones so just treat them all that way.                                                                              
+    ciEnv* env = CURRENT_ENV;                                                                                                              
+    if (klass() == env->String_klass() || klass() == env->Class_klass()) {                                                                  
+      return true;                                                                                                                          
+    }                                                                                                                                      
+  }                                                                                                                                        
   return handle() == NULL || !is_scavengable();                                                                                            
 }

tom

On Apr 6, 2011, at 3:52 PM, Tom Rodriguez wrote:

> Thanks!
> 
> tom
> 
> On Apr 6, 2011, at 2:07 PM, Vladimir Kozlov wrote:
> 
>> This looks better.
>> 
>> Thanks,
>> Vladimir
>> 
>> Tom Rodriguez wrote:
>>> On Apr 6, 2011, at 1:49 PM, Vladimir Kozlov wrote:
>>>> You replaced 'require_constant' with 'true' to the call make_from_constant() so I was wondering if it may change behavior. Specifically it may produce NULL now
>>>> if oop_constant->can_be_constant() is 'false':
>>>> 
>>>>    if (require_constant) {
>>>>      if (!o->can_be_constant())  return NULL;
>>> In the !JavaObjectInPerm world Strings should always return true for can_be_constant.  Actually I think it would be better to fix it by changing ciObject::should_be_constant instead of just patching that use. I also included Classes in the test for consistency with the old behaviour.  So I've removed the changes in parse3.cpp and done this instead.
>>> diff -r 8e77e1f26188 src/share/vm/ci/ciObject.cpp                                                                                                     --- a/src/share/vm/ci/ciObject.cpp                                                                                                                   +++ b/src/share/vm/ci/ciObject.cpp                                                                                                                   @@ -194,6 +194,16 @@ bool ciObject::can_be_constant() {
>>> // ciObject::should_be_constant()                                                                                                                    bool ciObject::should_be_constant() {                                                                                                                  if (ScavengeRootsInCode >= 2)  return true;  // force everybody to be a constant                                                                   +  if (!JavaObjectsInPerm) {                                                                                                                         +    // We want Strings and Classes to be embeddable by default since                                                                                 +    // they used to be in the perm world.  Not all Strings used to be                                                                               +    // embeddable but there's no easy way to distinguish the interned                                                                               +    // from the regulars ones so just treat them all that way.                                                                                       +    ciEnv* env = CURRENT_ENV;                                                                                                                       +    if (klass() == env->String_klass() || klass() == env->Class_klass()) {                                                                           +      return true;                                                                                                                                   +    }                                                                                                                                               +  }                                                                                                                                                    return handle() == NULL || !is_scavengable();                                                                                                      }
>>> tom
>>>> Vladimir
>>>> 
>>>> Tom Rodriguez wrote:
>>>>> http://cr.openjdk.java.net/~never/7032162
>>>>> 7032162: assert(flat != TypePtr::BOTTOM) failed: cannot alias-analyze an untyped ptr
>>>>> Reviewed-by:
>>>>> This appears to be an existing problem with OptimizeStringConcat where
>>>>> missing control can allow some loads to be split up to where their
>>>>> base is NULL which causes addresses that can't be analyzed.  The fix
>>>>> is to use the appropriate control.  If it's truly unneeded it will be
>>>>> optimized away.  The bug was exposed by the String not in perm changes
>>>>> because we were no longer treating some constant strings as embeddable
>>>>> constants.  This is fixed by handling them specially in push_constant.
>>>>> Tested with failing test from report and full CTW with string opts.
>