The choice of JVM_StartThread function to intercept is not clear to me. This is a native implementation of Java Thread.start method. It called for every new Java thread, while we are interested in something that is preferably called only at JVM startup. On the other hand, JVM_StartThread calls pthread_start under the hood, and we already have a pthread_start hook.

In general, library hooks is a subtle tool, which should be used only in exceptional cases.

After some experimentation the pthread hooks don't seem to be a good option currently
However I do agree that using the JVM_StartThread is sub-optimal .

pthread_create
All threads that go through the pthread_create step were observed & found that only one thread has the JVM in a good enough state to attach to it (JVM is visible via JNI method) but using that thread to initiate the operation would result in a deadlock condition on the JNI GetEnv, please refer to the following stack dump for reference

#0  0x00007f0732f61377 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
#1  0x00007f073228e8fb in PlatformMonitor::wait(unsigned long) () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so
#2  0x00007f073222abc5 in Monitor::wait(unsigned long) () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so
#3  0x00007f07325ab642 in VMThread::wait_until_executed(VM_Operation*) () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so
#4  0x00007f07325ac003 in VMThread::execute(VM_Operation*) () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so
#5  0x00007f07320858eb in JvmtiEnvBase::enable_virtual_threads_notify_jvmti() () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so
#6  0x00007f073208e42b in JvmtiExport::get_jvmti_interface(JavaVM_*, void**, int) () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so
#7  0x00007f07306ba1e5 in VM::init(JavaVM_*, bool) () from build/lib/libasyncProfiler.so
#8  0x00007f07306ba28f in VM::VMManualLoad(bool) () from build/lib/libasyncProfiler.so
#9  0x00007f07306ba337 in pthread_create_hook(unsigned long*, pthread_attr_t const*, void* (*)(void*), void*) () from build/lib/libasyncProfiler.so
#10 0x00007f0732282c4f in os::create_thread(Thread*, os::ThreadType, unsigned long) () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so
[#11](https://amzn-aws.slack.com/archives/C03K52T0BV2) 0x00007f0731f7bca9 in JVM_StartThread () from /usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so

** pthread_start**
Using this method will result segmentation fault when trying to initialize the JVM with the async profiler.
=> A deeper investigation will be needed on why this happens & how to prevent it.

pthread_exit
Using this method is too late in many case as it could happen at the end of the life cycle of the application being profiled.

It's not very reliable to look for any occurrence of a string. It should be better to check if a string ends with /libjava.so

If load does not succeed for any reason, we will keep calling it again and again, causing extra overhead for every thread start.

That is by intention as the JVM internally start multiple threads.
Not all threads will be started when the JVM is fully initialized and fully available via the JNI APIs.

However I agree that we should add a counter for example to prevent trying too many times.

This is not guaranteed to work with all JVMs, since libHandle here is a handle to libjava.so, whereas JNI_GetCreatedJavaVMs and JVM_StartThread are defined in libjvm.so.

Regardless of it being already the third copy of a patching loop in the sources, the whole idea of going through all loaded libraries does not seem right to me, since JVM_StartThread is called from one library only - libjava.so.

attach=true is for cases when we are attaching to a running JVM.
But what about a case when JVM is started after profiler?

I mean, this code is supposed to handle the case when JVM is just being started. So can find an earlier hook where we are able to call VM::init with attach=false?

When implementing the Thread solution which handles then Java after Async profiler case
It was observed that the that the JVM will already be in a running state when it's first detected via the Thread

As suggested this PR currently only handles the JVM starting first case
So in this case the attache will always be true as the VM is running

Not the best place for the call. This function is specifically for setting up signal handlers.
asprof_init() looks a better choice.

I realized that my previous suggestion about asprof_init is also imperfect, because asprof_init is normally called only once, but we want to check if JVM has been loaded in the beginning of every profiling session. Sorry for the confusion.
So the right location will be Profiler::start.

IMO, name choice is not great (double "VM"; what is "manual load"?)
Maybe VM::detect or VM::tryAttach?

Use typedef to avoid repeating cryptic declaration.

I wonder why RTLD_DEFAULT works. What if libjvm is loaded with dlopen(..., RTLD_LOCAL)?

I did some checking here, it's due to the compile command I'm using which will use the RTLD_GLOBAL
You're right this needs to change to account for the fact it's possible that JVM will be loaded in a RTLD_LOCAL manner

Since this function will be called on every profiling session of a native app, it should use something lightweight to detect if libjvm is loaded. Check what is the most efficient: dlopen("libjvm.so", RTLD_NOLOAD), dlsym, dl_iterate_phdr or something else.

Problems that could happen:

1. libjvm.so is initially loaded with RTLD_LOCAL option void* libHandle = dlopen("/usr/lib/jvm/java-21-amazon-corretto/lib/server/libjvm.so", RTLD_NOW | RTLD_LOCAL);
2. dlsym won’t detect the JNI_GetCreatedJavaVMs function as the libjvm.so isn’t visible in the async-profiler scope => check first point
3. dl_iterate_phdr will allow us to detect if libjvm.so was indeed loaded but won’t guarantee the loading options => check first point

Suggested Solution Steps:

1. Limit the amount of dlopen & dlsym operation to at most to 2 operations of each.
   The point above can be handled via simply adding a Flag that is initially TRUE.
   If the Flag is turned on the async-profiler on the next profiling session will attempt to load the JNI_GetCreatedJavaVMs function.
   This however will require first loading the libjvm.so first due the first problem mentioned above.

If the load operation is successful the function reference can be stored & used in future profiling session starts to try & detect the JVM to load it into async-profiler.
If the operation is a failure the Flag is turned off (LIB or function wasn’t found via the respective dlopen & dlsym to prevent this loading from happening again in future profiling sessions.

2. If a dlopen was detected on the libjvm between profiling sessions the flag can be turned back on for a reattempt of Step 1

Known issues & fixes if possible:

1. If libjvm is detected mid profiling session it won’t attach until a new profiling session starts, so the JVM symbols won’t be visible for the initial profiling session
   Suggested fix is to find a method that could be hooked into to detect that the JVM is ready for use & attach it mid profiling session to the async-profiler
   => TODO: this should be studied in depth & handled in a different PR
2. libjvm is decide but a JVM is never started.
   To prevent constant calls to try to load the JVM (JNI_GetCreatedJavaVMs was detected successfully) over many profiling sessions, a simple stopgap solution of a counter could be introduced to limit the attempts to detect & load the JVM to maximum amount X -> I would suggest 5 attempts here at maximum

@apangin what do you think?

What is "none native"? You mean "non-native"? There are Java applications and Native (or Non-Java) applications, but "non-native" sounds unusual.

The comment is misleading: tryAttach does not load a JVM, it rather checks if the JVM is already loaded.

Local variables follow snake_case naming convention.

This should be called only if the above dlopen succeeds.

Again, the comment is misleading.

Javadoc-style comments do not make sense for C++ code.

IIRC, this won't compile with some older gcc. There should not be whilespace before #.
Btw, inline #ifdefs in a method implementation are discouraged anyway.

#ifdef inside a function again.
Since you use this for choosing between .so/.dylib in multiple places, it may be better to declare a platform-specific constant or a macro at the top of the file.

Why creating an instance? These lines won't be needed if you declare the method static.

Exception must be checked after JNI-to-Java upcall.
You may run program with -Xcheck:jni to find potential issues with JNI code.

In what sense it is "common"?
Having 3 source files for a couple of tests seems an overkill. Can it be just one .cpp file?

Use PATH_MAX limit when constructing paths.

This may break test, since JAVA_HOME is not always set.

Will not work with JDK 8, where there is additional jre directory on the path.

That's correct.
I will make do the changes to prepare to make it work with JDK 8 in the future
However as if now Async-profiler doesn't compile using JDK-8, so as if now JDK-8 can't be used to run the tests

Some changes will be needed on the Async-profiler as a whole to support compiling & running the tests in JDK 8

It's true that async-profiler requires JDK 11+ to compile. This is because it uses JDK 11 APIs.
However, this does not mean tests cannot run with JDK 8. async-profiler compiled with JDK 11 can perfectly run on JDK 8.

Why 10?

Please note in JNI_OnLoad currently is using JNI_VERSION_1_6
This is because JNI_VERSION depends on the JDK version
In my case JNI_VERSION_10 was used as it the highest common version between JDK11 -> JDK21

I will make the value more dynamic depending the JDK version
But note this will require redefining some constants to avoid compile errors

You've listed all possible JNI exception functions :)
ExceptionDescribe is enough. It does everything: prints exception message with a stack trace and clears current exception.

New line character at EOF is missing

Why is $(LIBS) needed here?

"manual load" is unclear.
Anyways, I'd remove debug printout altogether.

Attaching thread to a JVM has side effects: e.g. it becomes visible in Java thread dumps, JDK Flight Recorder starts to report this thread, etc. If this is needed only to call VM::init, make sure the thread is detached as soon as initialization completes. But note that a thread may be already owned by the JVM - in this case, it must not be detached.

After checking JNI doc
I will remove this
The above _vm->GetEnv((void**)&_jvmti, JVMTI_VERSION_1_0) != 0 condition guarantees that the thread is already associated with the JVM

This is not fully correct way to get the JDK version.
Runner class already gets the JVM version here.
However, it's likely that you don't need this at all.

I will need it as to load the libjvm.so lib dynamically
The path is different between JDK8 & other versions on linux

=> it includes the arch as amd64 or i386 (x86) As well as aarch64 or arm (arm)

It's not good to have the architecture hard-coded.

Why is that? Can't you just always use JNI_VERSION_1_6?

Can be a constant. Also, double ; at the end.

"Wrapper" is a too generic word. It's not clear who wraps what.
It would be better to restructure the algorithm not to have wrappers at all. E.g., code that looks for JVM threads can be extracted in a separate function.

Doing that would effect the performance & readability of the normal init method
As we need to use vm->GetEnv((void**)&env, JNI_VERSION_1_6);

Using the GetEnv to get JVM TI isn't safe in this specific case

So moving this extra read & other thread logic outside the logic of original init is more readable IMO than adding a flag to only do this flow if the flag is enabled (Not having this wrapping will most likely result in more nesting in the init method)

I however agree that the initWrapper name is somewhat vague

sizeof(thread_name) instead of a constant.
Btw, 4096 looks too much when the longest thread name we care about is 20 characters.