Request for prereviews (S): 7047954: VM crashes with assert(is_Mem()) failed: invalid node class

[Tom Rodriguez]

On May 26, 2011, at 4:34 PM, Vladimir Kozlov wrote:

> Tom Rodriguez wrote:
>> On May 26, 2011, at 3:27 PM, Vladimir Kozlov wrote:
>>> http://cr.openjdk.java.net/~kvn/7047954/webrev
>>> 
>>> Fixed 7047954: VM crashes with assert(is_Mem()) failed: invalid node class
>>> 
>>> It was exposed by is_scavengable change. Final static array pointer was replaced with ConP. The main problem is a constant array pointer does not alias with other array pointers (see Compile::flatten_alias_type()).
>> Do you mean this logic:
>>  // Array pointers need some flattening
>>  const TypeAryPtr *ta = tj->isa_aryptr();
>>  if( ta && is_known_inst ) {
>>    if ( offset != Type::OffsetBot &&
>>         offset > arrayOopDesc::length_offset_in_bytes() ) {
>>      offset = Type::OffsetBot; // Flatten constant access into array body only
>>      tj = ta = TypeAryPtr::make(ptr, ta->ary(), ta->klass(), true, offset, ta->instance_id());
>>    }
>>  }
> 
> No. This logic which does not convert constant array pointer to bottom array pointer (keep them separate):
> 
>    // Make sure the Bottom and NotNull variants alias the same.
>    // Also, make sure exact and non-exact variants alias the same.
>    if( ptr == TypePtr::NotNull || ta->klass_is_exact() ) {
>      if (ta->const_oop()) {
>        tj = ta = TypeAryPtr::make(TypePtr::Constant,ta->const_oop(),ta->ary(),ta->klass(),false,offset);
>      } else {
>        tj = ta = TypeAryPtr::make(TypePtr::BotPTR,ta->ary(),ta->klass(),false,offset);
>      }
>    }

Ok.  Why are arrays handled differently than instances?  Those don't seem to do that.

> 
>> That seems odd.  It was added as part of 6723160: Nightly failure: Error: meet not symmetric.  Is instance_id set for constant objects?
>>> As result the address of a store has different memory slice (idx=4) than calculated memory slice affected by the store (idx=5):
>> It seems like there should be an assert that checks that these agree?
> 
> The check is in MemNode::adr_type() works fine but the problem is not there but in GraphKit::store_to_memory() where created store attached to mergemem:
> 
>  set_memory(st, adr_idx);
> 
> adr_idx is calculated from passed (general) adr_type and not from adr->bottom_type().

It seems like those should be required to agree though I suspect sometimes the passed in value is raw while adr->bottom_type is something else.

tom

> 
> Vladimir
> 
>>> 75     StoreI  ===  61  46  73  20  [[ 95  81 ]]  @int[int:>=0]:exact+any *, idx=5;  Memory: @int[int:1000000]<ciTypeArray length=1000000 type=<ciTypeArrayKlass name=[I ident=686 PERM address=0x8482d20> ident=696 address=0x8527328>+any *, idx=4; !jvms: ReadAfterWrite::main @ bci:16
>>> 
>>> It happened because when a store to array is created the type of affected memory is calculated using general array type TypeAryPtr::get_array_body_type(elem_type).
>>> 
>>> So we either should allow a constant array pointer alias with other array pointers or check if an array ptr is constant and use its type instead of general one. I used second approach and fixed all places where get_array_body_type() was used. But I need your opinion on this. Based on previous Tom's comment we should not convert array pointers to constant (can we have arrays in Perm?).
>> Yes we can.  Previously the char[] of constant Strings were like that though we only read from those and only recently started treating their fields as constants and we also recently moved them out of perm.  I don't think we embed any other constant perm arrays in compiled code though.
>>> But then there is invoke dynamic case and I am not sure that we will not get constant arrays from it.
>> Going forward we're going to be seeing a lot more constant objects in generated code since it's easy to embed them now.  292 in particular is going to want this so we can fold away as much goo as possible.
>> tom
>>> Thanks,
>>> Vladimir
>>>