RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Lindenmaier, Goetz]

Hi Frederic,

I'm now again working on my change "8139864: Improve handling of stack protection zones."
Coleen had asked me to wait until this change of yours is submitted.

You changed the stack_yellow_zone accessor functions in thread.hpp to 
handle both zones, yellow and reserved.
Therefore, reading the code, the reserved zone seems to be part of the yellow zone.

In the description of the bug, it says "The new zone defined by the proposed
solution is placed just before the yellow zone." This reads as if the zones are
separate.

Would you mind if I rename the stack_yellow_zone accessor functions to 
stack_yellow_reserved_zone?  This would make clear in the code that this
are two separate zones.

I anyways have to adapt most of the calls to these accessors.

Best regards,
  Goetz.




> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On
> Behalf Of Frederic Parain
> Sent: Montag, 14. Dezember 2015 14:24
> To: Karen Kinnear <karen.kinnear at oracle.com>
> Cc: hotspot-dev at openjdk.java.net; core-libs-dev <core-libs-
> dev at openjdk.java.net>
> Subject: Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical
> Sections
> 
> Karen,
> 
> Thank you for your review.
> 
> Fred
> 
> On 23/11/2015 20:10, Karen Kinnear wrote:
> > Frederic,
> >
> > Looks good.
> >
> > Many thanks.
> > Karen
> >
> >> On Nov 23, 2015, at 12:44 PM, Frederic Parain
> >> <frederic.parain at oracle.com <mailto:frederic.parain at oracle.com>>
> wrote:
> >>
> >> Karen,
> >>
> >> Thank you for your review, my answers are in-lined below.
> >>
> >> New Webrevs (including some fixes suggested by Paul Sandoz):
> >>
> >> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/
> >> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/
> >>
> >> On 20/11/2015 19:44, Karen Kinnear wrote:
> >>> Frederic,
> >>>
> >>> Code review for web revs you sent out.
> >>> Code looks good. I am not as familiar with the compiler code.
> >>>
> >>> I realize you need to check in at least a subset of the
> >>> java.util.concurrent changes for
> >>> the test to work, so maybe I should not have asked Doug about his
> >>> preference there.
> >>> Sorry.
> >>>
> >>> I am impressed you found a way to make a reproducible test!
> >>>
> >>> Comments/questions:
> >>> 1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”
> >>
> >> Fixed
> >>
> >>> 2. IIRC, due to another bug with windows handling of our guard pages,
> >>> this
> >>> is disabled for Windows. Would it be worth putting a comment in the
> >>> bug , 8067946, that once this is fixed, the reserved stack logic on
> >>> windows
> >>> will need testing before enabling?
> >>
> >> More than testing, the implementation would have to be completed on
> >> Windows. I've added a comment to JDK-8067946.
> >>
> >>> 3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
> >>> this is in interpreter code, you explicitly return the Java sender
> >>> of the current frame. I was expecting to see that on Solaris_sparc
> >>> and Windows
> >>> as well? I do see the assertion in caller that you do have a java frame.
> >>
> >> It doesn't make sense to check the current frame (no bytecode has been
> >> executed yet, so risk of partially executed critical section). I did the
> >> change but not for all platforms. This is now fixed for Solaris_SPARC
> >> and Windows too.
> >>
> >>> 4. test line 83 “writtent” -> “written”
> >>
> >> Fixed
> >>
> >>> 5. I like the way you set up the preallocated exception and then set
> >>> the message - we may
> >>> try that for default methods in future.
> >>>
> >>> 6. I had a memory that you had found a bug in safe_for_sender - did
> >>> you already check that in?
> >>
> >> I've fixed x86 platforms in JDK-8068655.
> >> I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
> >> I had hoped it would have been silently accepted :-)
> >>
> >>
> >>> 7. I see the change in trace.xml, and I see an include added to
> >>> SharedRuntime.cpp,
> >>> but I didn’t see where it was used - did I just miss it?
> >>
> >> trace.xml changes define a new event.
> >> This event is created at sharedRuntime.cpp::3006 and it is used
> >> in the next 3 lines.
> > Thanks. I must have mistyped when I searched for it.
> >>
> >> Thanks,
> >>
> >> Fred
> >>
> >> --
> >> Frederic Parain - Oracle
> >> Grenoble Engineering Center - France
> >> Phone: +33 4 76 18 81 17
> >> Email:Frederic.Parain at oracle.com <mailto:Frederic.Parain at oracle.com>
> >
> 
> --
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: Frederic.Parain at oracle.com