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

[Mandy Chung]

On Thu, 17 Dec 2020 04:59:17 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is now):
>>    https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> ----------
>> 
>> The java.lang.instrument spec:
>>   https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>>     "The agent class must implement a public static premain method
>>      similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   added to NegativeAgentRunner exit value check to be non-zero

@sspitsyn  Thanks for the update.   I think it's good to add the unnamed module check before calling `setAccessible` that becomes clear that this check only intends for unnamed module.   Maybe make it clear in the comment something like this: 

    // if the java agent class is in an unnamed module, the java agent class can be non-public.
    // suppress access check upon the invocation of the premain/agentmain method

Since the agent class can be non-public, it means that it's not necessary to modify the test agent class to public as you did in this patch.

I don't think `@library /test` is needed, isn't it?  The test library classes are under test/lib.

src/java.instrument/share/classes/sun/instrument/InstrumentationImpl.java line 475:

> 473: 
> 474:         // reject non-public premain or agentmain method
> 475:         if ((m.getModifiers() & java.lang.reflect.Modifier.PUBLIC) == 0) {

An alternative way: `if (Modifier.isPublic(m.getModifiers()))`

Similar can be applied for checking if the javaagentClass is public.

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

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