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

[Alan Bateman]

On Sat, 12 Dec 2020 01:29:28 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 8165276 to @bug list of impacted tests

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

> 466:             String msg = "method " + classname + "." +  methodname + " must be declared public";
> 467:             throw new IllegalAccessException(msg);
> 468:         }

I think the updated patch looks much better. 

If the agent class doesn't declare a premain then NoSuchMethodException will be thrown.

The only thing that I'm wondering about now is the exception message when the agent class is not public. If I read the changes correctly it means that IllegalAccessException will be thrown saying that <class>.premain should be public. Is that correct? We might need to adjust to the exception message to make it clear, or put in an explicit then to ensure to test that the agent class is public.

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

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