RFR: 8252685: APIs that require JavaThread should take JavaThread arguments

[David Holmes]

On Sat, 8 May 2021 13:06:01 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Our code is littered with API's that take, or manifest, a Thread* and then assert/guarantee that it must be a JavaThread, rather than taking/manifesting a JavaThread in the first place. The main reason for this is that the TRAPS macro, used in relation to exception generation and processing, is declared as "Thread* THREAD". In practice only JavaThreads that can execute Java code can generate arbitrary exceptions, because it requires executing Java code. In other places we can get away with other kinds of threads "throwing" exceptions because they are only pre-allocated instances that require no Java code execution (e.g. OOME), or when executed by a non-JavaThread the code actually by-passes the logic would throw an exception. Such code also eventually clears the exception and reports the OOM by some other means. We have been progressively untangling these code paths and modifying TRAPS/CHECK usage so that only JavaThreads will call TRAPS methods and throw exceptions. Having done t
 hat pre-work we are now ready to convert TRAPS to be "JavaThread* THREAD" and that is what this change does. The resulting changes are largely mechanical:
>> 
>> - declarations of "Thread* THREAD" become "JavaThread* THREAD" (where THREAD is needed for exceptions, otherwise THREAD is renamed to current)
>> - methods that took a Thread* parameter that was always THREAD, now take a JavaThread* param
>> - Manifestations of Thread::current() become JavaThread::current()
>> - THREAD->as_Java_thread() becomes just THREAD
>> - THREAD->is_Java_thread() is reworked so that THREAD is "current"
>> 
>> There are still places where a CompilerThread (which is a JavaThread but may not be able to execute Java code) calls a TRAPS function (primarily where OOME is possible). Fixing that would be a future RFE and may not be possible without reworking a lot of the allocation code paths.
>> 
>> Testing:
>>  - tiers 1-8 on Linux-x64
>>  - all builds in tiers 1-4
>>  - tiers 1-3 on all platforms
>> 
>> Thanks,
>> David
>
> src/hotspot/share/cds/archiveBuilder.cpp line 903:
> 
>> 901: 
>> 902:   static void write_klass(Klass* k, address runtime_dest, const char* type_name, int bytes, Thread* THREAD) {
>> 903:     ResourceMark rm(THREAD);
> 
> This change seems like it could be an independent check in because all you did was change the name of the parameter here, ie doesn't depend on on THREAD being a JavaThread.

Under these changes THREAD is always a JavaThread so I either need to change the type here (which would be wrong) or else change the name, because it doesn't actually relate to TRAPS/exceptions.  As this change touches a lot of files keeping it merged it time consuming so I'd prefer not to have to split out anything further at this stage. Thanks.

> src/hotspot/share/cds/dynamicArchive.cpp line 172:
> 
>> 170:   _header = mapinfo->dynamic_header();
>> 171: 
>> 172:   Thread* THREAD = Thread::current();
> 
> Unused?  Looks like you could also remove these in a trivial pre-commit.

Yes unused. Again trying to avoid further delays. Also I think this code just went through an undo and will soon be redone, in which case if it gets integrated first I'll try to get this cleaned up. Thanks.

> src/hotspot/share/runtime/sharedRuntime.cpp line 1197:
> 
>> 1195: 
>> 1196: methodHandle SharedRuntime::find_callee_method(TRAPS) {
>> 1197:   JavaThread* current = THREAD;
> 
> I think these look strange, especially the ones
> JavaThread* thread = THREAD;
> but they can be fixed later to just use THREAD when THREAD is available. And it's not really important.

They are the inverse of the

JavaThread* THREAD = current; // For exception macro use.

due to the general trend to not use THREAD for non-exception related code except in some trivial cases.

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

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