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

Wed Nov 11 17:13:41 UTC 2015

Hi Vladimir,

thanks for reviewing this change.

On Wed, Nov 11, 2015 at 7:36 AM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> wrote:
> 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.
>

Your right.

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

I've refactored that code to:

static void Type::get_arrays_base_elements(const Type *a1, const Type *a2,
                                    const TypePtr **e1, const TypePtr **e2):

and added the NULL-check you've mentioned above.

Unfortunately, Parse::return_current() requires both element types so
I had to make get_arrays_base_elements() return 'void' and use two
additional arguments for the return values.

Please find the new version here:

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

Regards,
Volker

> 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/
>>
>