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

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Nov 24 16:26:03 UTC 2015


On 11/23/15 10:44 AM, Frederic Parain 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/

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.


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*'.

     L1341:     { return stack_yellow_zone_base();}
         '{' should be at the end of the previous line.
         Missing space after ';'.

     L1343:     { return StackReservedPages * os::vm_page_size(); }
         '{' should be at the end of the previous line.

src/share/vm/runtime/thread.inline.hpp
     No comments.

src/share/vm/runtime/thread.cpp
     L307:     ((JavaThread*) 
this)->set_reserved_stack_activation((intptr_t*)stack_base());
     L1561:     if (reserved_stack_activation() != 
(intptr_t*)stack_base()) {
     L1562: set_reserved_stack_activation((intptr_t*)stack_base());
     L1565: set_reserved_stack_activation((intptr_t*)stack_base());

         I was expecting this type to be 'address' instead of 'intptr_t*'.

         Update: Still don't know why.

     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"

     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?

src/share/vm/runtime/sharedRuntime.hpp
     No comments.

src/share/vm/runtime/sharedRuntime.cpp
     L488:       if (thread->reserved_stack_activation() != (intptr_t*) 
thread->stack_base()) {
     L489: thread->set_reserved_stack_activation((intptr_t*) 
thread->stack_base());
     L2954: 
thread->set_reserved_stack_activation((intptr_t*)thread->stack_base());

         I was expecting this type to be 'address' instead of 'intptr_t*'.

         Update: Still don't know why.

     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 '('.

     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?

     L2996          continue;
         Wrong indent; needs one more space.

     L2958:   frame activation;
     L3013:   return activation;
         The return on L3013 can return a default constructed 'frame'.
         Is that default safe to return here?


src/share/vm/runtime/deoptimization.cpp
     No comments.

src/share/vm/runtime/javaCalls.cpp
     No comments.

src/share/vm/runtime/os.hpp
     No comments.

src/share/vm/runtime/os.cpp
     No comments.

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.

src/os/bsd/vm/os_bsd.cpp
     No comments.

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.

     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.

src/os/linux/vm/os_linux.hpp
     No comments.

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?

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.

     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.

src/os/solaris/vm/os_solaris.hpp
     No comments.

src/os/solaris/vm/os_solaris.cpp
     No comments.

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.

     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.

     L279:       //assert(fr->safe_for_sender(thread), "Safety check");
         Delete this line; you copied it to L282.

     L287   return true;
         Should this assert be added above this line?

         assert(fr->is_java_frame(), "Safety check");

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.

     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.

     L273:          *fr = fr->java_sender();
         Wrong indent; one too many spaces.

src/os/windows/vm/os_windows.hpp
     No comments.

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.

     L2387:   return true;
         Should this assert be added above this line?

         assert(fr->is_java_frame(), "Safety check");


src/share/vm/classfile/classFileParser.hpp
     No comments.

src/share/vm/classfile/vmSymbols.hpp
     No comments.

src/share/vm/classfile/classFileParser.cpp
     No comments.

src/share/vm/memory/universe.hpp
     No comments.

src/share/vm/memory/universe.cpp
     No comments.

src/share/vm/oops/method.hpp
     (old) L87:   u1 _flags;
     (new) L88:   u2 _flags;
         Ouch - just needed one more bit...

     L834:       return (_flags & _reserved_stack_access) != 0;
         Wrong indent; two fewer spaces.


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.

src/share/vm/runtime/arguments.cpp
     No comments.

src/cpu/aarch64/vm/globals_aarch64.hpp
     No comments.

src/cpu/sparc/vm/globalDefinitions_sparc.hpp
     No comments.

src/cpu/sparc/vm/globals_sparc.hpp
     No comments.

src/cpu/x86/vm/globalDefinitions_x86.hpp
     No comments.

src/cpu/x86/vm/globals_x86.hpp
     No comments.

src/cpu/zero/vm/globals_zero.hpp
     No comments.


src/share/vm/interpreter/interpreterRuntime.hpp
     No comments.

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?

src/cpu/sparc/vm/interp_masm_sparc.cpp
     No comments.

src/cpu/x86/vm/interp_masm_x86.cpp
     No comments.

src/cpu/x86/vm/templateInterpreter_x86_32.cpp
     No comments.

src/cpu/x86/vm/templateInterpreter_x86_64.cpp
     No comments.

src/share/vm/runtime/stubRoutines.hpp
     No comments.

src/share/vm/runtime/stubRoutines.cpp
     No comments.

src/cpu/x86/vm/stubGenerator_x86_32.cpp
     No comments.

src/cpu/x86/vm/stubGenerator_x86_64.cpp
     No comments.

src/cpu/sparc/vm/stubGenerator_sparc.cpp
     No comments.


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".

src/cpu/sparc/vm/macroAssembler_sparc.cpp
     No comments.

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".

src/cpu/x86/vm/macroAssembler_x86.cpp
     No comments.


src/share/vm/ci/ciMethod.hpp
     No comments.

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 '='.

src/cpu/sparc/vm/c1_LIRAssembler_sparc.cpp
     No comments.

src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
     No comments.

src/share/vm/c1/c1_Compilation.hpp
     No comments.

src/share/vm/c1/c1_Compilation.cpp
     No comments.

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 '('.

src/share/vm/c1/c1_Runtime1.cpp
     No comments.


src/cpu/sparc/vm/sparc.ad
     No comments.

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. :-(

src/cpu/x86/vm/x86_64.ad
     L960:   MacroAssembler _masm(&cbuf);
     L965:     MacroAssembler _masm(&cbuf);

         I think you can delete the _masm on L965.

src/share/vm/opto/compile.hpp
     No comments.

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 '{'.

src/share/vm/opto/parse1.cpp
     No comments.


src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java
     No comments.

src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotVMConfig.java
     No comments.

src/share/vm/jvmci/jvmciRuntime.cpp
     No comments.

src/share/vm/runtime/vmStructs.cpp
     No comments.

src/share/vm/trace/trace.xml
     No comments.

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...

     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?

     L201:               System.exit(-1);
         Wrong indent; needs two more spaces.


> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/

src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
     No comments.

src/java.base/share/classes/java/util/concurrent/locks/ReentrantLock.java
     No comments.

src/java.base/share/classes/jdk/internal/vm/annotation/ReservedStackAccess.java
     No comments.


Dan


>
> 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
>




More information about the core-libs-dev mailing list