RFR(M): 8159335: Fix problems with stack overflow handling.
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Jun 17 12:13:29 UTC 2016
Hi Goetz,
I'm done reviewing and am deferring any additional work to our other
platforms to another time. It also passes all of our nightly tests.
I'll sponsor when you upload the patch with the changeset.
Thanks!
Coleen
On 6/16/16 4:56 AM, Lindenmaier, Goetz wrote:
> Hi Colleen,
>
> thanks for looking this closely at my change. I did the below
> edits. I'll upload a new webrev once you are done reviewing, ok?
>
> Best regards,
> Goetz
>
>
> diff -r 306e3d472b61 src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp
> --- a/src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp Mon Jun 13 09:28:25 2016 +0200
> +++ b/src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp Thu Jun 16 10:44:01 2016 +0200
> @@ -526,11 +526,11 @@
>
> // locals + overhead, in bytes
> __ mov(rax, rdx);
> - __ shlptr(rax, Interpreter::logStackElementSize); // 2 slots per parameter.
> + __ shlptr(rax, Interpreter::logStackElementSize); // Convert parameter count to bytes.
> __ addptr(rax, overhead_size);
>
> #ifdef ASSERT
> - Label limit_okay, stack_size_okay;
> + Label limit_okay;
> // Verify that thread stack overflow limit is non-zero.
> __ cmpptr(stack_limit, (int32_t)NULL_WORD);
> __ jcc(Assembler::notEqual, limit_okay);
> @@ -933,7 +933,7 @@
> __ load_unsigned_short(t, Address(t, ConstMethod::size_of_parameters_offset()));
>
> #ifndef _LP64
> - __ shlptr(t, Interpreter::logStackElementSize);
> + __ shlptr(t, Interpreter::logStackElementSize); // Convert parameter count to bytes.
> __ addptr(t, 2*wordSize); // allocate two more slots for JNIEnv and possible mirror
> __ subptr(rsp, t);
> __ andptr(rsp, -(StackAlignmentInBytes)); // gcc needs 16 byte aligned stacks to do XMM intrinsics
>
>
>
>
>> -----Original Message-----
>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>> bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
>> Sent: Donnerstag, 16. Juni 2016 03:09
>> To: hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR(M): 8159335: Fix problems with stack overflow handling.
>>
>>
>> Also,
>>
>> http://cr.openjdk.java.net/~goetz/wr16/8159335-
>> stkOvrflw/webrev.01/src/cpu/x86/vm/templateInterpreterGenerator_x86.c
>> pp.udiff.html
>>
>> + Label limit_okay, stack_size_okay;
>>
>>
>> stack_size_okay is unused.
>>
>> Coleen
>>
>>
>> On 6/15/16 7:33 PM, Coleen Phillimore wrote:
>>> Hi,
>>> I've reviewed this in detail for x86 (less so for the others but the
>>> concept is good). It unifies the calculation for the interpreter
>>> checking for stack overflow.
>>>
>>> - __ shlptr(rax, Interpreter::logStackElementSize); // 2 slots per
>>> parameter.
>>> + __ shlptr(rax, Interpreter::logStackElementSize); // convert
>>> parameter count to bytes
>>>
>>> There are two of these comments "2 slots per parameter" which are
>>> leftover from TaggedStackInterpreter days. Can you make this change
>>> to both?
>>>
>>> I'm implementing this on one of our closed platforms and testing now.
>>>
>>> Thanks! It's nice to make progress on these buggy and inconsistent
>>> calculations.
>>>
>>> Coleen
>>>
>>>
>>> On 6/14/16 3:47 PM, Lindenmaier, Goetz wrote:
>>>> Hi Coleen,
>>>>
>>>> Thanks for offering to sponsor this!
>>>> My last edits are really minor, see that file linked below.
>>>>
>>>> Best regards,
>>>> Goetz
>>>>
>>>>> -----Original Message-----
>>>>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>>>>> bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
>>>>> Sent: Tuesday, June 14, 2016 6:07 PM
>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: Re: RFR(M): 8159335: Fix problems with stack overflow
>>>>> handling.
>>>>>
>>>>>
>>>>> Hi, I haven't had a chance to look at this latest change but I will
>>>>> sponsor it.
>>>>>
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>> On 6/14/16 7:51 AM, Lindenmaier, Goetz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> i found vm_default_page_size() on linux/aix is unused now,
>>>>>> and stack_page_size on aix was never used. I removed this
>>>>>> dead coding and updated the webrev in-place.
>>>>>>
>>>>>> Incremental, non-aix diff:
>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8159335-
>> stkOvrflw/webrev.01/src/os/linux/vm/os_linux.hpp.udiff.html
>>>>>>
>>>>>> Best regards,
>>>>>> Goetz.
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Lindenmaier, Goetz
>>>>>>> Sent: Montag, 13. Juni 2016 13:33
>>>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>>>> Subject: RFR(M): 8159335: Fix problems with stack overflow handling.
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> During porting JEP 270 to ppc I detected a row of bugs in the stack
>>>>> overflow
>>>>>>> coding.
>>>>>>>
>>>>>>> This change fixes these.
>>>>>>>
>>>>>>> Please review these changes. I please need a sponsor.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8159335-
>>>>>>> stkOvrflw/webrev.01/index.html
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> javaCalls.cpp:
>>>>>>>
>>>>>>> Windows calls map_stack_shadow_pages() after doing
>>>>>>> os::stack_shadow_pages_available().
>>>>>>>
>>>>>>> Both functions read the stack pointer, but this might result in
>>>>>>>
>>>>>>> different sp's. In consequence, map_stack_shadow_pages() can hit
>>>>>>>
>>>>>>> a guard page although the previous check succeeded.
>>>>>>>
>>>>>>> So I pass in the sp so we can assure we use the same in both. Also,
>>>>>>>
>>>>>>> I optimized the map_stack_shadow_pages() on windows, we saw bad
>>>>>>>
>>>>>>> assembly for it.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> os_<os>.cpp:
>>>>>>>
>>>>>>> fix bug & Streamline computation of min_stack_allowed.
>>>>>>>
>>>>>>> - In some OS'es it was forgotten to consider the reserved pages.
>>>>>>>
>>>>>>> - OS'es use different multiplicators (aix: 8K, bsd: system page size,
>>>>>>>
>>>>>>> linux: 8K, solaris: system page size (often 8K), win: system
>>>>>>> page size)
>>>>>>>
>>>>>>> --> Use 8K everywhere, but I noted it down with *4K as zone
>>>>>>>
>>>>>>> sizes are also given in 4K units.
>>>>>>>
>>>>>>> - min_stack_allowed should be page aligned, else the warning is
>>>>>>>
>>>>>>> misleading.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> templateTable_ppc_64.cpp:
>>>>>>>
>>>>>>> Fix bug on ppc: in monitorenter() the wrong stack bang mechanism
>>>>>>>
>>>>>>> was used.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> globals_<cpu>.hpp:
>>>>>>>
>>>>>>> Streamline ppc stack guard sizes. The current ppc sizes are not
>>>>>>>
>>>>>>> adapted to openJdk. There SocketOutputStream.socketWrite0()
>>>>>>>
>>>>>>> uses a large buffer, so shadow pages have to be increased.
>>>>>>>
>>>>>>> This makes no big difference as many ppc systems have 64K
>>>>>>>
>>>>>>> pages, and the zone sizes are adapted to page sizes.
>>>>>>>
>>>>>>> Because zone sizes are calculated based on 4K pages now,
>>>>>>>
>>>>>>> the shadow zone on sparc has to be increased, too.
>>>>>>>
>>>>>>> Sparc uses 8K pages per default (as far as I konw),
>>>>>>>
>>>>>>> which was the old multiplier (wrong since 8139864).
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> zone sizes:
>>>>>>>
>>>>>>> red yellow res shadow
>>>>>>>
>>>>>>> aarch 1 2 0 4+5
>>>>>>>
>>>>>>> ppc 1 6 1 6+2 --> 1 / 2 / 1 / 20+2
>>>>>>>
>>>>>>> sparc 1 2 1 10+1 --> 1 / 2 / 1 / 20+2
>>>>>>>
>>>>>>> spac32 1 2 1 3+1 --> 1 / 2 / 1 / 6+2
>>>>>>>
>>>>>>> x86_64 1 2 1 20+2
>>>>>>>
>>>>>>> x86_32 1 2 1 4+5
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> win
>>>>>>>
>>>>>>> x86_64 1 3 0 6+2
>>>>>>>
>>>>>>> x86_32 1 3 0 4+5
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> templateInterpreterGeneratore_<cpu>.cpp:
>>>>>>>
>>>>>>> Simplify checking whether the new frame fits on the stack.
>>>>>>>
>>>>>>> Use JavaThread->_stack_overflow_limit on all cpus. This
>>>>>>>
>>>>>>> saves several instructions.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I also have to increase the stack sizes of some tests. They fail
>>>>>>> because
>>>>>>>
>>>>>>> systems with 64K pages require five of them for the shadow/guard
>>>>>>> area.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I ran this in our night build for a few days, so it's tested on
>>>>>>> linuxx86_64,
>>>>>>>
>>>>>>> Ppc aix, linux and linux-le, sparc and darwinintel. Among others,
>>>>>>> all
>>>>> hotspot
>>>>>>> Jtreg tests and a lot of jck tests have been executed.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Best regards,
>>>>>>>
>>>>>>> Goetz.
>>>>>>>
>>>>>>>
More information about the hotspot-runtime-dev
mailing list