RFR: 8252406: Introduce Thread::as_Java_thread() convenience function

[David Holmes]

Hi Kim,

Thanks for taking a look.

On 8/09/2020 9:23 am, Kim Barrett wrote:
>> On Sep 7, 2020, at 5:47 PM, David Holmes <dholmes at openjdk.java.net> wrote:
>>
>> […]
>> Commit messages:
>> - Initial changes for 8252406
>>
>> Changes: https://git.openjdk.java.net/jdk/pull/37/files
>> Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=00
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8252406
>>   Stats: 463 lines in 112 files changed: 15 ins; 124 del; 324 mod
>>   Patch: https://git.openjdk.java.net/jdk/pull/37.diff
>>   Fetch: git fetch https://git.openjdk.java.net/jdk pull/37/head:pull/37
>>
>> PR: https://git.openjdk.java.net/jdk/pull/37
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/thread.hpp
>   506   JavaThread* as_Java_thread() {
>   507     assert(is_Java_thread(), "incorrect cast to JavaThread");
>   508     return (JavaThread*)this;
>   509   }
> 
> This should be using a static_cast.  However, making that change will
> uncover a problem.
> 
> This definition cannot correctly be located here. It must follow the
> definition of the JavaThread class.

I did try to do the right thing with static_cast and of course 
discovered that I couldn't use it at that location. As I was replacing 
C-style casts, and as other C-style casts continue to be used in similar 
methods, I just kept with the C-styler cast.

> At this point (in the body of the
> Thread class definition), JavaThread is incomplete, so the C-style
> cast is a reinterpret_cast. It's only by implementation accident (and
> that we're not using multiple or virtual inheritance anywhere in the
> vicinity), that a reinterpret_cast happens to work.
> 
> (The definition can remain in this file if that's more
> convenient #include-wise than putting it in thread.inline.hpp, though
> the latter would be the more usual placement.)

Okay I will look at what is involved in putting it in the .inline.hpp 
file; if that causes too many problems then I'll just shift it to an 
inline definition later in the hpp file.

> (One of the very real dangers with C-style casts is that you might not
> be doing the cast you think you are doing.)
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/thread.hpp
>   510   JavaThread * as_const_Java_thread() const {
>   511     assert(is_Java_thread(), "incorrect cast to JavaThread");
>   512     return (JavaThread*)this;
>   513   }
> 
> This should be
> 
> const JavaThread* as_Java_Thread() const {
>    assert(is_Java_thread(), "incorrect cast to JavaThread");
>    return static_cast<const JavaThread*>(this);
> }
> 
> And of course, moved after the definition of JavaThread, per the above
> discussion of the non-const function.

Will fix.

> ------------------------------------------------------------------------------
> 
> I reviewed all the (non-Shenandoah) gc changes, and they look good.

Thanks for the review and coding tips!

Cheers,
David