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

Wed Jun 1 13:16:13 UTC 2016

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.