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

[Volker Simonis]

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