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

[Vladimir Kozlov]

In type.cpp you need to check that ftp != NULL && ftp->isa_instptr() in case of number of dimensions are different.
In cfgnode.cpp you need check jtp.

I wish you can factor out that code into type.cpp function instead of repeating 3 times.

Thanks,
Vladimir

On 11/9/15 9:46 AM, Volker Simonis wrote:
> 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/
>