RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Dec 2 18:22:18 UTC 2015
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
>>>
>>
>
More information about the core-libs-dev
mailing list