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

Wed Jun 1 00:30:32 UTC 2016

On 5/20/16 3: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 :)

It is very complicated.  Every time I see it, I have to redo the work to 
understand it again.
>
>> 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.

Yes, this would be better. The reason that there were separate checks 
was because the stack bang preceded the generate_stack_overflow_check() 
code, which we found out we needed later for some large frame in the jck 
tests.

This is what the comment in here is trying to say:

// TODO: rather than touching all pages, check against 
stack_overflow_limit and bang yellow page to
// generate exception.  Windows might need this to map the shadow pages 
though.

But I believe linux also has to map the shadow pages.

It would be really nice if all the platforms did the same thing with 
some common code!
>
> 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 :)

I think we've discussed this on the list before and agree this sort of 
consolidation would be good.   This would be a good enhancement for just 
after jdk9, especially since stack overflow handling is fresh in 
people's minds.

Thanks,
Coleen
>
> 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.
>>>
>>>
>>>