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

[Mandy Chung]

On Fri, 18 Dec 2020 12:00:48 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:
> 
>   1. Suppress premain method check only if agent class is in unnamed module; 2. minor tests cleanup

The change looks okay with minor comments on the tests.    Nit: it will be good to update `@summary` in the InheritAgentxxxx tests that now fail to load the agent.  

W.r.t. the premain/agentmain method declared in the agent class, the specification clearly states that in the package summary:

> The agent class must implement a public static premain method similar in principle to the main application entry point.

> The agent class must implement a public static agentmain method.

test/jdk/java/lang/instrument/PremainClass/NonPublicAgent.java line 27:

> 25:  * @test
> 26:  * @bug 8165276
> 27:  * @summary Test that agent with non-public premain method is rejected to load

the comment is incorrect.  This tests a non-public agent class with a public premain method.   The agent is not rejected.

test/jdk/java/lang/instrument/RetransformAgent.java line 41:

> 39: import asmlib.*;
> 40: 
> 41: public class RetransformAgent {

Making RetransformAgent as public is not necessary.   This fix does not change this test and so better to revert it.

test/jdk/java/lang/instrument/SimpleAgent.java line 26:

> 24: import java.lang.instrument.Instrumentation;
> 25: 
> 26: public class SimpleAgent {

There is no other change in this test except making SImpleAgent as public which is not necessary.   So better to revert it.

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

Marked as reviewed by mchung (Reviewer).

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