RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections
Frederic Parain
frederic.parain at oracle.com
Thu Dec 3 14:15:39 UTC 2015
All fixed.
Thank you Dan.
Fred
On 02/12/2015 19:22, Daniel D. Daugherty wrote:
>
> On 12/1/15 9:21 AM, Frederic Parain wrote:
>> Hi Dan,
>>
>> Thank you for your detailed review.
>> My answers are in-lined below.
>>
>> New webrev:
>>
>> http://cr.openjdk.java.net/~fparain/8046936/webrev.02/hotspot/
>
> Re-reviewed by comparing 8046936.0[12].hotspot.patch in jfilemerge...
>
> Just a couple of nits:
>
> src/os/windows/vm/os_windows.cpp
> L2365: assert(fr->safe_for_sender(thread), "Safety check");
> Wrong indent; should be 6 spaces instead of 8; actually I think
> this one is a tab.
>
> src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
> L381: assert(fr->safe_for_sender(thread), "Safety check");
> Wrong indent; this one also might be a tab
>
> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
> L194: assert(fr->safe_for_sender(thread), "Safety check");
> Wrong indent; this one also might be a tab
>
> src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
> L267: assert(fr->safe_for_sender(thread), "Safety check");
> Wrong indent; this one also might be a tab
>
> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
> L255: assert(fr->safe_for_sender(thread), "Safety check");
> Wrong indent; this one also might be a tab
>
>
> Thumbs up! I do not need to see a webrev for the above nits.
>
> Dan
>
>
>>
>>
>> On 24/11/2015 17:26, Daniel D. Daugherty wrote:
>>>
>>> src/cpu/sparc/vm/frame_sparc.cpp
>>> (old) L635: if (fp() - sp() > 1024 +
>>> m->max_stack()*Interpreter::stackElementSize) {
>>> (new) L635: if (fp() - unextended_sp() > 1024 +
>>> m->max_stack()*Interpreter::stackElementSize) {
>>> This looks like a bug fix independent of this fix.
>>
>> Correct, this is the SPARC version of JDK-8068655.
>>
>>> src/share/vm/runtime/thread.hpp
>>> L953: intptr_t* _reserved_stack_activation;
>>> L1382: intptr_t* reserved_stack_activation() const { return
>>> _reserved_stack_activation; }
>>> L1383: void set_reserved_stack_activation(intptr_t* addr) {
>>>
>>> I was expecting this type to be 'address' instead of
>>> 'intptr_t*'.
>>>
>>> Update: I've gone back through the changes and I still don't
>>> see a reason that this is 'intptr_t*'.
>>
>> The _reserved_stack_activation has been declared as an 'intptr_t*'
>> just to be consistent with the _sp and _fp fields of the frame class.
>> However, this is not really a requirement, the content stored at the
>> _reserved_stack_activation address is never read. This address is just
>> a "marker" on the stack to quickly check if the thread has exited the
>> annotated code section or not. I've change the type to address, there's
>> slightly less casts, and it doesn't impact the ReservedStackArea logic.
>>
>> Note: I've removed all further comments about _reserved_stack_activation
>> type in order to improve the e-mail readability.
>>
>>> L1341: { return stack_yellow_zone_base();}
>>> '{' should be at the end of the previous line.
>>> Missing space after ';'.
>>
>> Fixed
>>
>>> L1343: { return StackReservedPages * os::vm_page_size(); }
>>> '{' should be at the end of the previous line.
>>
>> Fixed
>>
>>> src/share/vm/runtime/thread.cpp
>>> L2543: // The base notation is from the stacks point of view,
>>> growing downward.
>>> L2565: // The base notation is from the stacks point of view,
>>> growing downward.
>>> Typo: "stacks point of view" -> "stack's point of view"
>>
>> Fixed
>>
>>> L2552: } else {
>>> L2553: warning("Attempt to guard stack reserved zone failed.");
>>> L2554: }
>>> L2555: enable_register_stack_guard();
>>>
>>> Should enable_register_stack_guard() be called when we issued
>>> the warning on L2553?
>>>
>>> L2571: } else {
>>> L2572: warning("Attempt to unguard stack reserved zone
>>> failed.");
>>> L2573: }
>>> L2574: disable_register_stack_guard();
>>>
>>> Should disable_register_stack_guard() be called when we issued
>>> the warning on L2572?
>>
>> enable_register_stack_guard() and disable_register_stack_guard() are
>> relics of the Itanium code (Itanium had a very different stack
>> management). These methods are currently empty on all OpenJDK and
>> Oracle platforms. May be another clean up opportunity here.
>> Regarding the placement of the calls, I followed the same pattern
>> as the other red/yellow pages management functions.
>>
>>> src/share/vm/runtime/sharedRuntime.cpp
>>>
>>> L784: java_lang_Throwable::set_message(exception_oop,
>>> L785: Universe::delayed_stack_overflow_error_message());
>>> Wrong indent; this should line up under the 'e' after the '('.
>>
>> Fixed
>>
>>> L2976: if (fr.is_interpreted_frame()) {
>>> L2978: prv_fr = fr;
>>> L2982: prv_fr = fr;
>>> This line is in both branches of the if-statement on L2976.
>>> Is there a reason not to save prv_fr before L2976?
>>
>> No particular reason, fixed.
>>
>>> L2996 continue;
>>> Wrong indent; needs one more space.
>>
>> Fixed
>>
>>> L2958: frame activation;
>>> L3013: return activation;
>>> The return on L3013 can return a default constructed 'frame'.
>>> Is that default safe to return here?
>>
>> Yes, the caller performs a check before using the returned
>> frame:
>> if (activation.sp() != NULL) { ...
>>
>>>
>>> src/os/bsd/vm/os_bsd.hpp
>>> L109: static bool get_frame_at_stack_banging_point(JavaThread*
>>> thread, ucontext_t* uc, frame* fr);
>>> Wrong indent; needs one less space.
>>
>> Fixed
>>
>>> src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
>>> L322: // For Forte Analyzer AsyncGetCallTrace profiling support -
>>> thread
>>> L323: // is currently interrupted by SIGPROF.
>>> Now fetch_frame_from_ucontext() is also used for stack overflow
>>> signal handling.
>>
>> Fixed
>>
>>> L379: assert(fr->safe_for_sender(thread), "Safety check");
>>> L380: if (!fr->is_first_java_frame()) {
>>> L381: *fr = fr->java_sender();
>>> The assert() on L379 should be before the java_sender()
>>> call on L381.
>>
>> Fixed
>>
>>> src/os/linux/vm/os_linux.cpp
>>> L1902: jt->stack_guards_enabled()) { // No pending
>>> stack overflow exceptions
>>> This line's comment used to align with the previous line's
>>> comment.
>>> Can you move the previous line's comment to align with this
>>> one?
>>
>> Done.
>>
>>> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>>> L135: // For Forte Analyzer AsyncGetCallTrace profiling support -
>>> thread
>>> L136: // is currently interrupted by SIGPROF.
>>> Now fetch_frame_from_ucontext() is also used for stack overflow
>>> signal handling.
>>
>> Fixed
>>
>>> L192: assert(fr->safe_for_sender(thread), "Safety check");
>>> L193: if (!fr->is_first_java_frame()) {
>>> L194: *fr = fr->java_sender();
>>> The assert() on L192 should be before the java_sender()
>>> call on L194.
>>
>> Fixed
>>
>>> src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
>>> L209: // For Forte Analyzer AsyncGetCallTrace profiling support -
>>> thread
>>> L210: // is currently interrupted by SIGPROF.
>>> Now fetch_frame_from_ucontext() is also used for stack overflow
>>> signal handling.
>>
>> Fixed
>>
>>> L265: assert(fr->safe_for_sender(thread), "Safety check");
>>> L266: if (!fr->is_first_java_frame()) {
>>> L267: *fr = fr->java_sender();
>>> The assert() on L265 should be before the java_sender()
>>> call on L267.
>>
>> Fixed
>>
>>> L279: //assert(fr->safe_for_sender(thread), "Safety check");
>>> Delete this line; you copied it to L282.
>>
>> Done
>>
>>> L287 return true;
>>> Should this assert be added above this line?
>>> assert(fr->is_java_frame(), "Safety check");
>>
>> Yes, this assert exists on other platforms, and there's no
>> reason to omit it on Solaris/SPARC
>>
>>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>>> L195: // For Forte Analyzer AsyncGetCallTrace profiling support -
>>> thread
>>> L196: // is currently interrupted by SIGPROF.
>>> Now fetch_frame_from_ucontext() is also used for stack overflow
>>> signal handling.
>>
>> Fixed
>>
>>> L253: assert(fr->safe_for_sender(thread), "Safety check");
>>> L254: if (!fr->is_first_java_frame()) {
>>> L255: *fr = fr->java_sender();
>>> The assert() on L253 should be before the java_sender()
>>> call on L255.
>>
>> Fixed
>>
>>> L273: *fr = fr->java_sender();
>>> Wrong indent; one too many spaces.
>>
>> Fixed
>>
>>
>>> src/os/windows/vm/os_windows.cpp
>>> L2364: assert(fr->safe_for_sender(thread), "Safety check");
>>> L2365: if (!fr->is_first_java_frame()) {
>>> L2366: *fr = fr->java_sender();
>>> The assert() on L2364 should be before the java_sender()
>>> call on L2366.
>>
>> Fixed
>>
>>> L2387: return true;
>>> Should this assert be added above this line?
>>> assert(fr->is_java_frame(), "Safety check");
>>
>> Certainly, fixed.
>>
>>> src/share/vm/oops/method.hpp
>>> (old) L87: u1 _flags;
>>> (new) L88: u2 _flags;
>>> Ouch - just needed one more bit...
>>
>> The initial implementation of the reserved stack area used the last
>> bit, but unfortunately, someone else steal it before I could push
>> my code :-( So I had to extend the flags field
>>
>>> L834: return (_flags & _reserved_stack_access) != 0;
>>> Wrong indent; two fewer spaces.
>>
>> Fixed
>>
>>> src/share/vm/runtime/globals.hpp
>>> L3549: range(MIN_STACK_RESERVED_PAGES,
>>> (DEFAULT_STACK_RESERVED_PAGES+10))\
>>> Wrong indent; should line up below the double quote in
>>> the previous line.
>>
>> Fixed
>>
>>> src/share/vm/interpreter/interpreterRuntime.cpp
>>> L328 IRT_ENTRY(void,
>>> InterpreterRuntime::throw_delayed_StackOverflowError(JavaThread*
>>> thread))
>>>
>>> The regular throw_StackOverflowError() increments
>>> a counter:
>>>
>>> L313: Atomic::inc(&Exceptions::_stack_overflow_errors);
>>>
>>> Should this function increment the same counter or
>>> a different counter?
>>
>> Good catch! I've added the counter increment to the method
>> throw_delayed_StackOverflowError(). I don't see a strong
>> rational to create a new counter for delayed stack overflows,
>> so it increments the same counter as throw_StackOverflowError().
>>
>>>
>>> src/cpu/sparc/vm/macroAssembler_sparc.hpp
>>> L1423: // Check for reserved stack access in method being exited
>>> (for the compilers)
>>> The X86 version says "for JIT compilers". I prefer "JIT".
>>
>> Fixed
>>
>>> src/cpu/x86/vm/macroAssembler_x86.hpp
>>> L643: // Check for reserved stack access in method being exited
>>> (for JIT compilers)
>>> The SPARC version says "for the compilers".
>>
>> Fixed
>>
>>> src/share/vm/ci/ciMethod.cpp
>>> L95: _has_reserved_stack_access =
>>> h_m()->has_reserved_stack_access();
>>> Wrong indent; should be only one space before '='.
>>
>> Fixed
>>
>>> src/share/vm/c1/c1_GraphBuilder.cpp
>>> L3323: if(callee->has_reserved_stack_access()) {
>>> L3336: if(callee->has_reserved_stack_access()) {
>>> L3356: if(callee->has_reserved_stack_access()) {
>>> Missing space between 'if' and '('.
>>
>> All fixed.
>>
>>> src/cpu/x86/vm/x86_32.ad
>>> L737: size += 64; // added to support ReservedStackAccess
>>> Usually I hate literals like this, but this function has
>>> them in spades. :-(
>>
>> I agree but I didn't find a better solution.
>>
>>> src/cpu/x86/vm/x86_64.ad
>>> L960: MacroAssembler _masm(&cbuf);
>>> L965: MacroAssembler _masm(&cbuf);
>>>
>>> I think you can delete the _masm on L965.
>>
>> Right, removed.
>>
>>> src/share/vm/opto/compile.cpp
>>> L675:
>>> _has_reserved_stack_access(target->has_reserved_stack_access()) {
>>> Wrong indent; should be a single space between ')' and '{'.
>>
>> Fixed
>>
>>> test/runtime/ReservedStack/ReservedStackTest.java
>>> L26: * @run main/othervm -XX:-Inline
>>> -XX:CompileCommand=exclude,java/util/concurrent/locks/AbstractOwnableSynchronizer,setExclusiveOwnerThread
>>>
>>> ReservedStackTest
>>>
>>> Should the comma before 'setExclusiveOwnerThread' be a period?
>>> Perhaps both formats work...
>>
>> Both formats work, but I changed it to be a period, it's cleaner.
>>
>>> L47: * else
>>> Wrong indent; needs one more space before 'else'.
>>>
>>> L52: * successfully update the status of the lock but he method
>>> Typo: 'update the' -> 'updates the'
>>> Typo: 'he method' -> 'the method'
>>>
>>> L60: * first StackOverflowError is thrown, the Error is catched
>>> Typo: 'is catched' -> 'is caught'
>>>
>>> L61: * and a few dozens frames are exited. Now the thread has
>>> Typo: 'a few dozens frames' -> 'a few dozen frames'
>>>
>>> L66: * of its invocation, tries to acquire the next lock
>>> Typo: 'its invocation' -> 'its invocations'
>>>
>>> L81: * stack to prevent false sharing. The test is using this
>>> Perhaps 'The test is using this'
>>> -> 'The test relies on this'
>>>
>>> to better match wording on L225-6.
>>>
>>> L82: * to have different stack alignments and hit the targeted
>>> Grammar: 'and hit' -> 'when it hits'
>>>
>>> L102: * exploit the property that interpreter frames are (much)
>>> Typo: 'exploit' -> 'exploits'
>>> Delete extra space after 'the'.
>>>
>>> L123: //LOCK_ARRAY_SIZE value
>>> Add a space after '//'.
>>>
>>> L188: @jdk.internal.vm.annotation.ReservedStackAccess
>>> This isn't privileged code and -XX:-RestrictReservedStack
>>> isn't specified so what does this do?
>>
>> It checks that by default the annotation is ignored for non-privileged
>> code, in case it is not ignored, the test would fail.
>>
>>>
>>> L201: System.exit(-1);
>>> Wrong indent; needs two more spaces.
>>
>> All fixed.
>>
>> Thank you very much!
>>
>> Fred
>>
>>>>
>>>> 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,
>>>>
>>>> Fred
>>>>
>>>
>>
>
--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com
More information about the core-libs-dev
mailing list