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 <daniel.daugherty at oracle.com> 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