Request for reviews (S): 6820514: meet not symmetric failure in ctw

[Tom Rodriguez]

This code seems like it should do exactly the same thing as the code  
which handles the normal loaded case comparing against a loaded  
j.l.O.  Maybe you could take a copy of that code and plug in the types  
we know we have in xmeet_unloaded, and simplify the crap out of it  
based on loaded is j.l.O and unloaded is some unloaded thing and see  
what answer that produces.  The other thing I noticed about this code  
is that it completely ignores the offset for xmeet_unloaded which may  
be ok, though it seems odd.

xmeet_unloaded may be designed to produce conservative but correct  
answers so maybe the answer it produces is allowed to skew from what  
you'd expect.  Maybe John has some history about how this should  
behave.  I'd be tempted to compare a simplified version of a normal  
meet with a version of xmeet_unloaded that fixes just TopPtr test and  
see where it diverges.  The core logic is well tested I think and  
applying it to the Object vs. unloaded case should lead to a correct  
answer.

tom

> Yes, you are right, the code for AnyNull case does not return
> the original "unloaded" type. It returns the meet type with
> meet PTR, offset and instance_id, "false" klass exactness and
> NULL for const_oop. The exactness and const_oop could be wrong.

>
>
> At the beginning I wanted just fix the "original" original source
> at the switch end and remove one which was added later.
> In this case it matches the comment and fix the bug without
> calling meet methods:
>
>       assert(loaded->ptr() != TypePtr::Null, "insanity check");
>       //
>       if(      loaded->ptr() == TypePtr::TopPTR ) { return unloaded; }
> -      else if (loaded->ptr() == TypePtr::AnyNull) { return  
> TypeInstPtr::make( ptr, unloaded->klass() ); }
>       else if (loaded->ptr() == TypePtr::BotPTR ) { return  
> TypeInstPtr::BOTTOM; }
>       else if (loaded->ptr() == TypePtr::Constant || loaded->ptr()  
> == TypePtr::NotNull) {
>         if (unloaded->ptr() == TypePtr::BotPTR  ) { return  
> TypeInstPtr::BOTTOM;  }
>         else                                      { return  
> TypeInstPtr::NOTNULL; }
>       }
> -      else if( unloaded->ptr() == TypePtr::TopPTR )  { return  
> unloaded; }
> -
> +      assert(loaded->ptr() == TypePtr::AnyNull, "insanity check");
> +      if( unloaded->ptr() != TypePtr::TopPTR )  { return unloaded; }
>       return unloaded->cast_to_ptr_type(TypePtr::AnyNull)- 
> >is_instptr();
>     }
>
> But I am not sure it is correct.
>
> Note, the original check if( unloaded->ptr() == TypePtr::TopPTR )
> was wrong, it should be "!=". Because of this bug, I think,
> the additional code (with the explicit AnyNull check) was added
> to fix it.
>
> Also I can't move meet_ptr() since it is also used below this code.
>
> Thanks,
> Vladimir
>
> Tom Rodriguez wrote:
>> On Mar 23, 2009, at 2:14 PM, Vladimir Kozlov wrote:
>>> No, it is not true. The original comment's line is still true
>> Sorry, my original comment about what it changed it to was wrong.   
>> I'd rewritten this several times and forgot to correct that.  The  
>> original comment was wrong and it's still wrong for your change.
>>>
>>>
>>> >       //  AnyNull |  U-AN     
>>> |................Unloaded......................|
>>>
>>> meet_ptr() maps almost everything to the original PRT:
>>>
>>> >   //              TopPTR,    AnyNull,   Constant, Null,    
>>> NotNull, BotPTR,
>>> >   { /* AnyNull */ AnyNull,   AnyNull,   Constant, BotPTR,  
>>> NotNull, BotPTR,},
>> Which means that the comment should be:
>>>>     //  Object  |   TOP    |   AnyNull | Constant |   NotNull |   
>>>> BOTTOM   |
>>>>     //  AnyNull |....... U-AN .........|  U-Con   |    U-NN   |    
>>>> U-Bot   |
>> The AnyNull case never returns unloaded unchanged, except for the  
>> AN/AN case, and having the word unloaded in the table implies that  
>> it simply returns it.
>> My comment about the Constant line can be ignored too since it  
>> ignores the results of the meet_ptr.  I might like the code better  
>> is the meet_ptr call was moved to it's uses since it's commonly  
>> completely ignored.
>> tom
>>>
>>>
>>> except TOP which corresponds to the special case in the code table  
>>> and NULL which
>>> is not in the table.
>>>
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> There's a nice comment above this code and it would be nice if  
>>>> the table and code agreed in both order and results.
>>>>     // Meet unloaded class with java/lang/Object
>>>>     //
>>>>     // Meet
>>>>     //          |                     Unloaded Class
>>>>     //  Object  |   TOP    |   AnyNull | Constant |   NotNull |   
>>>> BOTTOM   |
>>>>     //   
>>>> ===================================================================
>>>>     //   TOP     
>>>> | ..........................Unloaded......................|
>>>>     //  AnyNull |  U-AN     
>>>> |................Unloaded......................|
>>>>     // Constant | ... O-NN .................................. |    
>>>> O-BOT   |
>>>>     //  NotNull | ... O-NN .................................. |    
>>>> O-BOT   |
>>>>     //  BOTTOM  | ........................Object- 
>>>> BOTTOM ..................|
>>>> Your changes appear to make this line:
>>>>     //  AnyNull |  U-AN     
>>>> |................Unloaded......................|
>>>> become
>>>>     //  AnyNull |...........................  U- 
>>>> AN  ......................|
>>>> The -AN part isn't quite true since we're returning the results  
>>>> of meet_ptr which uses the table below.  AnyNull meeting NotNull  
>>>> returns NotNull.  The Constant line in the comments seems to be  
>>>> wrong too.
>>>> const TypePtr::PTR TypePtr::ptr_meet[TypePtr::lastPTR] 
>>>> [TypePtr::lastPTR] = {
>>>> //              TopPTR,    AnyNull,   Constant, Null,   NotNull,  
>>>> BotPTR,
>>>> { /* Top     */ TopPTR,    AnyNull,   Constant, Null,   NotNull,  
>>>> BotPTR,},
>>>> { /* AnyNull */ AnyNull,   AnyNull,   Constant, BotPTR, NotNull,  
>>>> BotPTR,},
>>>> { /* Constant*/ Constant,  Constant,  Constant, BotPTR, NotNull,  
>>>> BotPTR,},
>>>> { /* Null    */ Null,      BotPTR,    BotPTR,   Null,   BotPTR,   
>>>> BotPTR,},
>>>> { /* NotNull */ NotNull,   NotNull,   NotNull,  BotPTR, NotNull,  
>>>> BotPTR,},
>>>> { /* BotPTR  */ BotPTR,    BotPTR,    BotPTR,   BotPTR, BotPTR,   
>>>> BotPTR,}
>>>> };
>>>> The code itself looks right to me after much rereading of the  
>>>> meet logic.
>>>> tom
>>>> On Mar 20, 2009, at 11:31 PM, Vladimir Kozlov wrote:
>>>>>
>>>>> http://cr.openjdk.java.net/~kvn/6820514/webrev.00
>>>>>
>>>>> Fixed 6820514: meet not symmetric failure in ctw
>>>>>
>>>>> Problem:
>>>>> Missing instance_id meet for the case j.l.Object:NotNull meets
>>>>> unloaded instance klass.
>>>>>
>>>>> Solution:
>>>>> Add missing code.
>>>>>
>>>>> Reviewed by:
>>>>>
>>>>> Fix verified (y/n): y, ctw test from bug
>>>>>
>>>>> Other testing:
>>>>> JPRT
>>>>>