RFR(xs): 8184339: Thread::current_or_null() should not assert if Posix TLS is not yet initialized

[Thomas Stüfe]

Hi David!

On Tue, Aug 1, 2017 at 9:33 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> Back from vacation ...
>
> I appreciate the effort to fix this problem, however ... :)
>
> Thomas Stüfe thomas.stuefe at gmail.com
>> Mon Jul 17 07:00:01 UTC 2017
>> Hi all,
>>
>> may I please have reviews + a sponsor for the following fix:
>>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8184339
>> webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8184339-Thread-cu
>> rrent-or-null-shall-not-assert/webrev.00/webrev/
>>
>> The problem is caused by the fact that Posix TLS cannot be used before it
>> is initialized. It is initialized in os::init(). If we use Posix TLS (eg
>> via Thread::current()) before, we assert. It is used now (since
>> JDK-8183925
>> <https://bugs.openjdk.java.net/browse/JDK-8183925>) before os::init()
>> (see
>> callstack in bug description).
>>
>
> ... those kind of initialization order changes are exactly what the
> assertion was supposed to detect! It can be dangerous to use various VM
> facilities "too soon" in the initialization sequence. The change in
> JDK-8183925 needs a close examination IMHO before relaxing this guard. :(


I agree with you that using a VM facility "too soon" is the core of this
problem. But I am not sure that just forbidding that is a practical
solution, because these pre-init-uses have the tendency to creep up on you:

There are many functions in the VM which very basic and which are used
without regard for initialization sequence. os::malloc() is one example,
Thread::current_or_null() is another. It is difficult to find pre-init uses
with tests, especially if we have code paths which are only executed on one
platform, like in this case.

In this case, on AIX, we had two problems, Thread::current_or_null() was
asserting, then it was again used in error handling, asserting again, which
lead to an annoying recursion difficult to entangle (on AIX with its
less-than-optimal debugger at least).

To prevent this from happening, one would have to guard every
"Thread::current_or_null()" use with something like "if
ThreadLocalStorage.is_initialized()". But I think a more practical solution
would be to keep these basic functions safe to be called anytime. And maybe
make this a clear part of the interface, and test this too. Similar to the
Posix list of functions safe to be called in signal handlers.

Thread::current_or_null() could return NULL before initialization,
os::malloc() could refrain from using NMT before its initialization ran,
and NMT in turn could tolerate "free/realloc-without-malloc" cases...

(Note that this discussion feels similar to the "CanUseSafeFetch()" issue
we had two years ago, and we did not agree then either :)


>
> There are two functions, Thread::current() and Thread::current_or_null().
>> The latter should not assert if Posix TLS is not yet available but return
>> NULL instead. This is what callers expect: this function is safe to call,
>> but it might not return a valid Thread*.
>>
>
> Yes but because the current thread has not yet been set, not because we're
> executing the code well before any VM initialization has been performed! :(


I think anyone using Thread::current_or_null() is aware that the thread
pointer returned may be NULL and is prepared to handle it, but does not
care for the reason it is NULL. Like, in error handling.



> Note that this issue currently only comes up at AIX, because AIX is the
>> only platform using Posix TLS for Thread::current() - all other platforms
>> use Compiler-based TLS (__thread variables).
>>
>
> It's also used in the mobile-dev project IIRC, but they don't have
> JDK-8183925 applied.
>
> However, we want to keep the Posix TLS code path alive, so this may also
>> affect other platforms. There have been compiler bugs in the past (e.g.
>> gcc
>> + glibc) leading to errors when using compiler-based TLS, so it is good to
>> have a reliable fallback.
>>
>
> Definitely!
>
>
Good to be in agreement here!

Kind Regards, Thomas

Cheers,
> David
>
> Thanks,
>>
>> Thomas
>>
>>