RFR: JDK-8293346: Add NoThreadCurrentMark to simulate non-attached threads [v2]

[David Holmes]

On Thu, 22 Sep 2022 17:08:02 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> When working on [JDK-8293344](https://bugs.openjdk.org/browse/JDK-8293344), I needed a way to verify that I had removed all offending usages of ResourceArea memory inside an extend.
>> 
>> For that, a NoThreadCurrentMark was useful. It temporarily sets the current Thread* to NULL for the extend. Any use of Thread::current below that mark will now crash or assert.
>> 
>> This can be used for two things:
>> - guard code that is supposed to be safe for non-attached threads (os::malloc, stack printing, UL etc) against accidental usage of Thread::current() (e.g. resource area allocation)
>> - in gtests, simulate a non-attached thread to cover code that behaves differently with Thread::current()==NULL.
>> 
>> This patch:
>> 
>> - Introduces the debug-only `NoThreadCurrentMark`
>> - Adds a gtest for it
>> - Adds it to guard dwarf-parsing-based stack printing, which had not been safe due to use of ResourceArea, fixed with  [JDK-8293344](https://bugs.openjdk.org/browse/JDK-8293344)
>> - Adds it to `os::malloc()`, `os::realloc()` and `os::free()`, since those can certainly be called without a current thread.
>> - Replaces the test-local mark in the SafeFetch gtests that did a similar thing
>> - I may add and fix up other users later
>> 
>> I also had to change Library-based TLS initialization such that it runs at C++ dynamic initialization time. That is because the `NoThreadCurrentMark` needs to set both Library-based TLS and C++ TLS slots, and having library-based TLS not available before VM initialization made the code too complex. This also means we can remove the explicit TLS initialization from create_vm.
>> 
>> Finally, while I was here, I also fixed the error printing in library based TLS (since the pthread_key_xxx functions don't modify errno).
>
> Thomas Stuefe has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - New solution
>  - Revert changes to TLS initialization

This version of the code seems fine in terms of basic functionality.

I'm less convinced about the actual usefulness of this, but as it is debug only code ... My main query here is why some of this code should ever be being executed by non-attached threads? Conditional pre-VM-init usage? (If it unconditional then we don't need to simulate anything as we will have a NULL current thread.)

src/hotspot/share/runtime/thread.hpp line 668:

> 666:   if (t != nullptr && t->_thread_current_disabled) {
> 667:     t = nullptr;
> 668:   }

This needs to be in `ifdef ASSERT`

src/hotspot/share/runtime/thread.hpp line 679:

> 677:   if (t != nullptr && t->_thread_current_disabled) {
> 678:     t = nullptr;
> 679:   }

This needs to be in `ifdef ASSERT`

src/hotspot/share/runtime/thread.inline.hpp line 96:

> 94: 
> 95: #ifdef ASSERT
> 96: // Mark disables Thread::current by setting it to NULL for the extend. Can be

s/extend/extent/

src/hotspot/share/runtime/thread.inline.hpp line 98:

> 96: // Mark disables Thread::current by setting it to NULL for the extend. Can be
> 97: // used to simulate running code in non-attached threads, or to guard code
> 98: // against accidental usage of features that depend on Thread::current() (e.g.

So the "guard" aspect of this is that we expect to crash if the code tries to use the result of `Thread::current()`?

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

PR: https://git.openjdk.org/jdk/pull/10178