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

[Vladimir Kozlov]

 > +    if (a1tp && a2tp) {
 > +      if (e1) *e1 = a1tp;
 > +      if (e2) *e2 = a2tp;
 > +    }

a2tp is not NULL for any pointer not just instance:

+      a2tp = a2tap->elem()->make_ptr();

so you can have altp as arryptr and a2tp as instptr. I don't see how it guards against different dimensions.

 > I'll write this up in more detail and add it to 8028165, but in my
 > opinion we could close 8028165 as a duplicate of this change once we
 > push this one. What do you think?

Agree, it would be nice.

Thanks,
Vladimir

On 11/14/15 12:49 AM, Volker Simonis wrote:
> On Sat, Nov 14, 2015 at 9:15 AM, Vladimir Kozlov
> <vladimir.kozlov at oracle.com> wrote:
>> So what happens to my next question?:
>>
>>>> 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.
>>
>> get_arrays_base_elements() code is good. You just need to check all its
>> results.
>>
>
> Sorry, but I don't understand. The check for "same dimension" is now
> at the end of get_arrays_base_elements():
>
> +    if (a1tp && a2tp) {
> +      if (e1) *e1 = a1tp;
> +      if (e2) *e2 = a2tp;
> +    }
>
> I only return a result if both results are not NULL, so in my opinion
> that should be enough - or am I missing something?
>
> By the way, yesterday I could verify, that my change also fixes
> "8028165: assert(t->meet(t0) == t) failed: Not monotonic"
> (https://bugs.openjdk.java.net/browse/JDK-8028165). I could reproduce
> that error, create a replay file and verify with that replay file that
> the error doesn't happen with this changes applied.
>
> I then tried, but didn't succeed, to create a smaller test case so I
> debugged with the replay file. I'm currently writing together why this
> change also fixes 8028165, but the short story is that the inserted
> CheckCastPP for meets of class- vs. interface-arrays helps to prevent
> the not-monotonic meet which causes 8028165. In the error case there's
> a Phi node which merges a Field- and a Method-array to an
> AccessibleObject-array which leads to problems later on because
> AccessibleObject is not implementing Member. With my change, the same
> Phi node has two Member-arrays as input (because it adds the checkcast
> nodes which cast the corresponding class arrays to interface arrays
> (i.e. array of Member)).
>
> I'll write this up in more detail and add it to 8028165, but in my
> opinion we could close 8028165 as a duplicate of this change once we
> push this one. What do you think?
>
> Regards,
> Volker
>
>> Thanks,
>> Vladimir
>>
>>
>> On 11/12/15 10:11 AM, Volker Simonis wrote:
>>>
>>> Hi John, Andrew,
>>>
>>> thanks for the nice references and history behind the implementation
>>> (altough two of the bugs are not visible :(
>>>
>>> But the link to 8028165 was especially helpful. I think it describes a
>>> similar problem. I've just managed to reproduce it and I'm now testing
>>> if my fix also helps there. While doing the tests I've also discovered
>>> a problem with my fix which I've addressed in this new version of the
>>> webrev:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2015/8141551.v2/
>>>
>>> The problem was that I casted the class-array to an interface instead
>>> of casting it to an array of interface in Parse::return_current()
>>>
>>> -        value = _gvn.transform(new CheckCastPPNode(0,value,tr));
>>> +          value = _gvn.transform(new CheckCastPPNode(0, value,
>>> phi->bottom_type()));
>>>
>>> And I've also put the whole new part which handles oop-arrays vs.
>>> arrays-of-interface into an else branch because it is mutually
>>> exclusive with the existing part which handles returning oops to an
>>> interface-return.
>>>
>>> Regards,
>>> Volker
>>>
>>>
>>> On Thu, Nov 12, 2015 at 11:27 AM, Andrew Dinn <adinn at redhat.com> wrote:
>>>>
>>>> On 12/11/15 01:33, John Rose wrote:
>>>>>
>>>>> Ughh, that again.  This is 15-year-old technical debt in the C2 type
>>>>> system.
>>>>> It has almost earned VP-level recognition as an organizational cost
>>>>> center.
>>>>>
>>>>> Here is some of the trail of tears:
>>>>> https://bugs.openjdk.java.net/browse/JDK-6312651
>>>>> https://bugs.openjdk.java.net/browse/JDK-4641534
>>>>> https://bugs.openjdk.java.net/browse/JDK-6837094
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8028165?focusedCommentId=13614504&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13614504
>>>>
>>>>
>>>> A shame that the first two links seem not to be traversable (from
>>>> outside of Oracle at least). The final link is well worth a visit, not
>>>> just to the referenced comment but also to the opener -- which is not
>>>> afraid to name names.
>>>>
>>>> I think I resolved the references correctly as follows:
>>>>
>>>> Davey/Priestley to
>>>>
>>>>
>>>>
>>>> http://www.cambridge.org/gb/academic/subjects/mathematics/algebra/introduction-lattices-and-order-2nd-edition
>>>>
>>>> and Nielson/Nielson/Hankin to
>>>>
>>>>     http://www.springer.com/us/book/9783540654100
>>>>
>>>> Hecht to
>>>>
>>>>     http://dl.acm.org/citation.cfm?id=540175
>>>>
>>>> The first one looks like it is just basic lattice theory whereas the
>>>> second one looks to apply that theory to the compilation task and seems
>>>> to be highly relevant to what C2 does (1999 pub date :-). The third text
>>>> also seems to be about applications of lattices to compiler analysis but
>>>> dates back to the 70s. I assume it is one of those 'seminal works'. A
>>>> reference to the 2nd text in type.hpp would probably have helped me a
>>>> lot about 2 years ago.
>>>>
>>>> A Happy Christmas (or alter/non-denominational festive vacation) to all
>>>> our readers.
>>>>
>>>> regards,
>>>>
>>>>
>>>> Andrew Dinn
>>>> -----------
>>>>
>>