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/
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.par...@oracle.com
