Skip to content

8287281: adjust guarantee in Handshake::execute for the case of target thread being current #8992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

8287281: adjust guarantee in Handshake::execute for the case of target thread being current #8992

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
14bee58
Disallow handshaking with self
jdksjolen Jun 1, 2022
b7b4c08
Call do_thread if handshaking with current thread as target
jdksjolen Jun 3, 2022
b40577f
Use is_handshake_safe_for and add the return
jdksjolen Jun 3, 2022
8ddb442
Switch order of handshake check
jdksjolen Jun 3, 2022
e356932
Remove checks for is_handshake_for, instead call Handshake::execute
jdksjolen Jun 3, 2022
bf75d4c
do_thread(target) not self
jdksjolen Jun 3, 2022
d4aea87
Use current instead of self as name for current thread
jdksjolen Jun 7, 2022
c476d30
Remove unused variable
jdksjolen Jun 7, 2022
f2646e0
Move assert up and remove other assert, remove unused var
jdksjolen Jun 7, 2022
cc6736c
Check only for target == current
jdksjolen Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/hotspot/share/prims/jvmtiEnvThreadState.cpp
View file
Original file line number Diff line number Diff line change
@@ -396,13 +396,8 @@ void JvmtiEnvThreadState::reset_current_location(jvmtiEvent event_type, bool ena
// The java thread stack may not be walkable for a running thread
// so get current location with direct handshake.
GetCurrentLocationClosure op;
Thread *current = Thread::current();
if (thread->is_handshake_safe_for(current)) {
op.do_thread(thread);
} else {
Handshake::execute(&op, thread);
guarantee(op.completed(), "Handshake failed. Target thread is not alive?");
}
Handshake::execute(&op, thread);
guarantee(op.completed(), "Handshake failed. Target thread is not alive?");
Comment on lines +399 to +400
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I much prefer that the current-thread case is internalised by Handshake::execute now. The code creating the handshake op shouldn't have to worry about current thread or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having Handshake::execute() handle the current-thread case will certainly
allow us to make the code consistent in all the callers of Handshake::execute().

op.get_current_location(&method_id, &bci);
set_current_location(method_id, bci);
}
12 changes: 3 additions & 9 deletions src/hotspot/share/prims/jvmtiEventController.cpp
View file
Original file line number Diff line number Diff line change
@@ -352,12 +352,11 @@ void VM_ChangeSingleStep::doit() {


void JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *state) {
assert(state != NULL, "sanity check");
EC_TRACE(("[%s] # Entering interpreter only mode",
JvmtiTrace::safe_get_thread_name(state->get_thread_or_saved())));
JavaThread *target = state->get_thread();
Thread *current = Thread::current();

assert(state != NULL, "sanity check");
if (state->is_pending_interp_only_mode()) {
return; // An EnterInterpOnlyModeClosure handshake is already pending for execution.
}
@@ -367,13 +366,8 @@ void JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *state
return; // EnterInterpOnlyModeClosure will be executed right after mount.
}
EnterInterpOnlyModeClosure hs;
if (target->is_handshake_safe_for(current)) {
hs.do_thread(target);
} else {
assert(state->get_thread() != NULL, "sanity check");
Handshake::execute(&hs, target);
guarantee(hs.completed(), "Handshake failed: Target thread is not alive?");
}
Handshake::execute(&hs, target);
guarantee(hs.completed(), "Handshake failed: Target thread is not alive?");
}


17 changes: 11 additions & 6 deletions src/hotspot/share/runtime/handshake.cpp
View file
Original file line number Diff line number Diff line change
@@ -351,12 +351,17 @@ void Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target) {
}

void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh, JavaThread* target) {
JavaThread* self = JavaThread::current();
HandshakeOperation op(hs_cl, target, Thread::current());
guarantee(target != nullptr, "must be");

jlong start_time_ns = os::javaTimeNanos();
JavaThread* current = JavaThread::current();
if (target == current) {
hs_cl->do_thread(target);
return;
}

guarantee(target != nullptr, "must be");
HandshakeOperation op(hs_cl, target, current);

jlong start_time_ns = os::javaTimeNanos();
if (tlh == nullptr) {
guarantee(Thread::is_JavaThread_protected_by_TLH(target),
"missing ThreadsListHandle in calling context.");
@@ -389,9 +394,9 @@ void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh, JavaThr
hsy.add_result(pr);
// Check for pending handshakes to avoid possible deadlocks where our
// target is trying to handshake us.
if (SafepointMechanism::should_process(self)) {
if (SafepointMechanism::should_process(current)) {
// Will not suspend here.
ThreadBlockInVM tbivm(self);
ThreadBlockInVM tbivm(current);
}
hsy.process();
}