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

Wed Jun 1 17:31:40 UTC 2016

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.