RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]

[Serguei Spitsyn]

On Wed, 16 Dec 2020 19:44:04 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Mandy is correct, the bigger picture here is accessibility. Code in the java.instrument module should be able to invoke the premain or agentmain without needing to suppress access checks. It's a perquisite to supporting the deployment of java agents as modules. When an agent is deployed as a module it will need the agent class to be public in a package that is exported to at least the java.instrument module. The premain method will need to be public too.
>> So PR1694 is really just about aligning the spec so that the premain method is public. However, it does not fix the overall accessibility issue. Fixing the accessibility issue will fixing the java.lang.instrument package description to specify that the agent class is public.
>
>> So PR1694 is really just about aligning the spec so that the premain method is public. However, it does not fix the overall accessibility issue.
> 
> OK I see that JDK-5070281 changes to allow the agent class to be non-public as the main class that declares `public static void main`.   So `setAccessible` should only be called if the agent class is in an unnamed module.  
> 
> If the agent class is in a named module and not accessible to java.instrument module (if the java agent is packaged in a modular JAR file), it should fail with IAE - can you confirm?    I wanted to understand the current behavior.

Mandy, I've pushed updates with changes:
- NegativeAgentRunner checks if the exit value is non-zero
- AgentJarBuilder is replaced with JavaAgentBuilder
- removed unneeded qualified exports

> So setAccessible should only be called if the agent class is in an unnamed module.
Now the agent class is always loaded as in the unnamed module.
I tried to test this with a modular jar file which has module-info.class in it.
In fact, I don't know if there are any options that would help to load the premain agent class in named module.
I guess, it has to be implemented as part of the enhancement https://bugs.openjdk.java.net/browse/JDK-6932391 .

The following patch can be committed if you like as it does not harm:
@@ -476,11 +476,13 @@ public class InstrumentationImpl implements Instrumentation {
             String msg = "method " + classname + "." +  methodname + " must be declared public";
             throw new IllegalAccessException(msg);
         }
-
-        if ((javaAgentClass.getModifiers() & java.lang.reflect.Modifier.PUBLIC) == 0) {
-            // If the java agent class is not public, make the premain/agentmain
-            // accessible, so we can call it.
+        if ((javaAgentClass.getModifiers() & java.lang.reflect.Modifier.PUBLIC) == 0 &&
+            !javaAgentClass.getModule().isNamed()) {
+            // If the java agent class is not public and is in unnamed module
+            // then make the premain/agentmain accessible, so we can call it.
             setAccessible(m, true);
         }

-------------

PR: https://git.openjdk.java.net/jdk/pull/1694