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

[Alan Bateman]

On Wed, 16 Dec 2020 08:11:44 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Thanks, David!
>> Yes, I was also thinking that the setAccessible has to be remained after all the checks.
>
> I've pushed an update with the following changes:
> - InstrumentationImpl.java update with the process sequence suggested by David. More specifically: the premain/agentmain method public modifier is checked instead of m.canAccess, after that if the agent class is not public then the setAccessible is called to make the method invokeable;
> - The tests are refactored (tried to implement suggestion from Mandy to simplify the tests). It includes:
>    - new helper class are added to build agent jar file:  `test/jdk/java/lang/instrument/AgentJarBuilder.java`
>    - new helper class to run negative tests which are expected to throw an exception: `test/jdk/java/lang/instrument/NegativeAgentRunner.java`
>    - introduced in the first push new shell script is removed: `test/jdk/java/lang/instrument/PremainClass/MakeJAR.sh`
>    - the test runners with names *Test.java are removed, all the tests use jtreg commands instead (negative tests now use the NegativeAgentRunner)
>    - the test  `test/jdk/java/lang/instrument/NonPublicPremainAgent.java` is moved to the PremainClass sub-folder
>    - added new test to provide test coverage for non-public agent class:
>   `test/jdk/java/lang/instrument/PremainClass/NonPublicAgent.java`
> It might make sense to remove previously added public modifiers to several agent classes. However, I've decided to skip it for now. Please, let me know if you think it is desired thing to do.
> Mandy also suggested to consider using the exception message format produced by thrown by `jdk.internal.reflect.Reflection::newIllegalAccessException`. It looks like this: `Exception in thread "main" java.lang.IllegalAccessException: class sun.instrument.InstrumentationImpl (in module java.instrument) cannot access a member of class NonPublicAgent with modifiers "public static"`. Please, let me know if this format would be better.
> 
> Please, let me know if I missed anything. I feel that at least one more iteration will be needed anyway.

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.

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

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