RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections
Frederic Parain
frederic.parain at oracle.com
Tue Dec 1 16:21:47 UTC 2015
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.Parain at oracle.com
More information about the core-libs-dev
mailing list