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

[Vladimir Kozlov]

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