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 02:23:33 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> David, thank you for catching this. I'm probably missing something here.
>> If the agent class is not public then the `m.canAccess(null)` check is not passed and IAE is thrown with the message:
>>   `Exception in thread "main" java.lang.IllegalAccessException: method NonPublicAgent.premain must be declared public`
>> 
>> But the `NonPublicAgent.premain` is declared public as below:
>>     public static void premain(String agentArgs, Instrumentation inst) {
>>         System.out.println("premain: NonPublicAgent was loaded");
>>     }
>> It seems, the IAE is thrown because the agent class is not public.
>> Does it mean the `m.canAccess(null)` check is not fully correct?
>
> 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

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.

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

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