RFR (M): 7157365: jruby/bench.bench_timeout crashes with JVM internal error

[John Rose]

Reviewed.

— John

P.S. One comment:  This code in memnode.cpp obscured the root of your problem:

  case T_OBJECT:
#ifdef _LP64
    if (adr->bottom_type()->is_ptr_to_narrowoop()) {
      Node* load  = gvn.transform(new (C, 3) LoadNNode(ctl, mem, adr, adr_type, rt->make_narrowoop()));
      return new (C, 2) DecodeNNode(load, load->bottom_type()->make_ptr());
    } else
#endif

There should be a separate case T_NARROWOOP here, instead of overloading T_OBJECT and secretly distinguishing the adr-type.
The LoadNode::make factory should have a 1-1 relation between BasicType and LoadNode subtype.
Most uses of T_OBJECT mean "object value in a register".

The choice of using T_NARROWOOP should (probably) be moved higher up the call chain, to make it more clear which kind of oop we are referring to. I would consider putting the decision in Parse::do_get_xxx and parallel uses of GraphKit::make_load.

It seems to me, in this moment, that those few occurrences of T_OBJECT which map to memory references to compressed oops should be made explicitly T_NARROWOOP.  The alternative (non-compressed in-memory values) could be tagged T_ADDRESS (or maybe T_WIDEOOP) for explicitness, or T_OBJECT could continue to be a full word (as ArrayElementSize appears to promise).

Would you mind filing a separate bug for cleaning this up?  The #ifdef should go away too, of course. 
(If there is already an #ifdef removal bug, this could be added as a comment.)


On Jun 14, 2012, at 2:38 PM, Christian Thalinger wrote:

> http://cr.openjdk.java.net/~twisti/7157365
> 
> 7157365: jruby/bench.bench_timeout crashes with JVM internal error
> Reviewed-by:
> 
> The problem manifests itself as an assert in escape analysis code:
> 
> assert((ptnode_adr(adr->_idx) == NULL || ptnode_adr(adr->_idx)->as_Field()->is_oop())) failed: sanity
> 
> The out-of-line code for invokedynamic instructions loads the CallSite object
> from the constant pool cache.  Since the constant pool cache contains data 
> other than oops we return the type as byte[] and load the oop as raw pointer.
> Escape analysis notices this misbehavior and bails out.
> 
> The best possible fix for now is to define the constant pool cache base 
> pointer as an oop array and load a raw pointer from there.
> 
> Note that this is more of a temporary fix since the perm-gen removal will fix 
> this problem in a more correct fashion.    
> 
> src/share/vm/memory/universe.hpp
> src/share/vm/opto/callGenerator.cpp
> src/share/vm/opto/chaitin.cpp
> src/share/vm/opto/type.cpp
>