Make internal classloader instrumentation indy compatible

[JonasKunz]

These instrumentations were quite some fun ones.

They are slightly problematic, because they instrument ClassLoader.loadClass and ClassLoader.defineClass.
This is difficult with invokedynamic, because the bootstrapping of the invokedynamic instructions inserted for these instrumentations itself involves loading and defining of classes, causing an infinite recursion.

Here is how this PR avoid these recursions:

Avoiding ClassLoader.loadClass recursions

When an Advice is bootstrapped, ClassLoader.loadClass is invoked for loading the Advice-class and for initializing the dependencies of IndyBootsrap in case this is the very first bootstrapped advice. We ahve avoided this by:

• excluding AgentClassLoader and InstrumentationModuleClassloader from instrumentations instrumenting ClassLoader.loadClass overriddes
• Ensuring all AgentClassLoader and InstrumentationModuleClassloader override all overloads of ClassLoader.loadClass

The latter was needed because otherwise we would still get a recursion if invoking and overload, because it would fallback to the instrumented super method.
This is almost good enough, because it avoid the recursion for all classes loaded by the excluded loaders themselves. However, for classes bootstrap-classloader classes (such as CallDepth) used by IndyBootstrap.bootstrap we still would get a recursion, because then AgentClassLoader delegates to super.loadClass. This could occur on the very first invocation to IndyBootstrap.bootstrap if those classes are not linked yet. We avoid this by linking the corresponding classes (just CallDepth at the moment) eagerly in the static initializer of IndyBootstrap.

Avoiding ClassLoader.defineClass recursions

When an Advice is bootstrapped, the target Advice-class is loaded for the first time and therefore ClassLoader.defineClass is invoked for it. This therefore by defintion yields a invokedynamic bootstrapping recursion for the DefineClassInstrumentation.

We avoid this by adding a new mechanism to allow Advice-classes to not be loaded on the first invocation of the instrumented method, but instead already loading them when the invokedynamic bytecode is inserted. This already prevents the recursion, because then the loading of the Advice class during bootstrapping will not involve a defineClass call anymore.

[SylvainJuge]

[minor] maybe having a dedicated annotation for this could help, otherwise we just rely on a side-effect of having this ToReturned annotation present.

[JonasKunz]

I didn't want to introduce a separate mechanism here because this instrumentation is likely the very only case where this is needed and AdviceTransformer will be a temporary thing anyway

[SylvainJuge]

[for reviewer] without this the advice return type is assumed to be an array, which means skipOnIndex is added when using skipOn annotation attribute, and this breaks when advice is not inlined.

[SylvainJuge]

[minor] maybe add that this class needs to be explicitly initialized early because it's the one preventing recursive calls and is thus the one called first when loadClass advice method is executed. Adding a comment where this first call is made would also be relevant too just in case someone tries to add something before it.

Also, could we make this static init part of the advice static init instead ? If possible that would be easier to link with the advice method code. On the other hand this CallDepth init call might be needed elsewhere.

[JonasKunz]

It is in fact not the case that this class needs to be initialized early. It instead needs to be linked to IndyBootstrap early:
When some IndyBootstrap.bootstrapAdvice is executed for the first time, the JVM encounters a call to CallDepth.forClass. When linking this call, the JVM itself invokes AgentClassLoader.loadClass(CallDepth) to link that callsite. This in turn will delegate to the instrumented ClassLoader.loadClass because it is a bootstrap class. It doesn't matter where within bootstrapAdvice this call happens, it just happened by chance that it is the first one.

Also, could we make this static init part of the advice static init instead

For the reasons above: No, this is not possible, because it wouldn't link the usage of CallDepth from within the IndyBootstrap class.

[laurit]

Not related to this PR but I think we need to add a method to the experimental interface that lets instrumentation module declare that it is compatible with non-inline advice so we could disable the advice rewriting for it.

[JonasKunz]

I was thinking of it but felt like this instrumentation was a special case where this was needed and that would only be temporary until we remove support for non-indy instrumentations.

[laurit]

Actually I was thinking that the main use for it would be to track the indy conversion progress and ensuring that modules that were made indy compatible once aren't broken by later changes. I'm pretty sure there was at least one more module that use the same trick to disable the advice rewriting.

[laurit]

After changing the InstrumentationModuleClassLoader.defineClassWithPackage this probably isn't necessary any more. Either revert or add a comment that this is here to avoid calling super.loadClass.

[JonasKunz]

I've also removed the loadClass override in AgentClassLoader. Remove it in InstrumentationModuleClassLoader is not possible, because then the LoadClassAdvice itself becomes recursive (not the bootstrapping this time):

[otel.javaagent 2024-10-24 12:42:55:462 +0200] [main] DEBUG io.opentelemetry.javaagent.tooling.instrumentation.indy.IndyBootstrap - Fixing nested instrumentation invokedynamic instruction bootstrapping for instrumented class java.lang.ClassLoader and advice io.opentelemetry.javaagent.instrumentation.internal.classloader.LoadInjectedClassInstrumentation$LoadClassAdvice.onEnter, the instrumentaiton should be active now
[otel.javaagent 2024-10-24 12:42:55:463 +0200] [otel-javaagent-transform-safe-logger] DEBUG io.opentelemetry.javaagent.tooling.AgentInstaller$TransformLoggingListener - Failed to handle io.opentelemetry.javaagent.ClassLoaderData$$29539e36 for transformation on class loader io.opentelemetry.javaagent.tooling.util.ClassLoaderMap$1@29539e36
java.lang.StackOverflowError
	at io.opentelemetry.javaagent.instrumentation.internal.classloader.LoadInjectedClassInstrumentation$LoadClassAdvice.onEnter(LoadInjectedClassInstrumentation.java:59)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	at io.opentelemetry.javaagent.instrumentation.internal.classloader.LoadInjectedClassInstrumentation$LoadClassAdvice.onEnter(LoadInjectedClassInstrumentation.java:59)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	at io.opentelemetry.javaagent.instrumentation.internal.classloader.LoadInjectedClassInstrumentation$LoadClassAdvice.onEnter(LoadInjectedClassInstrumentation.java:59)
	...
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	at io.opentelemetry.javaagent.instrumentation.internal.classloader.LoadInjectedClassInstrumentation$LoadClassAdvice.onEnter(LoadInjectedClassInstrumentation.java:59)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	at io.opentelemetry.javaagent.instrumentation.internal.classloader.LoadInjectedClassInstrumentation$LoadClassAdvice.onEnter(LoadInjectedClassInstrumentation.java:59)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	at io.opentelemetry.javaagent.tooling.ignore.IgnoredClassLoadersMatcher.loadsExpectedClass(IgnoredClassLoadersMatcher.java:78)
	at io.opentelemetry.javaagent.tooling.ignore.IgnoredClassLoadersMatcher.delegatesToBootstrap(IgnoredClassLoadersMatcher.java:69)

If you prefer to, we could also use CallDepth in the LoadClassAdvice instead I think.

[laurit]

I think it is fine as it is for now.