On Fri, 18 Dec 2020 12:00:48 GMT, Serguei Spitsyn <sspit...@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
