RFR(M): 8141551: C2 can not handle returns with incompatible interface arrays

[Volker Simonis]

Hi,

can I please have a review and a sponsor for the following change:

http://cr.openjdk.java.net/~simonis/webrevs/2015/8141551/
https://bugs.openjdk.java.net/browse/JDK-8141551

The explanation is a little lengthy so I'd suggest to read the
HTML-version in the webrev. Nevertheless I've duplicated here for your
convenience:

Given the following interfaces and classes:

public static interface I1 { public String getName(); }
public static interface I2 { public String getName(); }
public static class I2C implements I2 { public String getName() {
return "I2";} }
public static class I21C implements I2, I1 { public String getName() {
return "I2 and I1";} }

public static class Helper {
  public static I2 createI2Array0() {
    return new I2C();
  }
  public static I2[] createI2Array1() {
    return new I2C[] { new I2C() };
  }
}

we can write the following "pseudo" Java code:

public class MeetIncompatibleInterfaceArrays0ASM {
  public static I1 run() {
    return Helper.createI2Array0(); // returns I2
  }
  public static void test() {
    I1 i1 = run();
    System.out.println(i1.getName());
  }
}
public class MeetIncompatibleInterfaceArrays1ASM {
  public static I1[] run() {
    return Helper.createI2Array1(); // returns I2[]
  }
  public static void test() {
    I1[] i1 = run();
    System.out.println(i1[0].getName());
  }
}

Notice that this is not legal Java code. We would have to use a cast
in run() to make it legal:

  public static I1[] run() {
    return (I1[])Helper.createI2Array1(); // returns I2[]
  }

But in pure bytecode, the run() methods are perfectly legal:

  public static I1[] run();
    Code:
      0: invokestatic  #16  // Method Helper.createI2Array1:()[LI2;
      3: areturn

C2 can compile MeetIncompatibleInterfaceArrays0ASM.run() but will fail
to compile MeetIncompatibleInterfaceArrays1ASM.run() because it can
not meet the two array types I1[] and I2[] during type analysis.
Currently this leads to an assertion in the debug build but to an
infinite compile loop with a final crash because of the lack of native
memory in a product build. This is the offending code from
Parse::do_exits() in src/share/vm/opto/parse1.cpp:

    if (!_exits.control()->is_top() && _gvn.type(ret_phi)->empty()) {
      // In case of concurrent class loading, the type we set for the
      // ret_phi in build_exits() may have been too optimistic and the
      // ret_phi may be top now.
#ifdef ASSERT
      {
        MutexLockerEx ml(Compile_lock, Mutex::_no_safepoint_check_flag);
        assert(ret_type->isa_ptr() &&
C->env()->system_dictionary_modification_counter_changed(), "return
value must be well defined");
      }
#endif
      C->record_failure(C2Compiler::retry_class_loading_during_parsing());
    }

If the compiler didn't find a valid return type for a method, it
simply assumes that this could only happen because of concurrent class
loading and retries the compilation (by setting the return value to
C2Compiler::retry_class_loading_during_parsing). Only in the debug
build he really asserts that concurrent class loading actually
happened.

This could should be fixed to something like:

    if (!_exits.control()->is_top() && _gvn.type(ret_phi)->empty()) {
      // In case of concurrent class loading, the type we set for the
      // ret_phi in build_exits() may have been too optimistic and the
      // ret_phi may be top now.
      // Otherwise, we've encountered an error and have to mark the method as
      // not compilable. Just using an assertion instead would be dangerous
      // as this could lead to an infinite compile loop in non-debug builds.
      {
        MutexLockerEx ml(Compile_lock, Mutex::_no_safepoint_check_flag);
        if (C->env()->system_dictionary_modification_counter_changed()) {
          C->record_failure(C2Compiler::retry_class_loading_during_parsing());
        } else {
          C->record_method_not_compilable("Can't determine return type.");
        }
      }
      return;
    }

That is, in the case where no concurrent class loading happened, we
simply record the method as not compilable in order to prevent an
infinite compile loop on that method. Of course that only fixes the
implications of C2's inability to correctly compile the respective
method. But it will also help if other unexpected problems occur and
it is always better to happily run with an uncompiled method instead
of crashing while trying to compile it.

However a real fix should also improve C2'S type analysis in such a
way to allow the compilation of such strange methods. This can be done
as follows:

C2 already has code which handles anomalies in its type system due to
the different handling of interfaces and classes. During the type
computation of a Phi-Node in PhiNode::Value() there's special code
which uplifts the meet of an interface with a class type to the
interface type if the result of the normal meet would otherwise be
empty. However this code only handles interface vs. class types but
not 'array of interface' vs 'array of class' types as they occur in
our example. The proper handling of such cases is added by this
changeset together with a complementary fix of
TypeOopPtr::filter_helper() which has exactly the same problem when
joining two types.

Unfortunately this only partially fixes the problem. There's another
code in Parse::return_current() which inserts implicit casts to
interface if a method returns a class type to an interface return.
This happens with our example if the createI2Array() method is inlined
into the run() method and constant propagation finds the 'real' return
type of the method which is I2C (instead of the declared I1). Again,
this can be fixed by handling returns of array-of-class types to
array-of-interface returns in the same way like this is already done
for returns of class types to interface returns (i.e. by inserting an
implicit cast to the interface type).

Finally, meeting interface with class types in the way described above
can result in unsymmetric type lattices (see Cliff Click's blog at
[1][2][3] for a nice description of the implications). They would be
detected and objected in debug builds by special verification code in
Type::meet_helper(). But because this also happens with simple
interface and class type meets, such meets are already excluded from
the check by the help of the Type::interface_vs_oop() function. This
helper is virtual and overloaded for TypeAry and TypeAryPtr but
unfortunately, the TypeAry version doesn't handle compressed oops
correctly which is fixed by this change as well.

This change also contains a regression test which tries to exercise
the offending Java bytecode code in various variations like
interpreted, C1, C2, with and without inlining and with arrays of
different dimensions. I've also verified the proposed code by running
the JCK vm and lang test in mixed mode, with and without tiered
compilation and with -Xcomp without seeing any problems.

Thank you and best regards,
Volker

[1] http://www.cliffc.org/blog/2012/02/12/too-much-theory
[2] http://www.cliffc.org/blog/2012/02/27/too-much-theory-part-2
[3] http://www.cliffc.org/blog/2012/03/24/too-much-theory-part-3

PS: I initially had another fix which changed
TypeAryPtr::xmeet_helper() to properly handle arryays of interfaces
meeting arrays of classes. It worked equally well and passed all the
tests I had done, but the problem was that it produced not only
unsymmetric type lattices but also non-commutative ones. They had to
be additionally excluded from the verification code in
Type::meet_helper() and I finally decided the the first solution with
a implicit CheckCastPPNode is nicer and cleaner. Just for your
information I'll also post the first version here:

http://cr.openjdk.java.net/~simonis/webrevs/2015/8141551_not_commutative/