Add premain for static agent support #8988

ziyilin · 2024-05-29T07:42:17Z

Java agent always has a premain method to initialize the agent. SVM needs the premain as well to support agent in native image.

At compile time, -H:PremainClasses= option is used to set the premain classes.
At runtime, premain runtime options are set along with main class' arguments in the format of -XX-premain:[class]:[options]

This PR is part of #8077

vjovanov · 2024-06-12T05:56:15Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+
+ private static final String PREMAIN_OPTION_PREFIX = "-XX-premain:";
+
+ class PremainMethod {


This could be a record, right?

Should be static if it is a class.

This could be a record, right?
Yes, in my current implementation it's recorded by native-image-agent.

This could be a record, right?

Yes, in my current implementation it's recorded by native-image-agent.

When I say a record I mean a java record.

I see. Yes, this is a java record. I have fixed it.

vjovanov · 2024-06-12T06:00:32Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ // premain method must be static
+ premainMethod.method.invoke(null, args);
+ } catch (Throwable t) {
+ VMError.shouldNotReachHere("Fail to execute " premainMethod.className ".premain", t);


This should be a user-facing error, right? User code can throw, so I don't see this as something that is a VMError. What does the JVM do in this case?

In JVM, if the premain method fails, the JVM starting process shall be terminated. See https://github.com/openjdk/jdk21u/blob/2971cb5769121b47ac8c8db1078d67680a19341f/src/java.instrument/share/native/libinstrument/InvocationAdapter.c#L623-L629

I use VMError because it's a fatal error during VM starts up. But the cause is indeed an user error.

vjovanov · 2024-06-12T06:01:22Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ * registered premain method's second parameter. At native image runtime, no actual
+ * instrumentation work can do. So all the methods here are empty.
+ */
+ public static class SVMRuntimeInstrumentImpl implements Instrumentation {


Maybe NativeImageNoOpRuntimeInstrumentation? SVM is an internal name which we should not use. I also feel we should mention it is a noop and that it happens at runtime.

vjovanov · 2024-06-12T06:04:58Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/InstrumentFeature.java

+ }
+ preMainSupport.registerPremainMethod(premainClass, premain, args.toArray(new Object[0]));
+ } catch (ClassNotFoundException e) {
+ VMError.shouldNotReachHere("Can't register agent premain method, because the declaring class " premainClass " is not found", e);


This is a UserError as the user can make a mistake. We should also add a sentence on how to recover from that mistake to the error message.

vjovanov · 2024-06-12T06:43:39Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ * <ul>
+ * <li>Isolate code by checking current runtime. For example: <code>
+ * <pre>
+ * String vm = System.getProperty("java.vm.name");


We should guide the users to use the properties from ImageInfo. Then they can see if the agent is applied at build time or at runtime. We also need to make sure that this property is set before premain is called for the Native Image builder.

Fixed. This property can be obtained in premain.

vjovanov · 2024-06-12T06:45:25Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ * Keep premain options and return the rest args as main args.
+ * The premain options format:
+ * <br>
+ * -XX-premain:[full.qualified.premain.class]:[premain options]


I am not sure if this should be our final format for the flag. I think we can use it until we complete the feature and decide what is the user interface in the end.

Agreed. I added a //todo for future changing.

vjovanov · 2024-06-12T06:46:32Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/InstrumentFeature.java

+
+ public static class Options {
+ @Option(help = "Specify premain-class list. Multiple classes are separated by comma, and order matters.")//
+ public static final HostedOptionKey<LocatableMultiOptionValue.Strings> PremainClasses = new HostedOptionKey<>(null);


This is also an internal option. In the end we should probably pick the premain functions from the agent JARs.

sor-bae · 2024-06-14T04:41:18Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/InstrumentFeature.java

+ @Override
+ public void afterRegistration(AfterRegistrationAccess access) {
+ FeatureImpl.AfterRegistrationAccessImpl a = (FeatureImpl.AfterRegistrationAccessImpl) access;
+ cl = a.getImageClassLoader().getClassLoader();


I'm not sure this is still required, but I had to provide an agent JAR twice as -J-javaagent and -cp for NI build configurations in agent JARs to be picked up by the NI builder. So just checking, users can provide an agent JAR only once as -J-javaagent for their premain classes to be registered?

In this PR, we are not automatically picking agent from -cp or -J-javaagent. It will be handled in the following PRs. But I'm sure user doesn't need to provide agent jar multiple times.

vjovanov · 2024-06-17T15:40:07Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/InstrumentFeature.java

+ }
+ preMainSupport.registerPremainMethod(premainClass, premain, args.toArray(new Object[0]));
+ } catch (ClassNotFoundException e) {
+ UserError.abort(e,"Can't register agent premain method, because the given class %s is not found. Please check your -H:%s setting.", premainClass, Options.PremainClasses.getName());


It is better to use SubstrateOptionsParser.commandArgument to print the option name. This keeps the correct behavior when options become public or change the type.

vjovanov · 2024-06-17T16:42:52Z

To keep this feature working as expected we need to add a few tests into: com.oracle.svm.test

The tests can be executed from similarly to the tests for class initialization.

ziyilin · 2024-06-19T08:59:56Z

To keep this feature working as expected we need to add a few tests into: com.oracle.svm.test

The tests can be executed from similarly to the tests for class initialization.

I have added a test task agenttest. It now only checks the premain method, doesn't do any class transformation yet.

vjovanov · 2024-07-22T11:19:54Z

substratevm/mx.substratevm/mx_substratevm.py

@@ -1717,6 1717,66 @@ def cinterfacetutorial(args):
 native_image_context_run(_cinterfacetutorial, args)


+@mx.command(suite.name, 'agenttest', 'Runs the ')


I feel it would be more accurate to call it javaagenttest.
Also, we should complete the usage message here. Something like:

usage_msg='Runs tests for java agents with native image'

vjovanov · 2024-07-22T11:21:51Z

We need to add the CHANGELOG.md entry that claims that this is an experimental feature for now.

vjovanov · 2024-07-22T11:24:47Z

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/agent/Agent2.java

+ }
+ }
+ }
+ System.setProperty("instrument.enable", "true");


I would add a set of assertions that checks that the values for Instrumentation are correct at image run time. This way we assure nobody changes those values and surprises the users.

The check of Instrumentation class is put in the com.oracle.svm.test.agent.Agent.
I removed the Instrumentation parameter from Agent2 so it can test the case of premain without Instrumention.

I would still add a check for all the relevant methods that can be invoked on the Instrumentation good examples are: Instrumentation#isModifiableClass, Instrumentation#isRetransformClassesSupported, Instrumentation#isModifiableModule, Instrumentation#getAllLoadedClasses, etc.

ziyilin · 2024-07-23T02:43:13Z

We need to add the CHANGELOG.md entry that claims that this is an experimental feature for now.

Not sure which version and the internal code, I added the entry at the top of the CHANGELOG.md.

vjovanov · 2024-07-23T10:17:43Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+
+ @Override
+ public Class<?>[] getAllLoadedClasses() {
+ return new Class<?>[0];


I think we should discuss this. How is this method typically used?

Should we maybe return all the classes that were reached? I think that we need an explanation for the value we return.

In observability scenario, user can call this method to collect all jars loaded by the application.
I think return the reached classes in native image here is reasonable.

Should we maybe return all the classes that were reached?

this sounds good if it's possible (and if there's a desire to go beyond no-op implementation)

vjovanov · 2024-07-23T10:29:04Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+
+ @Override
+ public long getObjectSize(Object objectToSize) {
+ return 0;


We should either return the actual value by looking into Native Image internals, or something completely unusable such as Long.MIN_VALUE. Is this information used to transform classes?

This method is rarely used.
It is not used by class transformation.
I've searched the source of opentelemetery java instrumentation and didn't find any usage as well.
In OpenJDK, it returns oop->size() * wordSize. What is the similar function in substratevm? We can do the similar calculation so it will be perfectly compatible with JVM. But considering the rarely usage, Long.MIN_VALUE may be also acceptable for now.

I'd suggest 0 in case anyone is summing over these values, otherwise I think they'll get negative overflow(?) by using MIN_VALUE

I set it to -1 as it's both invalid and won't get overflowed easily.

vjovanov · 2024-07-23T10:30:52Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ }
+
+ @Override
+ public void redefineModule(Module module, Set<Module> extraReads, Map<String, Set<Module>> extraExports, Map<String, Set<Module>> extraOpens, Set<Class<?>> extraUses,


I feel this should be a hard failure that will explain to the users what they should do (preferable).

Now, the question is how many agents will call this even when transformation is not allowed. I suspect not many.

This method is called to declare extra exports/opens/uses/reads packages from module.
It is not rarely used. The agent may need extra access to certain modules.
We can call APIs from ModuleNative to make checks only.

I'd suggest throw new UnsupportedOperationException() based on the javadoc for the method https://docs.oracle.com/javase/8/docs/api/java/lang/instrument/Instrumentation.html#redefineClasses-java.lang.instrument.ClassDefinition...-

UnsupportedOperationException - if the current configuration of the JVM does not allow redefinition (isRedefineClassesSupported() is false) or the redefinition attempted to make unsupported changes

vjovanov · 2024-07-23T10:32:24Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ }
+
+ /**
+ * This class is a dummy implementation of {@link Instrumentation} interface. It serves as the


We need to mention in the release notes that this is experimental and that the behavior of some methods might change. We should test only the methods for which we are completely certain they will stay the same.

vjovanov · 2024-07-23T10:33:16Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ * <ul>
+ * <li>Isolate code by checking current runtime. For example: <code>
+ * <pre>
+ * if (ImageInfo.inImageRuntimeCode()) {


I would prefer to use the system property directly here, or at least mention it. This will not require extra dependencies to use this feature.

vjovanov · 2024-07-23T10:33:37Z

substratevm/CHANGELOG.md

@@ -3,6 3,7 @@
 This changelog summarizes major changes to GraalVM Native Image.

 ## GraalVM for JDK 24 (Internal Version 24.2.0)
+* Together with Alibaba, we added java agent support for native image as experimental feature. 


I will polish this line when the PR is done.

trask · 2024-07-25T01:38:41Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ * registered premain method's second parameter. At native image runtime, no actual
+ * instrumentation work can do. So all the methods here are empty.
+ */
+ public static class NativeImageNoOpRuntimeInstrumentation implements Instrumentation {


I think no-op is ok for everything, under the assumption that agents can't do "runtime instrumentation" things in native images

vjovanov · 2024-07-30T12:26:06Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ @Override
+ public Class<?>[] getAllLoadedClasses() {
+ synchronized (this) {
+ if (allLoadedClasses == null) {


I would not cache here as it takes up RSS.

vjovanov · 2024-07-30T12:29:22Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/ClassForNameSupport.java

@@ -213,4 215,15 @@ public RuntimeConditionSet getConditionFor(Class<?> jClass) {
 return conditionalClass.getConditions();
 }
 }
+
+ public Class<?>[] getAllLoadedClasses() {


I would implement this as follows:

ArrayList<Class<?>> userClasses = new ArrayList<>(); CodeInfo imageCodeInfo = CodeInfoTable.getFirstImageCodeInfo(); while (imageCodeInfo.isNonNull()) { Class<?>[] classes = NonmovableArrays.heapCopyOfObjectArray(CodeInfoAccess.getClasses(imageCodeInfo)); if (classes != null) { for (Class<?> clazz : classes) { if (clazz != null) { Module module = clazz.getModule(); if (module == null || module.getName() == null || !isSystemClass(module)) { userClasses.add(clazz); } } } } imageCodeInfo = CodeInfoAccess.getNextImageCodeInfo(imageCodeInfo); } userClasses.trimToSize(); return userClasses.toArray(new Class[0]);

where we have

private static final Set<String> systemModules = Set.of("org.graalvm.nativeimage.builder", "org.graalvm.nativeimage", "org.graalvm.nativeimage.base", "com.oracle.svm.svm_enterprise", "org.graalvm.word", "jdk.internal.vm.ci", "jdk.graal.compiler", "com.oracle.graal.graal_enterprise"); private static boolean isSystemClass(Module module) { return systemModules.contains(module.getName()); }

Please re-use systemModules in com.oracle.svm.hosted.classinitialization.ClassInitializationSupport#isAlwaysReached. @loicottet we can use that list later for the list of types where reflection is prohibited.

CC @christianwimmer

Fixed. I put the getAllLoadedClasses implementation to PremainSupport, because it doesn't need anything from ClassForNameSupport any more.

vjovanov · 2024-07-30T12:31:08Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ }
+
+ @Override
+ public boolean isModifiableModule(Module module) {


@ivan-ristovic can you please look at this implementation?

LGTM. This is the same implementation as in java.instrument API, and our run-time module system has no specific quirks that require divergence from the JDK for this method.

vjovanov · 2024-07-30T12:33:38Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ }
+
+ @Override
+ public void appendToBootstrapClassLoaderSearch(JarFile jarfile) {


I would throw here as it is not really supported as well as to appendToSystemClassLoaderSearch.

vjovanov · 2024-07-30T12:35:12Z

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/agent/Agent.java

+ * defined.
+ */
+ Assert.assertNotNull(inst);
+ Assert.assertEquals("com.oracle.svm.core.PreMainSupport$NativeImageNoOpRuntimeInstrumentation", inst.getClass().getName());


Implementation details, I would avoid this.

vjovanov · 2024-07-30T12:41:33Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+public class PreMainSupport {
+
+ // todo: Not finally decided, could be changed in the future
+ private static final String PREMAIN_OPTION_PREFIX = "-XX-premain:";


I would go with -XXpremain: to be consistent with other arguments.

Once we make a flag for build-time transformation this should be enabled automatically for enabled agents. Option will be there only for passing the arguments.

vjovanov · 2024-07-30T12:43:28Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/InstrumentFeature.java

+ public static class Options {
+ // todo: Not finally decided, could be changed in the future
+ @Option(help = "Specify premain-class list. Multiple classes are separated by comma, and order matters.")//
+ public static final HostedOptionKey<AccumulatingLocatableMultiOptionValue.Strings> PremainClasses = new HostedOptionKey<>(


This should be an experimental option until we add an API option for this.

vjovanov · 2024-07-30T12:43:42Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/InstrumentFeature.java

+ private PreMainSupport preMainSupport;
+
+ public static class Options {
+ // todo: Not finally decided, could be changed in the future


Please remove all todos.

vjovanov · 2024-08-02T09:06:04Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+
+ @Override
+ public void appendToBootstrapClassLoaderSearch(JarFile jarfile) {
+ throw new UnsupportedOperationException("Native image doesn't support modify classloader search path at runtime.");


Maybe Native image doesn't support modification of the classloader search path at run time. Please avoid calling this method in Native Image by checking "runtime".equals(System.getProperty("org.graalvm.nativeimage.imagecode"))

vjovanov · 2024-08-02T09:06:38Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+
+ @Override
+ public void appendToSystemClassLoaderSearch(JarFile jarfile) {
+ throw new UnsupportedOperationException("Native image doesn't support modify classloader search path at runtime.");


Similar here, as a matter of fact I would add the action sentence to all UnsupportedOperationExceptions

vjovanov · 2024-08-02T09:07:53Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/PreMainSupport.java

+ }
+ imageCodeInfo = CodeInfoAccess.getNextImageCodeInfo(imageCodeInfo);
+ }
+ userClasses.trimToSize();


Now I see we don't need the trimToSize as the list is thrown away.

vjovanov · 2024-08-02T09:11:10Z

This PR makes a good foundation for the agent support. After the final changes I will run all the tests, polish the CHANGELOG.md entry, and start with merging.

Java agent always has a premain method to initialize the agent. SVM needs the premain as well to support agent in native image. At compile time, -H:PremainClasses= option is used to set the premain classes. At runtime, premain runtime options are set along with main class' arguments in the format of -XXpremain:[class]:[options]

vjovanov

LGTM. Any last comments from other reviewers?

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 29, 2024

vjovanov reviewed Jun 12, 2024

View reviewed changes

sor-bae reviewed Jun 14, 2024

View reviewed changes

ziyilin force-pushed the staticInstrument-premain branch 2 times, most recently from 8e2bd2f to c9d5652 Compare June 14, 2024 09:39

vjovanov reviewed Jun 17, 2024

View reviewed changes

ziyilin force-pushed the staticInstrument-premain branch from c9d5652 to 538ad3f Compare June 19, 2024 08:52

ziyilin force-pushed the staticInstrument-premain branch 3 times, most recently from 510b031 to bc9e8ad Compare June 26, 2024 08:22

vjovanov reviewed Jul 22, 2024

View reviewed changes

ziyilin force-pushed the staticInstrument-premain branch 2 times, most recently from f69fbd2 to b05de40 Compare July 23, 2024 07:10

vjovanov reviewed Jul 23, 2024

View reviewed changes

vjovanov requested a review from christianwimmer July 23, 2024 10:34

trask reviewed Jul 25, 2024

View reviewed changes

ziyilin force-pushed the staticInstrument-premain branch 3 times, most recently from 246d942 to 1553530 Compare July 26, 2024 09:57

vjovanov reviewed Jul 30, 2024

View reviewed changes

ziyilin force-pushed the staticInstrument-premain branch 2 times, most recently from 78461d0 to afca03a Compare July 31, 2024 08:24

vjovanov reviewed Aug 2, 2024

View reviewed changes

vjovanov previously approved these changes Aug 2, 2024

View reviewed changes

ziyilin force-pushed the staticInstrument-premain branch from afca03a to e3dc8e7 Compare August 2, 2024 09:39

ziyilin dismissed vjovanov’s stale review via cd5430e August 5, 2024 02:52

ziyilin force-pushed the staticInstrument-premain branch from e3dc8e7 to cd5430e Compare August 5, 2024 02:52

vjovanov approved these changes Aug 6, 2024

View reviewed changes


		private static final String PREMAIN_OPTION_PREFIX = "-XX-premain:";

		class PremainMethod {

		@@ -1717,6 1717,66 @@ def cinterfacetutorial(args):
		native_image_context_run(_cinterfacetutorial, args)


		@mx.command(suite.name, 'agenttest', 'Runs the ')

Add premain for static agent support #8988

Are you sure you want to change the base?

Add premain for static agent support #8988

Conversation

ziyilin commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjovanov Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjovanov commented Jun 17, 2024 • edited Loading

ziyilin commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjovanov commented Jul 22, 2024

Choose a reason for hiding this comment

ziyilin Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyilin commented Jul 23, 2024

vjovanov Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-ristovic Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjovanov commented Aug 2, 2024

vjovanov left a comment

Choose a reason for hiding this comment

vjovanov Jun 12, 2024 •

edited

Loading

vjovanov commented Jun 17, 2024 •

edited

Loading

ziyilin Jul 23, 2024 •

edited

Loading

vjovanov Jul 23, 2024 •

edited

Loading

trask Jul 25, 2024 •

edited

Loading

ivan-ristovic Jul 30, 2024 •

edited

Loading