Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v8]
On Thu, 17 Dec 2020 20:19:56 GMT, Mandy Chung wrote: >> 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. @mlchung Thank you for the suggestions. I've addressed them in the latest update. The `@library /test` is needed only in the tests that use NegativeAgentRunner as this class is located in folder one level up. I've removed the this line from the other tests. - PR: https://git.openjdk.java.net/jdk/pull/1694
Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v8]
On Thu, 17 Dec 2020 04:59:17 GMT, Serguei Spitsyn 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
Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v8]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1694/files - new: https://git.openjdk.java.net/jdk/pull/1694/files/09cf3fda..448bb8b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1694=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1694=06-07 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1694.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1694/head:pull/1694 PR: https://git.openjdk.java.net/jdk/pull/1694
