RFR: JDK-8172791 fixing issues with Reserved Stack Area

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

RFR: JDK-8172791 fixing issues with Reserved Stack Area

frederic parain
Greetings,

Please review this fix which addresses several issues with the
ReservedStackArea implementation:
   1 - the method look_for_reserved_stack_annotated_method() fails to
detect in-lined
        annotated methods, making the annotation ineffective for
in-lined methods
   2 - sometime an assertion failure related to reserved area status
occurs after incorrect
        restoring of guards pages during a return from runtime

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

webrev:
http://cr.openjdk.java.net/~fparain/8172791/webrev.00/index.html

This fix has been contributed by Andrew Haley.

Thank you,

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

Re: RFR: JDK-8172791 fixing issues with Reserved Stack Area

Andrew Haley
Just to explain why this patch is needed: without it, JEP 270
(ReservedStackArea) does not work at all if a method with a
ReservedStack annotation is inlined.  Which, in practice, is all the
time, because aggressive inlining is what C2 does.

Can somebody please have a look at this?


On 18/04/17 15:47, Frederic Parain wrote:

> Greetings,
>
> Please review this fix which addresses several issues with the
> ReservedStackArea implementation:
>    1 - the method look_for_reserved_stack_annotated_method() fails to
> detect in-lined
>         annotated methods, making the annotation ineffective for
> in-lined methods
>    2 - sometime an assertion failure related to reserved area status
> occurs after incorrect
>         restoring of guards pages during a return from runtime
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8172791
>
> webrev:
> http://cr.openjdk.java.net/~fparain/8172791/webrev.00/index.html
>
> This fix has been contributed by Andrew Haley.
>
> Thank you,
>
> Fred

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: JDK-8172791 fixing issues with Reserved Stack Area

Daniel D. Daugherty
On 6/30/17 9:03 AM, Andrew Haley wrote:

> Just to explain why this patch is needed: without it, JEP 270
> (ReservedStackArea) does not work at all if a method with a
> ReservedStack annotation is inlined.  Which, in practice, is all the
> time, because aggressive inlining is what C2 does.
>
> Can somebody please have a look at this?
>
>
> On 18/04/17 15:47, Frederic Parain wrote:
>> Greetings,
>>
>> Please review this fix which addresses several issues with the
>> ReservedStackArea implementation:
>>     1 - the method look_for_reserved_stack_annotated_method() fails to
>> detect in-lined
>>          annotated methods, making the annotation ineffective for
>> in-lined methods
>>     2 - sometime an assertion failure related to reserved area status
>> occurs after incorrect
>>          restoring of guards pages during a return from runtime
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8172791
>>
>> webrev:
>> http://cr.openjdk.java.net/~fparain/8172791/webrev.00/index.html

src/cpu/x86/vm/interp_masm_x86.cpp
     Is the deletion of:

     L1083:     push(rthread);

     related to the assertion failure part of the fix? It looks like
     it is just fixing a call protocol error (pushing rthread when it
     is not needed), but I'm not sure.

src/share/vm/code/compiledMethod.cpp
     No comments.

src/share/vm/code/compiledMethod.hpp
     No comments.

src/share/vm/opto/compile.cpp
     No comments.

src/share/vm/runtime/sharedRuntime.cpp
     L3157:         for (ScopeDesc *sd = nm->scope_desc_near(fr.pc());
sd; sd = sd->sender()) {
         nit: implied boolean with 'sd;'
              please change to:    'sd != NULL;'

test/runtime/ReservedStack/ReservedStackTest.java
     Why stop running the test on -Xint?


Thumbs up on the fix.


This fix is going to need a review from someone on the
Compiler team also. Fred, who did your JEP-270 reviews
from the Compiler team?

Dan


>>
>> This fix has been contributed by Andrew Haley.
>>
>> Thank you,
>>
>> Fred

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

Re: RFR: JDK-8172791 fixing issues with Reserved Stack Area

frederic parain
Dan,

Thank your for reviewing this code.
My answers are in-lined below.


> On Jun 30, 2017, at 19:56, Daniel D. Daugherty <[hidden email]> wrote:
>
> On 6/30/17 9:03 AM, Andrew Haley wrote:
>> Just to explain why this patch is needed: without it, JEP 270
>> (ReservedStackArea) does not work at all if a method with a
>> ReservedStack annotation is inlined.  Which, in practice, is all the
>> time, because aggressive inlining is what C2 does.
>>
>> Can somebody please have a look at this?
>>
>>
>> On 18/04/17 15:47, Frederic Parain wrote:
>>> Greetings,
>>>
>>> Please review this fix which addresses several issues with the
>>> ReservedStackArea implementation:
>>>    1 - the method look_for_reserved_stack_annotated_method() fails to
>>> detect in-lined
>>>         annotated methods, making the annotation ineffective for
>>> in-lined methods
>>>    2 - sometime an assertion failure related to reserved area status
>>> occurs after incorrect
>>>         restoring of guards pages during a return from runtime
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8172791
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~fparain/8172791/webrev.00/index.html
>
> src/cpu/x86/vm/interp_masm_x86.cpp
>    Is the deletion of:
>
>    L1083:     push(rthread);
>
>    related to the assertion failure part of the fix? It looks like
>    it is just fixing a call protocol error (pushing rthread when it
>    is not needed), but I'm not sure.

It’s fixing a call protocol error, this is not related to the assertion
failure described in point 2 above. AFAIK, this error did not trigger
issues, but it is cleaner to remove it.

>
> src/share/vm/code/compiledMethod.cpp
>    No comments.
>
> src/share/vm/code/compiledMethod.hpp
>    No comments.
>
> src/share/vm/opto/compile.cpp
>    No comments.
>
> src/share/vm/runtime/sharedRuntime.cpp
>    L3157:         for (ScopeDesc *sd = nm->scope_desc_near(fr.pc()); sd; sd = sd->sender()) {
>        nit: implied boolean with 'sd;'
>             please change to:    'sd != NULL;’

Fixed

>
> test/runtime/ReservedStack/ReservedStackTest.java
>    Why stop running the test on -Xint?
>

Running in interpreted only has very limited interest because the bug cannot
occur in this mode (due to the respective sizes of the different frames). Removing
this mode makes the test faster.

>
> Thumbs up on the fix.
>
>
> This fix is going to need a review from someone on the
> Compiler team also. Fred, who did your JEP-270 reviews
> from the Compiler team?
>

I cannot remember. I’ve discussed JEP-270 code with the compiler team before the
RFR. I’ll contact them to have a second review.

Thank you,

Fred


>>>
>>> This fix has been contributed by Andrew Haley.
>>>
>>> Thank you,
>>>
>>> Fred

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

Re: RFR: JDK-8172791 fixing issues with Reserved Stack Area

Tobias Hartmann-2
In reply to this post by frederic parain
Hi Fred,

the changes look reasonable to me.

Please run 'hs-precheckin-comp' before pushing.

Thanks,
Tobias

On 18.04.2017 16:47, Frederic Parain wrote:

> Greetings,
>
> Please review this fix which addresses several issues with the ReservedStackArea implementation:
>   1 - the method look_for_reserved_stack_annotated_method() fails to detect in-lined
>        annotated methods, making the annotation ineffective for in-lined methods
>   2 - sometime an assertion failure related to reserved area status occurs after incorrect
>        restoring of guards pages during a return from runtime
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8172791
>
> webrev:
> http://cr.openjdk.java.net/~fparain/8172791/webrev.00/index.html
>
> This fix has been contributed by Andrew Haley.
>
> Thank you,
>
> Fred
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: JDK-8172791 fixing issues with Reserved Stack Area

frederic parain
Tobias,

Thank you for the review.

I'll run 'hs-precheckin-comp' before pushing.

Regards,

Fred

On 07/06/2017 10:45 AM, Tobias Hartmann wrote:

> Hi Fred,
>
> the changes look reasonable to me.
>
> Please run 'hs-precheckin-comp' before pushing.
>
> Thanks,
> Tobias
>
> On 18.04.2017 16:47, Frederic Parain wrote:
>> Greetings,
>>
>> Please review this fix which addresses several issues with the ReservedStackArea implementation:
>>    1 - the method look_for_reserved_stack_annotated_method() fails to detect in-lined
>>         annotated methods, making the annotation ineffective for in-lined methods
>>    2 - sometime an assertion failure related to reserved area status occurs after incorrect
>>         restoring of guards pages during a return from runtime
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8172791
>>
>> webrev:
>> http://cr.openjdk.java.net/~fparain/8172791/webrev.00/index.html
>>
>> This fix has been contributed by Andrew Haley.
>>
>> Thank you,
>>
>> Fred
Loading...