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

[Lindenmaier, Goetz]

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 :)

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.
> >
> >
> >