RFR(S) 8190817: deopt special-case for _return_register_finalizer is confusing and leads to bugs

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

RFR(S) 8190817: deopt special-case for _return_register_finalizer is confusing and leads to bugs

dean.long
https://bugs.openjdk.java.net/browse/JDK-8190817

http://cr.openjdk.java.net/~dlong/8190817/webrev/

This fix replaces the problematic use of
_normal_table.entry(Bytecodes::_return).entry(vtos) as a deoptimization
entry point with a proper deopt entry point returned by
deopt_reexecute_return_entry().  This is needed to handle the situation
where compiled code is calling register_finalizer() and gets deoptimized.

I also noticed that we generate duplicate entry points unnecessarily, so
I cleaned that up at the same time.

aarch64/ppc64/s390 folks, please check that
compiler/runtime/Test8168712.java still passes.

dl

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8190817: deopt special-case for _return_register_finalizer is confusing and leads to bugs

Dmitrij Pochepko-2
Hi,

I've built clean fastdebug build with this patch and run
compiler/runtime/Test8168712.java on AArch64 (ThunderX) and it passed.

Please note that this test can't be run on AArch64 by default because of
@requires tag. I had to update this tag first. Btw: I wonder if it
should be also updated as a part of this patch?


I also took a look at Aarch64-specific change and it looks good to
me(not a Reviewer).


Thanks,

Dmitrij


On 13.11.2017 20:32, [hidden email] wrote:

> https://bugs.openjdk.java.net/browse/JDK-8190817
>
> http://cr.openjdk.java.net/~dlong/8190817/webrev/
>
> This fix replaces the problematic use of
> _normal_table.entry(Bytecodes::_return).entry(vtos) as a
> deoptimization entry point with a proper deopt entry point returned by
> deopt_reexecute_return_entry().  This is needed to handle the
> situation where compiled code is calling register_finalizer() and gets
> deoptimized.
>
> I also noticed that we generate duplicate entry points unnecessarily,
> so I cleaned that up at the same time.
>
> aarch64/ppc64/s390 folks, please check that
> compiler/runtime/Test8168712.java still passes.
>
> dl
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8190817: deopt special-case for _return_register_finalizer is confusing and leads to bugs

dean.long
Thanks Dmitrij!  I have updated the test as well.  Good suggestion.

dl

http://cr.openjdk.java.net/~dlong/8190817/webrev.1/


On 11/17/17 11:15 AM, Dmitrij Pochepko wrote:

> Hi,
>
> I've built clean fastdebug build with this patch and run
> compiler/runtime/Test8168712.java on AArch64 (ThunderX) and it passed.
>
> Please note that this test can't be run on AArch64 by default because
> of @requires tag. I had to update this tag first. Btw: I wonder if it
> should be also updated as a part of this patch?
>
>
> I also took a look at Aarch64-specific change and it looks good to
> me(not a Reviewer).
>
>
> Thanks,
>
> Dmitrij
>
>
> On 13.11.2017 20:32, [hidden email] wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8190817
>>
>> http://cr.openjdk.java.net/~dlong/8190817/webrev/
>>
>> This fix replaces the problematic use of
>> _normal_table.entry(Bytecodes::_return).entry(vtos) as a
>> deoptimization entry point with a proper deopt entry point returned
>> by deopt_reexecute_return_entry().  This is needed to handle the
>> situation where compiled code is calling register_finalizer() and
>> gets deoptimized.
>>
>> I also noticed that we generate duplicate entry points unnecessarily,
>> so I cleaned that up at the same time.
>>
>> aarch64/ppc64/s390 folks, please check that
>> compiler/runtime/Test8168712.java still passes.
>>
>> dl
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8190817: deopt special-case for _return_register_finalizer is confusing and leads to bugs

dean.long
Ping.  I still need a Reviewer to look at this.

dl


On 11/17/17 12:01 PM, [hidden email] wrote:

> Thanks Dmitrij!  I have updated the test as well.  Good suggestion.
>
> dl
>
> http://cr.openjdk.java.net/~dlong/8190817/webrev.1/
>
>
> On 11/17/17 11:15 AM, Dmitrij Pochepko wrote:
>> Hi,
>>
>> I've built clean fastdebug build with this patch and run
>> compiler/runtime/Test8168712.java on AArch64 (ThunderX) and it passed.
>>
>> Please note that this test can't be run on AArch64 by default because
>> of @requires tag. I had to update this tag first. Btw: I wonder if it
>> should be also updated as a part of this patch?
>>
>>
>> I also took a look at Aarch64-specific change and it looks good to
>> me(not a Reviewer).
>>
>>
>> Thanks,
>>
>> Dmitrij
>>
>>
>> On 13.11.2017 20:32, [hidden email] wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8190817
>>>
>>> http://cr.openjdk.java.net/~dlong/8190817/webrev/
>>>
>>> This fix replaces the problematic use of
>>> _normal_table.entry(Bytecodes::_return).entry(vtos) as a
>>> deoptimization entry point with a proper deopt entry point returned
>>> by deopt_reexecute_return_entry().  This is needed to handle the
>>> situation where compiled code is calling register_finalizer() and
>>> gets deoptimized.
>>>
>>> I also noticed that we generate duplicate entry points
>>> unnecessarily, so I cleaned that up at the same time.
>>>
>>> aarch64/ppc64/s390 folks, please check that
>>> compiler/runtime/Test8168712.java still passes.
>>>
>>> dl
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8190817: deopt special-case for _return_register_finalizer is confusing and leads to bugs

Vladimir Ivanov
In reply to this post by dean.long
> http://cr.openjdk.java.net/~dlong/8190817/webrev.1/

Looks good.

Best regards,
Vladimir Ivanov

>
>
> On 11/17/17 11:15 AM, Dmitrij Pochepko wrote:
>> Hi,
>>
>> I've built clean fastdebug build with this patch and run
>> compiler/runtime/Test8168712.java on AArch64 (ThunderX) and it passed.
>>
>> Please note that this test can't be run on AArch64 by default because
>> of @requires tag. I had to update this tag first. Btw: I wonder if it
>> should be also updated as a part of this patch?
>>
>>
>> I also took a look at Aarch64-specific change and it looks good to
>> me(not a Reviewer).
>>
>>
>> Thanks,
>>
>> Dmitrij
>>
>>
>> On 13.11.2017 20:32, [hidden email] wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8190817
>>>
>>> http://cr.openjdk.java.net/~dlong/8190817/webrev/
>>>
>>> This fix replaces the problematic use of
>>> _normal_table.entry(Bytecodes::_return).entry(vtos) as a
>>> deoptimization entry point with a proper deopt entry point returned
>>> by deopt_reexecute_return_entry().  This is needed to handle the
>>> situation where compiled code is calling register_finalizer() and
>>> gets deoptimized.
>>>
>>> I also noticed that we generate duplicate entry points unnecessarily,
>>> so I cleaned that up at the same time.
>>>
>>> aarch64/ppc64/s390 folks, please check that
>>> compiler/runtime/Test8168712.java still passes.
>>>
>>> dl
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8190817: deopt special-case for _return_register_finalizer is confusing and leads to bugs

dean.long
On 11/21/17 6:23 AM, Vladimir Ivanov wrote:

>> http://cr.openjdk.java.net/~dlong/8190817/webrev.1/
>
> Looks good.
>

Thanks Vladimir.

dl

> Best regards,
> Vladimir Ivanov
>
>>
>>
>> On 11/17/17 11:15 AM, Dmitrij Pochepko wrote:
>>> Hi,
>>>
>>> I've built clean fastdebug build with this patch and run
>>> compiler/runtime/Test8168712.java on AArch64 (ThunderX) and it passed.
>>>
>>> Please note that this test can't be run on AArch64 by default
>>> because of @requires tag. I had to update this tag first. Btw: I
>>> wonder if it should be also updated as a part of this patch?
>>>
>>>
>>> I also took a look at Aarch64-specific change and it looks good to
>>> me(not a Reviewer).
>>>
>>>
>>> Thanks,
>>>
>>> Dmitrij
>>>
>>>
>>> On 13.11.2017 20:32, [hidden email] wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8190817
>>>>
>>>> http://cr.openjdk.java.net/~dlong/8190817/webrev/
>>>>
>>>> This fix replaces the problematic use of
>>>> _normal_table.entry(Bytecodes::_return).entry(vtos) as a
>>>> deoptimization entry point with a proper deopt entry point returned
>>>> by deopt_reexecute_return_entry().  This is needed to handle the
>>>> situation where compiled code is calling register_finalizer() and
>>>> gets deoptimized.
>>>>
>>>> I also noticed that we generate duplicate entry points
>>>> unnecessarily, so I cleaned that up at the same time.
>>>>
>>>> aarch64/ppc64/s390 folks, please check that
>>>> compiler/runtime/Test8168712.java still passes.
>>>>
>>>> dl
>>>>
>>>
>>