RFR(s): 8212173: Thread._stack_base/_stack_size initialized too late for new threads (original) (raw)

Thomas Stüfe thomas.stuefe at gmail.com
Fri Oct 26 12:35:24 UTC 2018

Hi Volker,

thanks for the review!

On Fri, Oct 26, 2018 at 11:45 AM Volker Simonis <volker.simonis at gmail.com> wrote:

HI Thomas, your change looks good. Please find some minor comments below: ossolaris.cpp/oswindows.cpp - why don't you set "thread" to null after the return from callrun() like on the other platforms? Or maybe the comment is enough and you don't have to set it to null at all?

Because on those two platforms, "thread" is used some lines down in a pointer comparison for some counter thing:

if (thread != VMThread::vm_thread()...

I am not really sure if those counters are actually needed, but I did not dwelve into it, I wanted to keep the patch small and concise. I think at least on windows they can probably be removed.

Thanks, Thomas

There's no need to do a new webrev for this.

Thanks, Volker On Thu, Oct 25, 2018 at 8:02 PM Thomas Stüfe <thomas.stuefe at gmail.com> wrote: > > Ping... > > May I please have a second review? > > Thanks! > On Mon, Oct 22, 2018 at 6:56 PM Thomas Stüfe <thomas.stuefe at gmail.com> wrote: > > > > Dear all, > > > > may I have please reviews for the following fix: > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8212173 > > cr: http://cr.openjdk.java.net/~stuefe/webrevs/8212173-thread-stack-and-base-initialized-too-late/webrev.02/webrev/ > > > > See also prior mail discussions: [1] > > > > Wwhen iterating the thread list, threads may be visible which have > > stack base/size not yet set. Calling stackbase() will yield NULL, > > stacksize() 0 or it will assert. The reason for this is that there > > exist a race: Thread::stackbase and ::stacksize are initialized > > too late, when the Thread has already been added to the threads list. > > > > My solution to this problem is basically to move the initialization of > > Thread::stackbase and ::stacksize to an earlier point at which the > > thread has not yet been added to the threads list. > > > > Unfortunately, this is more difficult that it sounds. Retrieving > > thread stack base and size needs to be done from within the thread in > > question, and only on Linux and BSD we have the opportunity to execute > > code in the new child thread before its Thread object is added to the > > thread list. So, my patch solves the problem completely only on Linux > > and BSD. On Windows, Aix and Solaris it only minimizes the chance for > > it to happen. > > > > For more details, please see bug description and the discussion in [1]. > > > > ---- > > > > My changes in detail: > > > > - Thread::recordstackbaseandsize(): I removed all code which needs > > the surrounding Thread object being initialized, or which needs > > Thread::current() to be set. That makes the function safe to be called > > from the very start of the child thread's life (start of > > threadnativeentry()), where nothing is initialized yet. > > > > - I moved the call to Thread::recordstackbaseandsize() out of all > > ::run() functions into the very start of the > > threadnativeentry() on every platform. > > > > - The parts I moved out of Thread::recordstackbaseandsize() are: > > NMT thread stack initialization and Logging. I needed to find a new > > place for them. I wanted them to run at the end of the child thread > > initialization, when the child Thread is already initialized and > > Thread::current is initialized too. So I moved them to just before > > ::run() is called. > > > > - To prevent code duplication, I changed: > > > > public virtual Thread::run() > > > > to > > > > public (nonvirtual) Thread::callrun(); > > protected virtual Thread::run() = 0; > > > > Thread::callrun() now is the place to put common, > > platform-independent and -independent > > initializations. There I put the NMT stuff and logging. After > > ::run() returns, I put common cleanup checks. > > > > Other changes: > > > > - We had a function os::initializethread() which only existed for the > > sole benefit of Solaris, where if gives the platform a stab at > > correcting the stack base, size for a certain corner case when we run > > on primordial thread. On all other platform-cpu-combinations, this > > function was empty. I removed all empty implementations and renamed > > the function to the much more fitting, Solaris only > > "os::Solaris::correctstackboundariesforprimordialthread(Thread* > > thr)". > > > > - When ::run() returns, the Thread object may have > > been deleted by the child class run() function. To avoid errors I set > > the thread pointer to zero (where possible) to prevent accessing it to > > reference members. > > > > - Since Thread::recordstackbaseandsize() does not register the > > thread stack with NMT anymore, I needed to fix those places where the > > former is called for attached threads and add that NMT registration > > manually. > > > > - I made Thread::clearthreadcurrent() a static function since it > > needs no members of Thread (unlike Thread::initializethreadcurrent() > > which needs the this pointer to hook it into TLS). That makes it > > possible to use clearthreadcurrent() without having a valid Thread > > object, which is the case if ::run() deleted the > > Thread. > > > > ---- > > > > That's about it. > > > > The change ran through jdk-submit tests without errors. > > > > Thank you for reviewing. > > > > ..Thomas > > > > [1] http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-October/030516.html

More information about the hotspot-runtime-dev mailing list