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

Wed Jun 1 15:28:37 UTC 2016

Hi Coleen,

I think this is so that if you only bang at the end of the shadow zone,
as done in the native_entry, you can't reach past the guarded zones.
But the native_entry does not use generate_stack_overflow_check() ...

This is one of the oddities I don't really understand. I just use the same
value as on x86/sparc:
templateInterpreterGenerator_x86.cpp:548
templateInterpreterGenerator_sparc.cpp:614

I found I'm doing generate_stack_overflow_check() in the native
entry on ppc, I fixed that:
http://cr.openjdk.java.net/~goetz/wr16/8156922-ppcStackFix/webrev.02/
templateInterpreterGenerator_ppc.cpp:1011

Best regards,
  Goetz.

> -----Original Message-----
> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> Sent: Mittwoch, 1. Juni 2016 15:16
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
> dev at openjdk.java.net
> Subject: Re: [ping] RFR(S): 8156992: [ppc] Implement template interpreter
> stack overflow checks as on x86/sparc.
> 
> 
> I had a question embedded in my last response on this thread.   Why
> don't we compare in generate_stack_overflow_check() to the
> stack_reserved_zone_base().  stack_shadow_pages is something like 20 on
> many platforms and the reserved zone is something like 1+2+3 maybe?
> 
> This first check is to make sure large frames don't overflow into the
> reserved+yellow region.
> 
> So sp + framesize should be > stack_base - reserved+yellow+red zones.
> I thought.
> 
> 
> On 6/1/16 2:54 AM, Lindenmaier, Goetz wrote:
> > Hi Coleen,
> >
> > thanks for looking at this and going through the long Mail thread!
> > I also read your other comments.
> > Altogether I understand the following:
> >
> > - This change is ok for now to get ppc to the same implementation
> >     as the other platforms.
> > - stack_overflow_limit should be used on the other platforms, too.
> >     I'll make a follow up change for this. It's basically an optimization of
> >     the checking and does not change behavior, so it should be fine
> >     to do in jdk9.  I also found some other small optimizations of the
> >     overflow checking in our code I'd like to include in that change.
> 
> Yes, I'll do an FC exception request if you make this cleanup, and sponsor.
> > - A compare is needed in the template interpreter because frames
> >    can be bigger than the protected zones.  This is not the case for
> >    compiled frames, where the compiler tailors the banging code
> >    for the frame size.
> > - The compare + banging in the template interpreter should be
> >     done before pushing the frame, and both should go through
> >     the signal handler to have similar behavior as in the compilers.
> >     This is a change that should go to jdk10.
> 
> Agree.  I can help with the testing of this and save for when 10
> repository opens if you make the change.
> > While going through all our stack banging modifications I stumbled
> > over another issue.  SocketOutputStream.socketWrite0()
> > (jdk/src/java.base/unix/native/libnet/SocketOutputStream.c)
> > pushes a 65K buffer on the stack. I think we saw failing jck tests because
> > this reaches past all the stack protection zones.  We first increased
> > the ShadowZone, which lead to overhead in banging. We then
> > optimized banging but last reduced the size of that buffer to 8K.
> > Have you ever seen problems with that?
> 
> Yes, we had a bug because of socketWrite0.   We changed the
> StackShadowPages to 20 for this, which is unfortunate.
> 
> See also:
> 
> https://bugs.openjdk.java.net/browse/JDK-8072070
> https://bugs.openjdk.java.net/browse/JDK-8069196
> 
> I added a stackoverflow label to these bugs.
> 
> Thanks,
> Coleen
> 
> 
> >
> > Best regards,
> >    Goetz.
> >
> >
> >
> >
> >
> >
> >
> >> -----Original Message-----
> >> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> >> bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
> >> Sent: Wednesday, June 01, 2016 2:43 AM
> >> To: hotspot-runtime-dev at openjdk.java.net
> >> Subject: Re: [ping] RFR(S): 8156992: [ppc] Implement template interpreter
> >> stack overflow checks as on x86/sparc.
> >>
> >>
> >>
> >> On 5/30/16 7:23 PM, David Holmes wrote:
> >>> On 30/05/2016 5:28 PM, Lindenmaier, Goetz wrote:
> >>>> Hi David,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>> Sent: Samstag, 28. Mai 2016 00:10
> >>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> >> runtime-
> >>>>> dev at openjdk.java.net; Frederic Parain
> <frederic.parain at oracle.com>
> >>>>> Subject: Re: [ping] RFR(S): 8156992: [ppc] Implement template
> >>>>> interpreter
> >>>>> stack overflow checks as on x86/sparc.
> >>>>>
> >>>>> On 28/05/2016 1:35 AM, Lindenmaier, Goetz wrote:
> >>>>>> Hi David,
> >>>>>>
> >>>>>> well, yes, Dean had similar questions.
> >>>>>> X86/sparc load stack_base and stack_size, compute stack_end and
> >> then
> >>>>> add the
> >>>>>> Max(shadow, quard). All in assembly in the template interpreter
> >>>>>> prolog.
> >>>>> On ppc,
> >>>>>> we load it from the precomputed field stackoverflow_limit.  The
> >>>>>> value in
> >>>>> that field
> >>>>>> differed from what x86/sparc computed, which is the error I want to
> >>>>>> fix
> >>>>> here.
> >>>>>
> >>>>> That all sounds a bit broken. :(
> >>>> Broken as far as my change is concerned, or broken wrt. to the existing
> >>>> implementation?
> >>> The existing implementation.
> >>>
> >>>>>> I adapted the drawing to the change of that field.
> >>>>> I still don't see how the drawing matches the code given the MAX
> usage.
> >>>> If guard_zones > shadow zone, stack_overflow_limit points to
> >>>> stack_reserved_zone_base().
> >>>> If guard_zones < shadow zone, stack_overflow_limit points to
> >>>> somewhere in the shadow zone.
> >>> Ah! I was using frames view on the webrev and the "somewhere in
> here"
> >>> comment was off the page.
> >> I'm having trouble with the same picture too.
> >>
> >> The generate_stack_overflow_check() should be checking that the frame
> >> fits under the stack_guard_zone or stack_reserved_zone_base. Or what
> >> used to be yellow+red zones.
> >>
> >> Why is it
> >>
> >>     // Use the bigger size for banging.
> >>     const int max_bang_size =
> >> (int)MAX2(JavaThread::stack_shadow_zone_size(),
> >> JavaThread::stack_guard_zone_size());
> >>
> >>
> >> Which would put the pointer for _stack_overflow_limit in the
> "somewhere
> >> in here" place.  But I don't get it.
> >>
> >> This comment and variable name is wrong in the x86 code, I think, but
> >> this isn't your problem.
> >>
> >> The code looks good, but we might need a comment.   And we should file
> >> an RFE to use Thread::_stack_overflow_limit for the other platforms.
> >>
> >> Coleen
> >>> Thanks,
> >>> David
> >>>
> >>>> Best regards,
> >>>>    Goetz.
> >>>>
> >>>>> I can't convince myself that any of this stuff is correct. Given
> >>>>> Frederic was working in this area most recently I wonder if he can
> >>>>> comment (but I know he may not be around at the moment).
> >>>>>
> >>>>> Thanks,
> >>>>> David
> >>>>>
> >>>>>> Best regards,
> >>>>>>    Goetz
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>>> Sent: Donnerstag, 26. Mai 2016 00:36
> >>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> >> runtime-
> >>>>>>> dev at openjdk.java.net
> >>>>>>> Subject: Re: [ping] RFR(S): 8156992: [ppc] Implement template
> >>>>>>> interpreter
> >>>>>>> stack overflow checks as on x86/sparc.
> >>>>>>>
> >>>>>>> Hi Goetz,
> >>>>>>>
> >>>>>>> I know you were having discussions with Dean in relation to this
> >>>>>>> but I
> >>>>>>> really don't understand the change to the diagram in thread.hpp
> >>>>>>> nor the
> >>>>>>> change to the definition of stackoverflow_limit. To me the limit
> >>>>>>> has to
> >>>>>>> be the sum of the guard zone and shadow zone. With your MAX
> >> change
> >>>>> the
> >>>>>>> diagram doesn't even make sense AFAICS.
> >>>>>>>
> >>>>>>> ??
> >>>>>>>
> >>>>>>> David
> >>>>>>>
> >>>>>>> On 25/05/2016 6:07 PM, Lindenmaier, Goetz wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> could someone else have a look at this change?  I need a
> reviewer,
> >>>>> please.
> >>>>>>>> I also please need a sponsor.
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>>    Goetz.
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Lindenmaier, Goetz
> >>>>>>>>> Sent: Mittwoch, 18. Mai 2016 12:02
> >>>>>>>>> To: hotspot-runtime-dev at openjdk.java.net
> >>>>>>>>> Subject: RFR(S): 8156992: [ppc] Implement template interpreter
> >>>>>>>>> stack
> >>>>>>>>> overflow checks as on x86/sparc.
> >>>>>>>>>
> >>>>>>>>> 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.