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 <dhol...@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
