RFR(S): 8156922: [ppc] Implement template interpreter stack overflow checks as on x86/sparc.

[dean.long at oracle.com]

On 5/20/16 12:28 AM, Lindenmaier, Goetz wrote:
> Hi Dean,
>
>> OK, that helps, but also reinforces the idea that this code is more
>> complicated that I thought.
> Yes :)
>
>> Let me ask a simpler question.  What happens if this first pointer
>> compare check simply uses
>> stack_guard_zone_size() instead of MAX2(stack_shadow_zone_size(),
>> stack_guard_zone_size())?
>> It seems the worst that can happen is that the first check succeeds more
>> often, so we detect the stack
>> overflow with the stack bang and the signal handler instead.
> I think the second check, the banging, might not touch all pages in
> that case. But I think this could also happen if the frame >> shadow zone,
> as it's pushed already.
>
> I think a good implementation in the template interpreter would be
>    - do all the checking before pushing the frame
>    - first compare whether sp+newframe < stack_end + guardzone  + shadowzone,
>      i.e., a bang would hit the yellow zone.
>   - if the compare fails, push a dummy frame, and touch the yellow page --> signal handler
>   - if the compare succeeds, bang the pages so they are committed (might only be needed
>      on windows.)
> This would give a similar behavior as in the compiler.  Also, it makes it more easy to
> document and configure the different stack zones if it's handled the same everywhere.
>
> But actually, I wondered about the implementation and had hoped to get an
> answer to this on the mailing list.  Also, this is an interesting issue, but not
> topic of my change :)

OK, looking at just your change, I had the following comments. Overall, 
the changes look reasonable
to me, for pcc.  But it seems that after this change, 
_stack_overflow_limit is only used by ppc.
- Do you think it should be made a cpu-specific field?  Or do you think 
x86/sparc should use it as well?
- Do you think its value should be updated when the yellow/reserved 
zones are unprotected?

dl

PS - I'm not a Reviewer and don't focus on runtime.  It would be good to 
get input from people who understand the history of this code.

> Best regards,
>    Goetz.
>
>> dl
>>
>> On 5/19/16 12:34 PM, Lindenmaier, Goetz wrote:
>>> Hi Dean,
>>>
>>>> Hi Goetz.  Thanks for the explanation.  So using the shadow size in MAX2
>> is so that the
>>>> stack bang that comes next does not "overshoot" the red zone and stomp
>> on memory
>>>> that might be mapped for other purposes?  That makes sense if
>> bang_stack_shadow_pages()
>>>> was only banging one page, but when called from
>> generate_normal_entry(), "native_call" is
>>>> set to false, so we bang from 1 to n_shadow_pages.
>>> As I understand, we need to bang the space for the new frame on top of
>> the old shadow
>>> zone.  The old (caller's) shadow zone is 'clean'.  Not so if there are native
>> frames below
>>> us, they did not bang.  So in that case we have to bang all pages
>> (shadow+new frame).  See also
>>> AbstractAssembler::generate_stack_overflow_check().
>>> But it's not handled consistently.   It would be good if
>>> AbstractAssembler::generate_stack_overflow_check()
>>> (and only that) was called from the template interpreter.
>>>
>>>> If I'm now understanding the code
>>>> correctly, there may be an opportunity for a performance improvement
>> here, by banging
>>>> just 1 page instead of n_shadow_pages.
>>> One bang could always skip over the protected zones.  If you use a
>> compare, you could
>>> get along with one compare.
>>> But there is also the issue that the pages have to be commited, so
>>> you need to touch all of them. Else you think there
>>> is stack, and then you get out of memory or the like.  That's not supposed
>> to happen.
>>>> I reread the description in 8156922, and I'm having troubling following the
>> note that
>>>> starts with "The x86/sparc design can not handle frames bigger than ...."
>> Is the that
>>>> "overshoot" problem I described above?
>>> That's for the first check for only the frame.  Assume
>> yellow=red=reserved=shadow=1page=4K.
>>> The MAX2 is the 12K, leaving just the shadow zone=4k in the first check.
>>> Only if the frame is < 4K you will get the real overflow handling through
>> signal handler
>>> and utilizing the yellow zone / reserved zone after the stack bang. If the
>> frame
>>> is 5K you run into the special overflow at the beginning.
>>>
>>> Best regards,
>>>    Goetz.
>>>
>>>
>>> thanks,
>>>
>>> dl
>>>
>>> On 5/19/16 6:38 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> reading the mail again I think I understand your question better now:
>>>
>>> I think this is because we first want to push the frame of the called method
>>> and then do the real stack bang.
>>> But at least it must be checked that this frame fits on the stack below the
>> yellow zone.
>>> Obviously the person who implemented this also wanted to make sure that
>> a
>>> stack bang on top of this frame does not exceed stack_end(), therefore
>> the MAX2.
>>> If it does not fit, a stack overflow error is thrown without going through the
>>> signal handler - only the space of the shadow zone is used.
>>>
>>> If the frame fits, the real stack bang is executed. The frames below are all
>> fine, thus
>>> the signal handler and code on top can walk the stack.
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>
>>>
>>> From: dean.long at oracle.com [mailto:dean.long at oracle.com]
>>> Sent: Thursday, May 19, 2016 2:15 AM
>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>>> Subject: Re: RFR(S): 8156992: [ppc] Implement template interpreter stack
>> overflow checks as on x86/sparc.
>>> By changing the value of the _stack_overflow_limit field, does the
>> following x86/sparc comment
>>> still make sense?
>>>
>>> // TODO: rather than touching all pages, check against stack_overflow_limit
>> and bang yellow page to
>>> // generate exception
>>>
>>> Also, I have to admit that I don't understand why we use different values
>> for stack checks/bang in different places, especially the using MAX2 instead
>> of +.  Can anyone explain this?  The + makes sense to me, but the MAX2
>> does not.
>>> dl
>>> On 5/18/16 3:02 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>>
>>>
>>> When porting the template interpreter, we implemented a different
>> approach to
>>> Stack overflow handling.  See also the detailed description in the Jira bug.
>>>
>>>
>>>
>>> This change implements the stack overflow check as on x86/sparc.
>>>
>>> It requires simple shared changes, but only to code only used on ppc.
>>>
>>> The changes should not affect the other platforms.
>>>
>>>
>>>
>>> Please review this change. I please need a sponsor.
>>>
>>> http://cr.openjdk.java.net/~goetz/wr16/8156922-ppcStackFix/webrev.01/
>>>
>>>
>>>
>>> Best regards,
>>>
>>>     Goetz.
>>>
>>>
>>>