Quantcast

RFR : JDK-8166110 : C2 crash in compiling method handles

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR : JDK-8166110 : C2 crash in compiling method handles

Dmitry Chuyko

Summary: some method handles can be used with actual parameters not matching their signatures. It's too expensive to check this on call but possible to check during inlining. Inlined MHs with wrong usage caused further assertion failures in debug builds. The fix adds checks for signatures match during MH inlining so it fails if they are not matching. New test checks linkToStatic and invokeBasic cases.

Bug: https://bugs.openjdk.java.net/browse/JDK-8166110

Webrev: http://cr.openjdk.java.net/~vlivanov/dchuyko/8166110/webrev.00/

Testing: new test, Hotspot and JDK tests, promotion benchmarks.

Thanks,
-Dmitry

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR : JDK-8166110 : C2 crash in compiling method handles

Dmitry Chuyko
The link url may be wrong depending on how html is handled by email client. Thanks to those who noticed that.
 
Webrev: http://cr.openjdk.java.net/~vlivanov/dchuyko/8166110/webrev.00/

-Dmitry

On 02/03/2017 05:19 PM, Dmitry Chuyko wrote:

Summary: some method handles can be used with actual parameters not matching their signatures. It's too expensive to check this on call but possible to check during inlining. Inlined MHs with wrong usage caused further assertion failures in debug builds. The fix adds checks for signatures match during MH inlining so it fails if they are not matching. New test checks linkToStatic and invokeBasic cases.

Bug: https://bugs.openjdk.java.net/browse/JDK-8166110

Webrev: http://cr.openjdk.java.net/~vlivanov/dchuyko/8166110/webrev.00/

Testing: new test, Hotspot and JDK tests, promotion benchmarks.

Thanks,
-Dmitry


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR : JDK-8166110 : C2 crash in compiling method handles

Vladimir Ivanov
In reply to this post by Dmitry Chuyko
The fix looks good.

More detailed description of the problem:

MH.invokeBasic() and MH.linkTo* signature-polymorphic are inherently
unsafe. There are no type checks involved and it's user responsibility
to ensure there are no type mismatches possible. They are available only
to trusted code (in java.lang.invoke), which ensures the signatures
always match.

Unfortunately, even though type mismatches aren't possible at runtime,
JIT-compilers can encounter them in paradoxical situations. It usually
happens when optimizing effectively dead code.

BoundMethodHandle.arg(I) demonstrates one of the problematic code shapes:

final Object arg(int i) {
     switch (speciesData().fieldType(i)) {
     case L_TYPE: return
speciesData().getters[i].invokeBasic(this);
     case I_TYPE: return (int)
speciesData().getters[i].invokeBasic(this);
     case J_TYPE: return (long)
speciesData().getters[i].invokeBasic(this);
     case F_TYPE: return (float)
speciesData().getters[i].invokeBasic(this);
     case D_TYPE: return (double)
speciesData().getters[i].invokeBasic(this);
}

If C2 can constant fold speciesData().getters[i], it will try to inline
through invokeBasic() on all execution paths, though only 1 of them is
taken. On all other paths the signatures won't match and it can lead to
type paradoxes on IR level though the code is effectively dead.

Though it was observed only with invokeBasic() and I don't see such
situations is possible with MH.linkTo*(), for consistency the fix
touches MH linkers as well.

Best regards,
Vladimir Ivanov

On 2/3/17 5:19 PM, Dmitry Chuyko wrote:

> Summary: some method handles can be used with actual parameters not
> matching their signatures. It's too expensive to check this on call but
> possible to check during inlining. Inlined MHs with wrong usage caused
> further assertion failures in debug builds. The fix adds checks for
> signatures match during MH inlining so it fails if they are not
> matching. New test checks linkToStatic and invokeBasic cases.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8166110
>
> Webrev: http://cr.openjdk.java.net/~vlivanov/dchuyko/8166110/webrev.00/
>
> Testing: new test, Hotspot and JDK tests, promotion benchmarks.
>
> Thanks,
> -Dmitry
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR : JDK-8166110 : C2 crash in compiling method handles

Remi Forax
Hi Vladimir,
in the long term, c2 (and c1) should be fixed to stop to inline switch branches that have never been taken,
the dynamic language I currently maintain tranforms all switch to a tree of GWTs because of that,
i think we will need this improvement to implement pattern matching in 10 anyway*.

Rémi
* apart if we introduce a custom method handle.

----- Mail original -----
> De: "Vladimir Ivanov" <[hidden email]>
> À: "Dmitry Chuyko" <[hidden email]>, [hidden email]
> Envoyé: Vendredi 3 Février 2017 17:59:53
> Objet: Re: RFR : JDK-8166110 : C2 crash in compiling method handles

> The fix looks good.
>
> More detailed description of the problem:
>
> MH.invokeBasic() and MH.linkTo* signature-polymorphic are inherently
> unsafe. There are no type checks involved and it's user responsibility
> to ensure there are no type mismatches possible. They are available only
> to trusted code (in java.lang.invoke), which ensures the signatures
> always match.
>
> Unfortunately, even though type mismatches aren't possible at runtime,
> JIT-compilers can encounter them in paradoxical situations. It usually
> happens when optimizing effectively dead code.
>
> BoundMethodHandle.arg(I) demonstrates one of the problematic code shapes:
>
> final Object arg(int i) {
>     switch (speciesData().fieldType(i)) {
>     case L_TYPE: return
> speciesData().getters[i].invokeBasic(this);
>     case I_TYPE: return (int)
> speciesData().getters[i].invokeBasic(this);
>     case J_TYPE: return (long)
> speciesData().getters[i].invokeBasic(this);
>     case F_TYPE: return (float)
> speciesData().getters[i].invokeBasic(this);
>     case D_TYPE: return (double)
> speciesData().getters[i].invokeBasic(this);
> }
>
> If C2 can constant fold speciesData().getters[i], it will try to inline
> through invokeBasic() on all execution paths, though only 1 of them is
> taken. On all other paths the signatures won't match and it can lead to
> type paradoxes on IR level though the code is effectively dead.
>
> Though it was observed only with invokeBasic() and I don't see such
> situations is possible with MH.linkTo*(), for consistency the fix
> touches MH linkers as well.
>
> Best regards,
> Vladimir Ivanov
>
> On 2/3/17 5:19 PM, Dmitry Chuyko wrote:
>> Summary: some method handles can be used with actual parameters not
>> matching their signatures. It's too expensive to check this on call but
>> possible to check during inlining. Inlined MHs with wrong usage caused
>> further assertion failures in debug builds. The fix adds checks for
>> signatures match during MH inlining so it fails if they are not
>> matching. New test checks linkToStatic and invokeBasic cases.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166110
>>
>> Webrev: http://cr.openjdk.java.net/~vlivanov/dchuyko/8166110/webrev.00/
>>
>> Testing: new test, Hotspot and JDK tests, promotion benchmarks.
>>
>> Thanks,
>> -Dmitry
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR : JDK-8166110 : C2 crash in compiling method handles

Vladimir Ivanov
Remi,

> in the long term, c2 (and c1) should be fixed to stop to inline switch branches that have never been taken,
> the dynamic language I currently maintain tranforms all switch to a tree of GWTs because of that,
> i think we will need this improvement to implement pattern matching in 10 anyway*.

Yes, C2 can do better when optimizing switches (e.g., [1]), but
profiling information is not our friend here. Even if it says all
branches are equally taken, type paradoxes arise from decisions in the
context of the current compilation where only one branch is always taken
at runtime.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8058192

> ----- Mail original -----
>> De: "Vladimir Ivanov" <[hidden email]>
>> À: "Dmitry Chuyko" <[hidden email]>, [hidden email]
>> Envoyé: Vendredi 3 Février 2017 17:59:53
>> Objet: Re: RFR : JDK-8166110 : C2 crash in compiling method handles
>
>> The fix looks good.
>>
>> More detailed description of the problem:
>>
>> MH.invokeBasic() and MH.linkTo* signature-polymorphic are inherently
>> unsafe. There are no type checks involved and it's user responsibility
>> to ensure there are no type mismatches possible. They are available only
>> to trusted code (in java.lang.invoke), which ensures the signatures
>> always match.
>>
>> Unfortunately, even though type mismatches aren't possible at runtime,
>> JIT-compilers can encounter them in paradoxical situations. It usually
>> happens when optimizing effectively dead code.
>>
>> BoundMethodHandle.arg(I) demonstrates one of the problematic code shapes:
>>
>> final Object arg(int i) {
>>     switch (speciesData().fieldType(i)) {
>>     case L_TYPE: return
>> speciesData().getters[i].invokeBasic(this);
>>     case I_TYPE: return (int)
>> speciesData().getters[i].invokeBasic(this);
>>     case J_TYPE: return (long)
>> speciesData().getters[i].invokeBasic(this);
>>     case F_TYPE: return (float)
>> speciesData().getters[i].invokeBasic(this);
>>     case D_TYPE: return (double)
>> speciesData().getters[i].invokeBasic(this);
>> }
>>
>> If C2 can constant fold speciesData().getters[i], it will try to inline
>> through invokeBasic() on all execution paths, though only 1 of them is
>> taken. On all other paths the signatures won't match and it can lead to
>> type paradoxes on IR level though the code is effectively dead.
>>
>> Though it was observed only with invokeBasic() and I don't see such
>> situations is possible with MH.linkTo*(), for consistency the fix
>> touches MH linkers as well.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 2/3/17 5:19 PM, Dmitry Chuyko wrote:
>>> Summary: some method handles can be used with actual parameters not
>>> matching their signatures. It's too expensive to check this on call but
>>> possible to check during inlining. Inlined MHs with wrong usage caused
>>> further assertion failures in debug builds. The fix adds checks for
>>> signatures match during MH inlining so it fails if they are not
>>> matching. New test checks linkToStatic and invokeBasic cases.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166110
>>>
>>> Webrev: http://cr.openjdk.java.net/~vlivanov/dchuyko/8166110/webrev.00/
>>>
>>> Testing: new test, Hotspot and JDK tests, promotion benchmarks.
>>>
>>> Thanks,
>>> -Dmitry
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR : JDK-8166110 : C2 crash in compiling method handles

Remi Forax
Profile information false sharing !

Again, creating a thread local storage of MethodDatas at the starts of methodHandle.invoke so you will get profile info per method handle invoke.
It seems better and more general than by example the adhoc way to profile the condition of a gwt.

Rémi

----- Mail original -----
> De: "Vladimir Ivanov" <[hidden email]>
> À: "Remi Forax" <[hidden email]>
> Cc: [hidden email]
> Envoyé: Vendredi 3 Février 2017 18:29:56
> Objet: Re: RFR : JDK-8166110 : C2 crash in compiling method handles

> Remi,
>
>> in the long term, c2 (and c1) should be fixed to stop to inline switch branches
>> that have never been taken,
>> the dynamic language I currently maintain tranforms all switch to a tree of GWTs
>> because of that,
>> i think we will need this improvement to implement pattern matching in 10
>> anyway*.
>
> Yes, C2 can do better when optimizing switches (e.g., [1]), but
> profiling information is not our friend here. Even if it says all
> branches are equally taken, type paradoxes arise from decisions in the
> context of the current compilation where only one branch is always taken
> at runtime.
>
> Best regards,
> Vladimir Ivanov
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8058192
>
>> ----- Mail original -----
>>> De: "Vladimir Ivanov" <[hidden email]>
>>> À: "Dmitry Chuyko" <[hidden email]>,
>>> [hidden email]
>>> Envoyé: Vendredi 3 Février 2017 17:59:53
>>> Objet: Re: RFR : JDK-8166110 : C2 crash in compiling method handles
>>
>>> The fix looks good.
>>>
>>> More detailed description of the problem:
>>>
>>> MH.invokeBasic() and MH.linkTo* signature-polymorphic are inherently
>>> unsafe. There are no type checks involved and it's user responsibility
>>> to ensure there are no type mismatches possible. They are available only
>>> to trusted code (in java.lang.invoke), which ensures the signatures
>>> always match.
>>>
>>> Unfortunately, even though type mismatches aren't possible at runtime,
>>> JIT-compilers can encounter them in paradoxical situations. It usually
>>> happens when optimizing effectively dead code.
>>>
>>> BoundMethodHandle.arg(I) demonstrates one of the problematic code shapes:
>>>
>>> final Object arg(int i) {
>>>     switch (speciesData().fieldType(i)) {
>>>     case L_TYPE: return
>>> speciesData().getters[i].invokeBasic(this);
>>>     case I_TYPE: return (int)
>>> speciesData().getters[i].invokeBasic(this);
>>>     case J_TYPE: return (long)
>>> speciesData().getters[i].invokeBasic(this);
>>>     case F_TYPE: return (float)
>>> speciesData().getters[i].invokeBasic(this);
>>>     case D_TYPE: return (double)
>>> speciesData().getters[i].invokeBasic(this);
>>> }
>>>
>>> If C2 can constant fold speciesData().getters[i], it will try to inline
>>> through invokeBasic() on all execution paths, though only 1 of them is
>>> taken. On all other paths the signatures won't match and it can lead to
>>> type paradoxes on IR level though the code is effectively dead.
>>>
>>> Though it was observed only with invokeBasic() and I don't see such
>>> situations is possible with MH.linkTo*(), for consistency the fix
>>> touches MH linkers as well.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 2/3/17 5:19 PM, Dmitry Chuyko wrote:
>>>> Summary: some method handles can be used with actual parameters not
>>>> matching their signatures. It's too expensive to check this on call but
>>>> possible to check during inlining. Inlined MHs with wrong usage caused
>>>> further assertion failures in debug builds. The fix adds checks for
>>>> signatures match during MH inlining so it fails if they are not
>>>> matching. New test checks linkToStatic and invokeBasic cases.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166110
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~vlivanov/dchuyko/8166110/webrev.00/
>>>>
>>>> Testing: new test, Hotspot and JDK tests, promotion benchmarks.
>>>>
>>>> Thanks,
> >>> -Dmitry
Loading...