On 15/07/2021 10:29 pm, Jorn Vernee wrote:
On Wed, 14 Jul 2021 00:47:47 GMT, David Holmes <dhol...@openjdk.org> wrote:
Jorn Vernee has updated the pull request incrementally with one additional
commit since the last revision:
Assert frame is correct type in frame_data_for_frame
src/hotspot/share/prims/universalUpcallHandler.cpp line 76:
74:
75: // modelled after JavaCallWrapper::JavaCallWrapper
76: Thread* ProgrammableUpcallHandler::on_entry(OptimizedEntryBlob::FrameData*
context) {
This should return JavaThread not Thread.
Thanks.
I've been careful here to return a `Thread*` since the result is stored in
`r15_thread` and I thought converting between sub and super types could
potentially result in different pointers due to the way super types are laid
out within a subtype. I thought it worked like this:
Wow! Okay I've never seen anyone query this before. AFAIK whatever we
store in r15_thread is required to be a correct pointer to the current
thread object whatever its exact subtype may be. The returned pointer
has to work correctly for virtual functions and can't be a "sliced"
Thread instead of the real type. So as far as I know this "just works"
and I think we'd be in big trouble if it didn't work.
But I don't deal with the under-the-covers parts of the C++ compiler.
David
-----
Subclass
+---
| {Subclass vtable pointer}
| +--- (base class Super)
| | {Super vtable pointer}
| +---
+---
So, I thought plainly using a `JavaThread*` in generated machine code where a
`Thread*` was expected could cause trouble, since the pointer needs to be
offset for the type conversion.
But now that I'm looking at some cases with compiler explorer, the pointer
offset only seems to be needed when using multiple inheritance, for instance:
class SuperA {
public:
virtual void foo();
};
class SuperB {
public:
virtual void bar();
};
class Sub : public SuperA, public SuperB {
public:
virtual void baz();
};
Results in:
class Sub size(16):
+---
0 | +--- (base class SuperA)
0 | | {vfptr}
| +---
8 | +--- (base class SuperB)
8 | | {vfptr}
| +---
+---
Sub::$vftable@SuperA@:
| &Sub_meta
| 0
0 | &SuperA::foo
1 | &Sub::baz
Sub::$vftable@SuperB@:
| -8
0 | &SuperB::bar
Sub::baz this adjustor: 0
(https://godbolt.org/z/rq9bT8d9d)
It seems that the sub type just reuses the vtable pointer of the first super
type (probably to avoid having to do this pointer offsetting). Though,
converting between `SuperB*` and `Sub*` would require offsetting the pointer.
I'm still not sure this is guaranteed to work like this with all compilers
though (the example is with MSVC, which has options to dump class layouts).
The result of `on_entry` is stored in `r15_thread`, so I guess I'm wondering if
it's safe to store a `JavaThread*` instead of a `Thread*` in `r15`, and other
code, which may expect `r15` to hold a `Thread*`, is guaranteed to keep
working? (FWIW, after changing the return type to `JavaThread*` the tests that
exercise this code still pass on Windows with MSVC, and on WSL Linux with GCC).
-------------
PR: https://git.openjdk.java.net/jdk17/pull/149
