6894807: problem with interfaces in C2 type system

[Vladimir Kozlov]

John, Tom

I need your help with this.

The original implementation of TypeOopPtr::filter() was done by John
and pushed by Ross for:

6467870: Fixes to monotonically narrow or widen types during igvn

Ross Knippel wrote:
 > This change is from John Rose (I'm just the medium.)
 > It's a piece extracted from his current work.

It did not have the bug's problem since an interface could not be exact.

Then Tom added the klass part for:

6788347: C2Compiler crash 6u7

Which causes the problem since interface klass could be exact (constant,
as in the bug's case) and casting j.l.O klass to interface's ptr
returns incorrectly exact j.l.O klass:

     return ktkp->cast_to_ptr_type(ftkp->ptr());

The fix is either simple return original j.l.o klass:

     return kills;

or cast to ptrs meet:

     return ktkp->cast_to_ptr_type(ktkp->meet_ptr(ftkp->ptr()));

Both of them fixed the problem. But I am not sure since in
both cases we loose the precision of interface klass which
allows more ideal optimizations (for example, for CmpP).

Thanks,
Vladimir

Vladimir Kozlov wrote:
> I am investigating 6894807: No ClassCastException for HashAttributeSet 
> constructors if run with -Xcomp
> 
> Because of Escape Analysis is on by default all constructors are inlined
> in the test case and object scalar replaced. But due to, I think,
> the problem in C2 type system the result is incorrect.
> 
> We should check if interface is exact before replacing
> it with j.l.O in TypeOopPtr::filter() for the case
> (exact interface klass)->filter(j.l.O klass).
> 
> Vladimir
> 
> ----------------------------------------------------------------
> Test code expects ClassCastException exception:
> 
> import javax.print.attribute.Attribute;
> import javax.print.attribute.AttributeSet;
> import javax.print.attribute.DocAttribute;
> import javax.print.attribute.HashAttributeSet;
> import javax.print.attribute.standard.JobState;
> 
>         try {
>             new MyHashAttributeSet(JobState.CANCELED, DocAttribute.class);
>             System.out.println("No ClassCastException in t2!");
>         } catch (ClassCastException _) {
>         }
> 
> EA forces to inline all constructors for MyHashAttributeSet:
> 
> class MyHashAttributeSet extends HashAttributeSet{
>     MyHashAttributeSet(Attribute attribute,
>                            Class interfaceName){
>         super(attribute, interfaceName);
>     }
> 
> then
> 
>     protected HashAttributeSet(Attribute attribute, Class<?> 
> interfaceName) {
>         if (interfaceName == null) {
>             throw new NullPointerException("null interface");
>         }
>         myInterface = interfaceName;
>         add (attribute);
>     }
> 
> add() is inlined also:
> 
>     public boolean add(Attribute attribute) {
>         Object oldAttribute =
>             attrMap.put(attribute.getCategory(),
>                         AttributeSetUtilities.
>                         verifyAttributeValue(attribute, myInterface));
>         return (!attribute.equals(oldAttribute));
>     }
> 
> and verifyAttributeValue(attribute, myInterface) is inlined as well:
> 
>     public static Attribute
>         verifyAttributeValue(Object object, Class<?> interfaceName) {
> 
>         if (object == null) {
>             throw new NullPointerException();
>         }
>         else if (interfaceName.isInstance (object)) {
>             return (Attribute) object;
>         } else {
>             throw new ClassCastException();
>         }
>     }
> 
> object is JobState.CANCELED which is
> 
>     public static final JobState CANCELED = new JobState (7);
> 
> and interfaceName is DocAttribute.class where
> 
> public interface DocAttribute extends Attribute {}
> 
> ----------------------------------------------------------------
> So we end up with
> 
> (DocAttribute.class).isInstance(JobState.CANCELED)
> 
> We have intrinsic for isInstance() for which we generated
> 
>   // Now load the mirror's klass metaobject, and null-check it.
>   // Side-effects region with the control path if the klass is null.
>   Node* kls = load_klass_from_mirror(mirror, never_see_null, nargs,
>                                      region, _prim_path);
> 
>   case vmIntrinsics::_isInstance:
>     // nothing is an instance of a primitive type
>     query_value = gen_instanceof(obj, kls);
> 
> where due to NULL check we have next CastPP for DocAttribute.class:
> [t at 19 l at 19]: print kls->dump(1)
>  390    LoadKlass       === _  7  389  [[ 391  396 ]]  @rawptr:BotPTR, 
> idx=Raw; # *  Klass: * !jvms: 
> AttributeSetUtilities::verifyAttributeValue @ bci:14 
> HashAttributeSet::add @ bci:15 HashAttributeSet::<init> @ bci:36 
> MyHashAttributeSet::<init> @ bci:3 InstanceCheck::t2 @ bci:10
>  395    IfTrue  ===  393  [[ 370  396 ]] #1 !jvms: 
> AttributeSetUtilities::verifyAttributeValue @ bci:14 
> HashAttributeSet::add @ bci:15 HashAttributeSet::<init> @ bci:36 
> MyHashAttributeSet::<init> @ bci:3 InstanceCheck::t2 @ bci:10
>  396    CastPP  ===  395  390  [[ 409  409  413  432  427 ]]  #klass 
> java/lang/Object: 0x0811c7e0 *  Klass:klass java/lang/Object: 0x0811c7e0 
> * !jvms: AttributeSetUtilities::verifyAttributeValue @ bci:14 
> HashAttributeSet::add @ bci:15 HashAttributeSet::<init> @ bci:36 
> MyHashAttributeSet::<init> @ bci:3 InstanceCheck::t2 @ bci:10
> 
> During IGVN LoadKlass(390) transformed to constant klass which is 
> interface:
> 
>  702    ConP    ===  0  [[ 396  391 ]]  #precise klass 
> javax/print/attribute/DocAttribute: 0x08407298:Constant:exact *  
> Interface:precise klass javax/print/attribute/DocAttribute: 
> 0x08407298:Constant:exact *
>  396    CastPP  ===  395  702  [[ 409  409  413  432  427 ]]  #klass 
> java/lang/Object: 0x0811c7e0 *  Klass:klass java/lang/Object: 0x0811c7e0 
> * !jvms: AttributeSetUtilities::verifyAttributeValue @ bci:14 
> HashAttributeSet::add @ bci:15 HashAttributeSet::<init> @ bci:36 
> MyHashAttributeSet::<init> @ bci:3 InstanceCheck::t2 @ bci:10
> 
> ConstraintCastNode::Value() calls TypeOopPtr::filter() method
> which, I think, returns incorrect result:
> 
> [t at 19 l at 19]: print ft->dump()
> precise klass javax/print/attribute/DocAttribute: 
> 0x08407298:Constant:exact *
> 
> [t at 19 l at 19]: print ftkp->klass()->print()
> <ciInstanceKlass name=javax/print/attribute/DocAttribute loader=0x0 
> loaded=true initialized=false finalized=false subklass=false size=9 
> flags=public,interface,abstract mirror=PRESENT ident=655 PERM 
> address=0x8407298>
> 
> [t at 19 l at 19]: print ktkp->klass()->print()
> <ciInstanceKlass name=java/lang/Object loader=0x0 loaded=true 
> initialized=true finalized=false subklass=true size=8 flags=public,super 
> mirror=PRESENT ident=558 PERM address=0x811c7e0>ktkp->klass()->print() = 
> (void)
> 
> because of this code:
> 
>   // If we have an interface-typed Phi or cast and we narrow to a class 
> type,
>   // the join should report back the class.  However, if we have a 
> J/L/Object
>   // class-typed Phi and an interface flows in, it's possible that the 
> meet &
>   // join report an interface back out.  This isn't possible but happens
>   // because the type system doesn't interact well with interfaces.
> 
>   if (ftkp != NULL && ktkp != NULL &&
>       ftkp->is_loaded() &&  ftkp->klass()->is_interface() &&
>       ktkp->is_loaded() && !ktkp->klass()->is_interface()) {
>     // Happens in a CTW of rt.jar, 320-341, no extra flags
>     return ktkp->cast_to_ptr_type(ftkp->ptr());
>   }
> 
> the result is
> 
> precise klass java/lang/Object: 0x0811c7e0:Constant:exact *
> 
> which leads to incorrect result from CmpP node:
> 
>  703    ConP    ===  0  [[ 704  432  704 ]]  #precise klass 
> java/lang/Object: 0x0811c7e0:Constant:exact *  Klass:precise klass 
> java/lang/Object: 0x0811c7e0:Constant:exact *
>  704    CmpP    === _  703  703  [[ 705 ]]  !orig=[413] !jvms: 
> AttributeSetUtilities::verifyAttributeValue @ bci:14 
> HashAttributeSet::add @ bci:15 HashAttributeSet::<init> @ bci:36 
> MyHashAttributeSet::<init> @ bci:3 InstanceCheck::t2 @ bci:10
>