[jdk8u-dev] RFR: 8339882: Replace ThreadLocalStorage::thread with Thread::current_or_null in jdk8 backport of JDK-8183925

[Andrew John Hughes]

On Tue, 10 Sep 2024 23:55:34 GMT, Ben Taylor <btaylor at openjdk.org> wrote:

> This change replaces 3 instances of `ThreadLocalStorage::thread` with `Thread::current_or_null` to ensure that `ThreadLocalStorage::is_initialized` is checked before attempting to get the current thread.
> 
> Change passes all tier1 tests locally on Linux x86_64 and Windows x86_64

The patch looks fine to me. The calls now match what is done in 11u and I don't see any harm in the additional initialisation check.

The current state of 8u on this is even more convoluted than the bug suggests. Although all three calls are introduced in 8183925 in 11u, the [8u one is different](https://github.com/openjdk/jdk8u/commit/6a9155aa13d4c6713e63f23f2d66435d2eb60bf8) and only adds `ThreadLocalStorage::thread` in `os.cpp` and removes `WatcherThreadCrashProtection`. The other two calls appear when [the JFR backport](https://github.com/openjdk/jdk8u/commit/6ca492afe7c59b014c6ce0628eb88d7da34020b5) introduces `ThreadCrashProtection`. The only discussion of this I could see [on the mailing list was in relation to JFR](https://mail.openjdk.org/pipermail/jdk8u-dev/2020-February/011073.html).

For `Thread::current_or_null` itself, it was added to support [JDK-8237499](https://bugs.openjdk.org/browse/JDK-8237499) in 8u, but it doesn't originate from there in 11u. It was only brought in to support the existence of the function in that patch. Instead, it originates from the [JDK-8132510](https://bugs.openjdk.org/browse/JDK-8132510) enhancement.

There is thus very little use of `current_or_null` in the 8u codebase, compared with 11u:

~~~
$ grep -r 'current_or_null' ../jdk8u/hotspot/src/
../jdk8u/hotspot/src/share/vm/code/codeCache.cpp:  Thread* current_thread = Thread::current_or_null();
../jdk8u/hotspot/src/share/vm/jfr/support/jfrThreadLocal.cpp:  Thread* thread = Thread::current_or_null();
../jdk8u/hotspot/src/share/vm/runtime/thread.hpp:  static inline Thread* current_or_null();
../jdk8u/hotspot/src/share/vm/runtime/thread.hpp:  Thread* current = current_or_null();
../jdk8u/hotspot/src/share/vm/runtime/thread.hpp:inline Thread* Thread::current_or_null() {
~~~

I'm happy for this change to go in as is to fix the bug you found, but it might be worth going through 8132510 to find other cases that could be replaced, without bringing in that whole TLS enhancement change (`current_or_null` operates differently in 11u with that patch, if `USE_LIBRARY_BASED_TLS_ONLY` is set)

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

Marked as reviewed by andrew (Reviewer).

PR Review: https://git.openjdk.org/jdk8u-dev/pull/576#pullrequestreview-2444172235