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

Mon Nov 16 15:29:22 UTC 2015

On Sat, Nov 14, 2015 at 9:56 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com
> wrote:

> > +    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.
>
>
Sorry, your of course right! (Shouldn't work on the weekend :)

I've put the check for isa_instptr() into get_arrays_base_elements() now.
I've also changed the return values of get_arrays_base_elements() from
TypePtr* to TypeInstPtr*. This way I could simplify the callers of
get_arrays_base_elements()
and remove some checks for isa_instptr() in the callers as this check is
now done directly in get_arrays_base_elements(). Here's the new version:

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

Could you please run it trough JPRT one time just for the case.

Thanks,
Volker

> > 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
>>>>> -----------
>>>>>
>>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151116/303ce5d1/attachment-0001.html>