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

[David Holmes]

On Thu, 18 Mar 2021 12:17:25 GMT, Coleen Phillimore <coleenp 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
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 1217:
> 
>> 1215:     assert(fr.is_entry_frame(), "must be");
>> 1216:     // fr is now pointing to the entry frame.
>> 1217:     callee_method = methodHandle(thread, fr.entry_frame_call_wrapper()->callee_method());
> 
> Ok, now this just seems arbitrary to me.  I don't see why we should introduce a JavaThread* thread, when there's a perfectly good THREAD available here, and THREAD is fine here.  There's a lot of code that uses THREAD as a Handle parameter and there's nothing wrong  with it.  If it's lower case 'thread', one has to wonder and look around to see if it's the current thread (changing the convention to 'current' was good where you did that).  Don't change these ones.  It seems like a conflict of your preferences to what's in the code, and I vote for what's used in the code everywhere.  Removing TRAPS from functions that don't throw an exception is also good. But I don't see the point of this at all and I think it's worse.

The intent is that THREAD, ideally, is only used in relation to exception throwing/catching and you can't tell that if you use it just for convenience everywhere else. I haven't introduced a new local "thread" when there is only a single use, but if there are multiple uses then using "thread" or "current" is IMO much clearer because we can clearly isolate the the code that relates to exceptions and the code that does not. That is what I'm trying to achieve here. It wont be perfect and not every use of THREAD will be replaced, but it will be a great improvement over what we currently have.
In this code I would love to change "thread" to "current" but the JRT macros require the use of "thread" and changing that would be too big a change. Note that in the code above I'm not introducing "thread" it is already there.

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

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