Skip to content

8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync #18238

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 5 commits into from
Closed

8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync #18238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b34ad5f
Switch to WXWrite before entering the vm
reinrich Mar 12, 2024
9075f4b
Set WXWrite at more locations
reinrich Mar 15, 2024
1b4d760
Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
reinrich Mar 18, 2024
fc38372
2 more locations
reinrich Mar 20, 2024
8239638
Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
reinrich Mar 20, 2024
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
2 changes: 2 additions & 0 deletions src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ void JfrJvmtiAgent::retransform_classes(JNIEnv* env, jobjectArray classes_array,
return;
}
ResourceMark rm(THREAD);
// WXWrite is needed before entering the vm below and in callee methods.
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, THREAD));
Comment on lines +159 to +160
Copy link
Member

Choose a reason for hiding this comment

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

I understand you placed this here to cover the transition inside create_classes_array and the immediate one at line 170, but doesn't this risk having the wrong WX state for code in between?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've asked this myself (after making the change).
Being in WXWrite mode would be wrong if the thread would execute dynamically generated code. There's not too much happening outside the scope of the ThreadInVMfromNative instances. I see jni calls (GetObjectArrayElement, ExceptionOccurred) and a jvmti call (RetransformClasses) but these are safe because the callees enter the vm right away. We even avoid switching to WXWrite and back there.
So I thought it would be ok to coarsen the WXMode switching.
But maybe it's still better to avoid any risk especially since there's likely no performance effect.

Copy link
Member

Choose a reason for hiding this comment

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

Or could the ThreadInVMfromNative tvmfn(THREAD); in check_exception_and_log be move out to JfrJvmtiAgent::retransform_classes? And then only use one ThreadInVMfromNative in JfrJvmtiAgent::retransform_classes

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this would require hoisting the ThreadInVMfromNative out of the loop with the check_exception_and_log call but then the thread would be in _thread_in_vm when doing the GetObjectArrayElement jni call which would be wrong.

jclass* const classes = create_classes_array(classes_count, CHECK);
assert(classes != nullptr, "invariant");
for (jint i = 0; i < classes_count; i++) {
Expand Down
4 changes: 3 additions & 1 deletion src/hotspot/share/jfr/jni/jfrJniMethod.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ NO_TRANSITION_END
NO_TRANSITION(void, jfr_set_enabled(JNIEnv* env, jclass jvm, jlong event_type_id, jboolean enabled))
JfrEventSetting::set_enabled(event_type_id, JNI_TRUE == enabled);
if (EventOldObjectSample::eventId == event_type_id) {
ThreadInVMfromNative transition(JavaThread::thread_from_jni_environment(env));
JavaThread* thread = JavaThread::thread_from_jni_environment(env);
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, thread));
ThreadInVMfromNative transition(thread);
if (JNI_TRUE == enabled) {
LeakProfiler::start(JfrOptionSet::old_object_queue_size());
} else {
Expand Down
4 changes: 3 additions & 1 deletion src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,8 @@ void JfrRecorderService::emit_leakprofiler_events(int64_t cutoff_ticks, bool emi
// and serializes all event emit checkpoint events to the same segment.
JfrRotationLock lock;
// Take the rotation lock before the transition.
ThreadInVMfromNative transition(JavaThread::current());
JavaThread* current_thread = JavaThread::current();
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, current_thread));
ThreadInVMfromNative transition(current_thread);
LeakProfiler::emit_events(cutoff_ticks, emit_all, skip_bfs);
}
1 change: 1 addition & 0 deletions src/hotspot/share/jfr/recorder/storage/jfrStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ void JfrStorage::register_full(BufferPtr buffer, Thread* thread) {
JavaThread* jt = JavaThread::cast(thread);
if (jt->thread_state() == _thread_in_native) {
// Transition java thread to vm so it can issue a notify.
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt));
ThreadInVMfromNative transition(jt);
_post_box.post(MSG_FULLBUFFER);
return;
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/jfr/support/jfrIntrinsics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ void* JfrIntrinsicSupport::write_checkpoint(JavaThread* jt) {
assert(JfrThreadLocal::is_vthread(jt), "invariant");
const u2 vthread_thread_local_epoch = JfrThreadLocal::vthread_epoch(jt);
const u2 current_epoch = ThreadIdAccess::current_epoch();
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt));
if (vthread_thread_local_epoch == current_epoch) {
// After the epoch test in the intrinsic, the thread sampler interleaved
// and suspended the thread. As part of taking a sample, it updated
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/jfr/writers/jfrJavaEventWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ void JfrJavaEventWriter::flush(jobject writer, jint used, jint requested, JavaTh
u1* const new_current_position = is_valid ? buffer->pos() + used : buffer->pos();
assert(start_pos_offset != invalid_offset, "invariant");
// can safepoint here
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt));
ThreadInVMfromNative transition(jt);
oop const w = JNIHandles::resolve_non_null(writer);
assert(w != nullptr, "invariant");
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/prims/jvmtiExport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ JvmtiExport::get_jvmti_interface(JavaVM *jvm, void **penv, jint version) {
if (JvmtiEnv::get_phase() == JVMTI_PHASE_LIVE) {
JavaThread* current_thread = JavaThread::current();
// transition code: native to VM
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, current_thread));
ThreadInVMfromNative __tiv(current_thread);
VM_ENTRY_BASE(jvmtiEnv*, JvmtiExport::get_jvmti_interface, current_thread)
debug_only(VMNativeEntryWrapper __vew;)
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/prims/jvmtiExtensions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ static jvmtiError JNICALL GetCarrierThread(const jvmtiEnv* env, ...) {
thread_ptr = va_arg(ap, jthread*);
va_end(ap);

MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, current_thread));
ThreadInVMfromNative tiv(current_thread);
JvmtiVTMSTransitionDisabler disabler;

Expand Down