RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

[Ioi Lam]

On Thu, 18 Mar 2021 02:48:05 GMT, David Holmes <dholmes at openjdk.org> wrote:

> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
> 
> This should be cleaned up along with a general check on misuse of TRAPS/THREAD. 
> 
> Testing: tiers 1-3
> 
> Thanks,
> David

Looks good to me in general. I have some comments about coding style.

src/hotspot/share/interpreter/interpreterRuntime.cpp line 319:

> 317:     if (trap_mdo == NULL) {
> 318:       ExceptionMark em(current);
> 319:       JavaThread* THREAD = current; // for exception macros

Need to assert that `current` is indeed the current thread. Maybe we should have an inline function like this which does the current thread check?

JavaThread* THREAD = current->for_exception_handling();

(Same comment for all the other `Thread* THREAD = thread;` in this PR).

src/hotspot/share/interpreter/interpreterRuntime.cpp line 974:

> 972: 
> 973: 
> 974: nmethod* InterpreterRuntime::frequency_counter_overflow(JavaThread* current, address branch_bcp) {

Need to assert that `current` is indeed current.

(Same comment for all other parameters that you've changed to `current`).

src/hotspot/share/interpreter/interpreterRuntime.cpp line 1117:

> 1115: 
> 1116: JRT_ENTRY(MethodCounters*, InterpreterRuntime::build_method_counters(JavaThread* thread, Method* m))
> 1117:   MethodCounters* mcs = Method::build_method_counters(m, THREAD);

Need `ExceptionMark em(THREAD);`

src/hotspot/share/oops/method.cpp line 579:

> 577: 
> 578:   if (LogTouchedMethods) {
> 579:     mh->log_touched(THREAD);

When you have THREAD as the last parameter, it's not clear whether

- You are calling a non-throwing function that just wants a thread, you
- You are calling an exception-throwing function but you are ignoring the exception on purpose

It seems the former is the case. I would prefer something like this, which makes it clear what you're doing:

mh->log_touched(THREAD->as_JavaThread());

src/hotspot/share/runtime/sharedRuntime.cpp line 1231:

> 1229: methodHandle SharedRuntime::resolve_helper(bool is_virtual, bool is_optimized, TRAPS) {
> 1230:   methodHandle callee_method;
> 1231:   callee_method = resolve_sub_helper(is_virtual, is_optimized, THREAD);

The two occurrences of  `THREAD` in this function seem fishy. Maybe they can be replaced with CHECK? But it's unrelated to this PR so probably should be done in a separate REF.

src/hotspot/share/runtime/sharedRuntime.cpp line 1661:

> 1659: 
> 1660: methodHandle SharedRuntime::handle_ic_miss_helper(TRAPS) {
> 1661:   JavaThread* thread = THREAD->as_Java_thread();

A general comment about the functions that took both `thread` and `TRAPS`, where you removed the `thread`:

Are we sure they cannot be called on a non-current thread?

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

Changes requested by iklam (Reviewer).

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