RFR(M): 8159335: Fix problems with stack overflow handling.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Jun 17 13:00:07 UTC 2016
Hi Coleen,
thanks for reviewing and sponsoring!
Here is the new webrev:
http://cr.openjdk.java.net/~goetz/wr16/8159335-stkOvrflw/webrev.02/
Best regards,
Goetz.
> -----Original Message-----
> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> Sent: Freitag, 17. Juni 2016 14:13
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
> dev at openjdk.java.net
> Subject: Re: RFR(M): 8159335: Fix problems with stack overflow handling.
>
> 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