Hi David, our mail service did the wrong thing here.
This was a reply to your comment, so you should have been in To-field,
this quote above "Hi David" should not have been here, and this was a
reply your mail (via github comment).
Thanks, Robbin
On 2020-09-08 09:39, Robbin Ehn wrote:
On Mon, 7 Sep 2020 23:07:13 GMT, Yasumasa Suenaga <[email protected]> wrote:
When these two methods (set_frame_pop/clear_frame_pop) are called in a
handshake the requesting thread will have lock
the JvmtiThreadState_lock. But the thread executing one of these in the
handshake may not be the owner.
So we only check that JvmtiThreadState_lock is locked.
When verifying the callers to these methods I notice "clear_to_frame_pop" was
unused, so instead of fixing it I remove
it.
Passes testing locally, still running T3 and T7.
Now passed!
Thanks for catching up this. Looks good.
Hi David,
When these two methods (set_frame_pop/clear_frame_pop) are called in a
handshake the requesting thread will have lock
the JvmtiThreadState_lock. But the thread executing one of these in the
handshake may not be the owner.
Ouch! We continue to get bitten by the fact the executor of a handshake
operation is not known a-priori. :(
We could add this. But I would prefer not doing it in this bug fix.
So we only check that JvmtiThreadState_lock is locked.
That avoids the problem but it is not really a sufficient check - we
need to know that it is the handshaker thread that owns the lock and
that it acquired it purely for the purpose of this handshake operation!
Right now we know that because I traced the code :)
As I said I can make the requesting thread known to execution thread,
but I would really prefer not doing that in this bug fix.
More importantly this issue shows that the locking code is in fact
incorrect. We are on very dangerous ground if we are going to allow
locks to be "proxied" in this fashion! This is akin to reintroducing
"sneaky locks"!
A problem is that we use a safepoiting global lock to protect per thread
resource.
The way we get around this with safepoints is:
"assert_locked_or_safepoint" (assert may be passed by a non-JavaThread or
JavaThread in blocked/native without lock
during a safepoint (safepoint + VM thread would fix that)) Which have this in
it:
// see if invoker of VM operation owns it
VM_Operation* op = VMThread::vm_operation();
if (op != NULL && op->calling_thread() == lock->owner()) return;
So we already have proxied locks and in other cases just avoiding them (by just
checking if at safepoint).
And as I said a few times now, I said I can make the requesting thread known to
execution thread,
but I would really prefer not doing that in this bug fix.
If a handshake operation can be executed by either the target thread or
the handshaker thread, and locks are required, then they will have to be
acquired in the context of the handshake operation. If we can't do that
because of potential deadlocks in the handshake mechanism then I feel
that handshakes have a serious flaw that needs to be rectified. Perhaps
we need a way to force the operation to be executed by the handshaker so
that they can setup the right execution environment prior to the handshake?
The problem is that it's a safepointing lock.
Only executing the handshake by requester is an interesting idea, I'll
investigate that.
When verifying the callers to these methods I notice "clear_to_frame_pop" was
unused, so instead of fixing it I remove
it.
There is already a bug for that:
https://bugs.openjdk.java.net/browse/JDK-8252816
so if this proceeds you should add that issue to the PR.
Ok, I'll do that.
Thanks for having a look, let me know howto proceed.
Thanks,
David
-----
-------------
PR: https://git.openjdk.java.net/jdk/pull/60
