RFR: 8253717: Relocate stack overflow code out of thread.hpp/cpp [v2]

[David Holmes]

On Tue, 6 Oct 2020 19:18:24 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change moves the significant amount of stack overflow related code (with ascii art!) out of thread files into a
>> new file.  Many of the functions are static functions and some go through JavaThread::_stack_overflow_state where
>> needed.   All functions are moved and not modified except for qualification.  I also added a delegating constructor to
>> JavaThread::JavaThread so reordered the assignments as initializers from JavaThread::initialize.
>> Tested with tier1-6 and builds on arm32, ppc, s390 and zero.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8253717: Relocate stack overflow code out of thread.hpp/cpp

Hi Coleen,

The code reorganisation seems okay though I'm not clear on the motivation as stackoverflow protection is a feature of
JavaThreads. I have a couple of minor comments.

Thanks,
David

src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 183:

> 181:   nonstatic_field(JavaThread,                  _should_post_on_exceptions_flag,
> int)                                   \ 182:   nonstatic_field(JavaThread,
> _jni_environment,                              JNIEnv)                                \ 183:
> nonstatic_field(JavaThread,                  _stack_overflow_state._reserved_stack_activation,
> address)                            \

This doesn't look right. I thought these had to be direct entries for fields in given classes - not indirections??

src/hotspot/share/runtime/thread.cpp line 1669:

> 1667:   _on_thread_list(false),
> 1668:   DEBUG_ONLY(_java_call_counter(0) COMMA)
> 1669:   _entry_point(nullptr),

I didn't realize we had made the NULL -> nullptr switch yet ??

src/hotspot/share/runtime/thread.cpp line 2953:

> 2951: // Verification
> 2952:
> 2953: void JavaThread::frames_do(void f(frame*, const RegisterMap* map)) {

Where does this come from ??

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
line 398:

> 396:     public final int jvmciCountersThreadOffset = getFieldOffset("JavaThread::_jvmci_counters", Integer.class,
> "jlong*"); 397:     public final int doingUnsafeAccessOffset = getFieldOffset("JavaThread::_doing_unsafe_access",
> Integer.class, "bool", Integer.MAX_VALUE, JVMCI || JDK >= 14); 398:     public final int
> javaThreadReservedStackActivationOffset = JDK <= 8 ? 0 :
> getFieldOffset("JavaThread::_stack_overflow_state._reserved_stack_activation", Integer.class, "address"); // JDK-8046936

Again unclear this can actually work.

src/hotspot/share/runtime/stackOverflow.hpp line 33:

> 31: class JavaThread;
> 32:
> 33: class StackOverflow {

Can we add a descriptive comment of what this class actually represents please - and the fact it is tied to JavaThreads
only. Ideally this would be within the logical namespace of JavaThread so that it doesn't appear to be a
standalone/independent entity. Also the name isn't quite right as it doesn't represent an actual stack-overflow but the
protection/state mechanism around that - so perhaps StackOverflowState or StackOverflowProtection ?

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/522