Better handling for attach & de-attach & edge cases for stop on a native thread

Author: Baraa-Hasheesh

src/asprof.cpp

@@ -32,32 +32,21 @@ DLLEXPORT asprof_error_t asprof_execute(const char* command, asprof_writer_t out
 
     Log::open(args);
 
-    bool attached = false;
-    if (args._action <= ACTION_CHECK && VM::loaded() && VM::jni() == NULL) {
-        if (VM::attachThread() == NULL) {
-            return asprof_error("Could not attach to the JVM");
-        }
-        attached = true;
-    }
-
     if (!args.hasOutputFile()) {
         CallbackWriter out(output_callback);
         error = Profiler::instance()->runInternal(args, out);
+        if (!error) {
+            return NULL;
+        }
     } else {
         FileWriter out(args.file());
         if (!out.is_open()) {
-            error = Error("Could not open output file");
-        } else {
-            error = Profiler::instance()->runInternal(args, out);
+            return asprof_error("Could not open output file");
+        }
+        error = Profiler::instance()->runInternal(args, out);
+        if (!error) {
+            return NULL;
         }
-    }
-
-    if (attached) {
-        VM::detachThread();
-    }
-
-    if (!error) {
-        return NULL;
     }
 
     return asprof_error(error.message());

src/flightRecorder.cpp

@@ -745,7 +745,7 @@ class Recording {
     }
 
     void writeJvmInfo(Buffer* buf) {
-        if (_agent_properties == NULL && !(VM::loaded() && parseAgentProperties())) {
+        if (_agent_properties == NULL && !(VMCapabilities::available() && parseAgentProperties())) {
             return;
         }
 

src/profiler.cpp

@@ -1038,7 +1038,7 @@ Engine* Profiler::activeEngine() {
 }
 
 Error Profiler::checkJvmCapabilities() {
-    if (VM::loaded()) {
+    if (VMCapabilities::available()) {
         if (!VMStructs::hasJavaThreadId()) {
             return Error("Could not find Thread ID field. Unsupported JVM?");
         }
@@ -1068,9 +1068,10 @@ Error Profiler::start(Arguments& args, bool reset) {
         return Error("Profiler already started");
     }
 
+    VMCapabilities vm_capabilities;
     // If profiler is started from a native app, try to detect a running JVM and attach to it
     if (!VM::loaded()) {
-        VM::tryAttach();
+        vm_capabilities.tryAttach();
     }
 
     Error error = checkJvmCapabilities();
@@ -1084,6 +1085,8 @@ Error Profiler::start(Arguments& args, bool reset) {
         return Error("No profiling events specified");
     } else if ((_event_mask & (_event_mask - 1)) && args._output != OUTPUT_JFR) {
         return Error("Only JFR output supports multiple events");
+    } else if (VM::loaded() && !VMCapabilities::available() && (_event_mask & (EM_ALLOC | EM_LOCK))) {
+        return Error("Could not attach to the JVM");
     } else if (!VM::loaded() && (_event_mask & (EM_ALLOC | EM_LOCK))) {
         return Error("Profiling event is not supported with non-Java processes");
     }
@@ -1166,6 +1169,8 @@ Error Profiler::start(Arguments& args, bool reset) {
         return Error("target-cpu is only supported with perf_events");
     } else if (_engine != &perf_events && args._record_cpu) {
         return Error("record-cpu is only supported with perf_events");
+    } else if (_engine == &instrument && VM::loaded() && !vm_capabilities.available()) {
+        return Error("Could not attach to the JVM");
     }
 
     _cstack = args._cstack;
@@ -1242,6 +1247,7 @@ Error Profiler::start(Arguments& args, bool reset) {
         startTimer();
     }
 
+    _started_with_vm = vm_capabilities.available();
     return Error::OK;
 
 error5:
@@ -1274,6 +1280,11 @@ Error Profiler::stop(bool restart) {
         return Error("Profiler is not active");
     }
 
+    VMCapabilities vm_capabilities;
+    if (_started_with_vm && !vm_capabilities.available()) {
+        return Error("Profiler started with VM, unable to attach to VM, couldn't stop");
+    }
+
     uninstallTraps();
 
     if (_event_mask & EM_WALL) wall_clock.stop();
@@ -1313,6 +1324,7 @@ Error Profiler::check(Arguments& args) {
         return Error("Profiler already started");
     }
 
+    VMCapabilities vm_capabilities;
     Error error = checkJvmCapabilities();
 
     if (!error && args._event != NULL) {
@@ -1369,6 +1381,8 @@ Error Profiler::dump(Writer& out, Arguments& args) {
         return Error("Profiler has not started");
     }
 
+    VMCapabilities vm_capabilities;
+
     if (_state == RUNNING) {
         updateJavaThreadNames();
         updateNativeThreadNames();
@@ -1803,7 +1817,7 @@ u64 Profiler::addTimeout(u64 start_micros, int timeout) {
 }
 
 void Profiler::startTimer() {
-    if (VM::loaded()) {
+    if (VMCapabilities::available()) {
         JNIEnv* jni = VM::jni();
         jclass Thread = jni->FindClass("java/lang/Thread");
         jmethodID init = jni->GetMethodID(Thread, "<init>", "(Ljava/lang/String;)V");

src/profiler.h

@@ -51,6 +51,7 @@ class Profiler {
   private:
     Mutex _state_lock;
     State _state;
+    bool _started_with_vm;
     Trap _begin_trap;
     Trap _end_trap;
     bool _nostop;
@@ -157,6 +158,7 @@ class Profiler {
   public:
     Profiler() :
         _state(NEW),
+        _started_with_vm(true),
         _begin_trap(2),
         _end_trap(3),
         _thread_filter(),

src/tsc.cpp

@@ -21,7 +21,7 @@ void TSC::enable(Clock clock) {
     }
 
     if (!_initialized) {
-        if (VM::loaded()) {
+        if (VMCapabilities::available()) {
             JNIEnv* env = VM::jni();
 
             jfieldID jvm;

src/vmEntry.cpp

@@ -32,7 +32,7 @@ int VM::_hotspot_version = 0;
 bool VM::_openj9 = false;
 bool VM::_zing = false;
 
-GetCreatedJavaVMs VM::_getCreatedJavaVMs = NULL;
+GetCreatedJavaVMs VMCapabilities::_getCreatedJavaVMs = NULL;
 
 jvmtiError (JNICALL *VM::_orig_RedefineClasses)(jvmtiEnv*, jint, const jvmtiClassDefinition*);
 jvmtiError (JNICALL *VM::_orig_RetransformClasses)(jvmtiEnv*, jint, const jclass* classes);
@@ -110,7 +110,7 @@ static void* resolveMethodIdEnd() {
 
 // Workaround for JDK-8308341: since JNI_GetCreatedJavaVMs may return an uninitialized JVM,
 // we verify the readiness of the JVM by presence of "VM Thread" and "Service Thread".
-bool VM::hasJvmThreads() {
+bool VMCapabilities::hasJvmThreads() {
     char thread_name[32];
     int threads_found = 0;
 
@@ -302,7 +302,7 @@ bool VM::init(JavaVM* vm, bool attach) {
 }
 
 // Try to find a running JVM instance and attach to it
-void VM::tryAttach() {
+void VMCapabilities::tryAttach() {
     if (_getCreatedJavaVMs == NULL) {
         void* lib_handle = dlopen(OS::isLinux() ? "libjvm.so" : "libjvm.dylib", RTLD_LAZY | RTLD_NOLOAD);
         if (lib_handle != NULL) {
@@ -324,11 +324,16 @@ void VM::tryAttach() {
     jint get_env_result = vm->GetEnv((void**)&env, JNI_VERSION_1_6);
     if (get_env_result == JNI_OK) {
         // Current thread already belongs to the running JVM
-        VM::init(vm, true);
+        if (VM::init(vm, true)) {
+            _available = true;
+        }
     } else if (get_env_result == JNI_EDETACHED) {
         // There is a running JVM, but we need to check it is initialized
         if (hasJvmThreads() && vm->AttachCurrentThreadAsDaemon((void**)&env, NULL) == JNI_OK) {
-            VM::init(vm, true);
+            if (VM::init(vm, true)) {
+                _available = true;
+                _attached = true;
+            }
         }
     }
 }
@@ -454,6 +459,36 @@ jvmtiError VM::RetransformClassesHook(jvmtiEnv* jvmti, jint class_count, const j
     return result;
 }
 
+thread_local bool VMCapabilities::_available = false;
+
+VMCapabilities::VMCapabilities() : _attached(false) {
+    _available = false;
+
+    if (!VM::loaded()) {
+        return;
+    }
+
+    if (VM::jni()) {
+        _available = true;
+        return;
+    }
+
+    if (VM::attachThread()) {
+        _attached = true;
+        _available = true;
+        return;
+    }
+}
+
+VMCapabilities::~VMCapabilities() {
+    _available = false;
+
+    if (_attached) {
+        VM::detachThread();
+    }
+}
+
+
 extern "C" DLLEXPORT jint JNICALL
 Agent_OnLoad(JavaVM* vm, char* options, void* reserved) {
     if (!_global_args._preloaded) {

src/vmEntry.h

@@ -105,16 +105,13 @@ class VM {
     static bool _openj9;
     static bool _zing;
 
-    static GetCreatedJavaVMs _getCreatedJavaVMs;
-
     static jvmtiError (JNICALL *_orig_RedefineClasses)(jvmtiEnv*, jint, const jvmtiClassDefinition*);
     static jvmtiError (JNICALL *_orig_RetransformClasses)(jvmtiEnv*, jint, const jclass* classes);
 
     static void ready();
     static void applyPatch(char* func, const char* patch, const char* end_patch);
     static void loadMethodIDs(jvmtiEnv* jvmti, JNIEnv* jni, jclass klass);
     static void loadAllMethodIDs(jvmtiEnv* jvmti, JNIEnv* jni);
-    static bool hasJvmThreads();
 
   public:
     static AsyncGetCallTrace _asyncGetCallTrace;
@@ -123,8 +120,6 @@ class VM {
 
     static bool init(JavaVM* vm, bool attach);
 
-    static void tryAttach();
-
     static bool loaded() {
         return _jvmti != NULL;
     }
@@ -192,4 +187,23 @@ class VM {
     static jvmtiError JNICALL RetransformClassesHook(jvmtiEnv* jvmti, jint class_count, const jclass* classes);
 };
 
+class VMCapabilities {
+  private:
+    static GetCreatedJavaVMs _getCreatedJavaVMs;
+    static thread_local bool _available;
+    bool _attached;
+
+    static bool hasJvmThreads();
+
+  public:
+    VMCapabilities();
+    ~VMCapabilities();
+
+    void tryAttach();
+
+    static bool available() {
+        return _available;
+    }
+};
+
 #endif // _VMENTRY_H

test/test/nonjava/NonjavaTests.java

@@ -75,4 +75,15 @@ public void startDifferentThread(TestProcess p) throws Exception {
         Output out = p.readFile("%profile");
         assert out.contains("cpuHeavyTask");
     }
+
+    // Profile is re-started on a different native thread from the one that started the JVM/profiler,
+    // and profiler is stopped on a 3rd different native thread
+    @Test(sh = "%testbin/non_java_app 7 %profile.jfr", output = true)
+    public void startStopDifferentThreadsJfr(TestProcess p) throws Exception {
+        p.waitForExit();
+        assert p.exitCode() == 0;
+
+        Output out = Output.convertJfrToCollapsed(p.getFilePath("%profile"));
+        assert out.contains("cpuHeavyTask");
+    }
 }