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
