[ping] RFR(S): 8156992: [ppc] Implement template interpreter stack overflow checks as on x86/sparc.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Jun 1 19:50:41 UTC 2016
Hi Coleen,
thanks a lot for reviewing and sponsoring!
Best regards,
Goetz.
> -----Original Message-----
> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> Sent: Wednesday, June 01, 2016 7:32 PM
> 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.
>
>
>
> On 6/1/16 11:28 AM, Lindenmaier, Goetz wrote:
> > 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
>
> Okay. Add to part of your RFE to use _stack_overflow_limit for all
> platforms to figure out why this is and then they'll all be the same.
> Better to keep it consistent for now.
> >
> > 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
>
> I was wondering about this. Luckily someone left a comment in the x86
> code.
>
> // native calls don't need the stack size check since they have no
> // expression stack and the arguments are already on the stack and
> // we only add a handful of words to the stack
>
> I see this change.
>
> This looks good to me. I'll sponsor it.
>
> Coleen
> >
> > 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.
More information about the hotspot-runtime-dev
mailing list