Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]
On Wed, 16 Dec 2020 19:44:04 GMT, Mandy Chung wrote:
>> Mandy is correct, the bigger picture here is accessibility. Code in the
>> java.instrument module should be able to invoke the premain or agentmain
>> without needing to suppress access checks. It's a perquisite to supporting
>> the deployment of java agents as modules. When an agent is deployed as a
>> module it will need the agent class to be public in a package that is
>> exported to at least the java.instrument module. The premain method will
>> need to be public too.
>> So PR1694 is really just about aligning the spec so that the premain method
>> is public. However, it does not fix the overall accessibility issue. Fixing
>> the accessibility issue will fixing the java.lang.instrument package
>> description to specify that the agent class is public.
>
>> So PR1694 is really just about aligning the spec so that the premain method
>> is public. However, it does not fix the overall accessibility issue.
>
> OK I see that JDK-5070281 changes to allow the agent class to be non-public
> as the main class that declares `public static void main`. So
> `setAccessible` should only be called if the agent class is in an unnamed
> module.
>
> If the agent class is in a named module and not accessible to java.instrument
> module (if the java agent is packaged in a modular JAR file), it should fail
> with IAE - can you confirm?I wanted to understand the current behavior.
Mandy, I've pushed updates with changes:
- NegativeAgentRunner checks if the exit value is non-zero
- AgentJarBuilder is replaced with JavaAgentBuilder
- removed unneeded qualified exports
> So setAccessible should only be called if the agent class is in an unnamed
> module.
Now the agent class is always loaded as in the unnamed module.
I tried to test this with a modular jar file which has module-info.class in it.
In fact, I don't know if there are any options that would help to load the
premain agent class in named module.
I guess, it has to be implemented as part of the enhancement
https://bugs.openjdk.java.net/browse/JDK-6932391 .
The following patch can be committed if you like as it does not harm:
@@ -476,11 +476,13 @@ public class InstrumentationImpl implements
Instrumentation {
String msg = "method " + classname + "." + methodname + " must be
declared public";
throw new IllegalAccessException(msg);
}
-
-if ((javaAgentClass.getModifiers() &
java.lang.reflect.Modifier.PUBLIC) == 0) {
-// If the java agent class is not public, make the
premain/agentmain
-// accessible, so we can call it.
+if ((javaAgentClass.getModifiers() &
java.lang.reflect.Modifier.PUBLIC) == 0 &&
+!javaAgentClass.getModule().isNamed()) {
+// If the java agent class is not public and is in unnamed module
+// then make the premain/agentmain accessible, so we can call it.
setAccessible(m, true);
}
-
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 [v4]
On Wed, 16 Dec 2020 08:18:49 GMT, Alan Bateman wrote: > So PR1694 is really just about aligning the spec so that the premain method > is public. However, it does not fix the overall accessibility issue. OK I see that JDK-5070281 changes to allow the agent class to be non-public as the main class that declares `public static void main`. So `setAccessible` should only be called if the agent class is in an unnamed module. If the agent class is in a named module and not accessible to java.instrument module (if the java agent is packaged in a modular JAR file), it should fail with IAE - can you confirm?I wanted to understand the current behavior. - 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 [v4]
On Wed, 16 Dec 2020 08:11:44 GMT, Serguei Spitsyn wrote: >> Thanks, David! >> Yes, I was also thinking that the setAccessible has to be remained after all >> the checks. > > I've pushed an update with the following changes: > - InstrumentationImpl.java update with the process sequence suggested by > David. More specifically: the premain/agentmain method public modifier is > checked instead of m.canAccess, after that if the agent class is not public > then the setAccessible is called to make the method invokeable; > - The tests are refactored (tried to implement suggestion from Mandy to > simplify the tests). It includes: >- new helper class are added to build agent jar file: > `test/jdk/java/lang/instrument/AgentJarBuilder.java` >- new helper class to run negative tests which are expected to throw an > exception: `test/jdk/java/lang/instrument/NegativeAgentRunner.java` >- introduced in the first push new shell script is removed: > `test/jdk/java/lang/instrument/PremainClass/MakeJAR.sh` >- the test runners with names *Test.java are removed, all the tests use > jtreg commands instead (negative tests now use the NegativeAgentRunner) >- the test `test/jdk/java/lang/instrument/NonPublicPremainAgent.java` is > moved to the PremainClass sub-folder >- added new test to provide test coverage for non-public agent class: > `test/jdk/java/lang/instrument/PremainClass/NonPublicAgent.java` > It might make sense to remove previously added public modifiers to several > agent classes. However, I've decided to skip it for now. Please, let me know > if you think it is desired thing to do. > Mandy also suggested to consider using the exception message format produced > by thrown by `jdk.internal.reflect.Reflection::newIllegalAccessException`. It > looks like this: `Exception in thread "main" > java.lang.IllegalAccessException: class sun.instrument.InstrumentationImpl > (in module java.instrument) cannot access a member of class NonPublicAgent > with modifiers "public static"`. Please, let me know if this format would be > better. > > Please, let me know if I missed anything. I feel that at least one more > iteration will be needed anyway. Mandy is correct, the bigger picture here is accessibility. Code in the java.instrument module should be able to invoke the premain or agentmain without needing to suppress access checks. It's a perquisite to supporting the deployment of java agents as modules. When an agent is deployed as a module it will need the agent class to be public in a package that is exported to at least the java.instrument module. The premain method will need to be public too. So PR1694 is really just about aligning the spec so that the premain method is public. However, it does not fix the overall accessibility issue. Fixing the accessibility issue will fixing the java.lang.instrument package description to specify that the agent class 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 [v4]
On Wed, 16 Dec 2020 02:23:33 GMT, Serguei Spitsyn wrote:
>> David, thank you for catching this. I'm probably missing something here.
>> If the agent class is not public then the `m.canAccess(null)` check is not
>> passed and IAE is thrown with the message:
>> `Exception in thread "main" java.lang.IllegalAccessException: method
>> NonPublicAgent.premain must be declared public`
>>
>> But the `NonPublicAgent.premain` is declared public as below:
>> public static void premain(String agentArgs, Instrumentation inst) {
>> System.out.println("premain: NonPublicAgent was loaded");
>> }
>> It seems, the IAE is thrown because the agent class is not public.
>> Does it mean the `m.canAccess(null)` check is not fully correct?
>
> Thanks, David!
> Yes, I was also thinking that the setAccessible has to be remained after all
> the checks.
I've pushed an update with the following changes:
- InstrumentationImpl.java update with the process sequence suggested by David.
More specifically: the premain/agentmain method public modifier is checked
instead of m.canAccess, after that if the agent class is not public then the
setAccessible is called to make the method invokeable;
- The tests are refactored (tried to implement suggestion from Mandy to
simplify the tests). It includes:
- new helper class are added to build agent jar file:
`test/jdk/java/lang/instrument/AgentJarBuilder.java`
- new helper class to run negative tests which are expected to throw an
exception: `test/jdk/java/lang/instrument/NegativeAgentRunner.java`
- introduced in the first push new shell script is removed:
`test/jdk/java/lang/instrument/PremainClass/MakeJAR.sh`
- the test runners with names *Test.java are removed, all the tests use
jtreg commands instead (negative tests now use the NegativeAgentRunner)
- the test `test/jdk/java/lang/instrument/NonPublicPremainAgent.java` is
moved to the PremainClass sub-folder
It might make sense to remove previously added public modifiers to several
agent classes. However, I've decided to skip it for now. Please, let me know if
you think it is desired thing to do.
Mandy also suggested to consider using the exception message format produced by
thrown by `jdk.internal.reflect.Reflection::newIllegalAccessException`. It
looks like this: `Exception in thread "main" java.lang.IllegalAccessException:
class sun.instrument.InstrumentationImpl (in module java.instrument) cannot
access a member of class NonPublicAgent with modifiers "public static"`.
Please, let me know if this format would be better.
-
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 [v4]
Hi Mandy,
On 16/12/2020 12:54 pm, Mandy Chung wrote:
On 12/15/20 5:50 PM, David Holmes wrote:
On 16/12/2020 11:35 am, Serguei Spitsyn wrote:
On Wed, 16 Dec 2020 00:50:04 GMT, David Holmes
wrote:
The agent class doesn't have to be public it just has to be accessible.
The premain method should be queried for public modifier rather than
just relying on a failed invocation request.
David, thank you for catching this. I'm probably missing something here.
If the agent class is not public then the `m.canAccess(null)` check
is not passed and IAE is thrown with the message:
`Exception in thread "main" java.lang.IllegalAccessException:
method NonPublicAgent.premain must be declared public`
But the `NonPublicAgent.premain` is declared public as below:
public static void premain(String agentArgs, Instrumentation
inst) {
System.out.println("premain: NonPublicAgent was loaded");
}
It seems, the IAE is thrown because the agent class is not public.
Does it mean the `m.canAccess(null)` check is not fully correct?
canAccess will check both the type accessibility and the method
accessibility.
The premain class is not required to be public
(I think you meant "exported").
No I meant "public" in the context of the earlier issue that relaxed
that constraint.
The premain method must be public as this issue about. I expect the
agent module must export the package of the Premain-Class (or
Agent-Class) premain method (qualifiedly or unconditionally) for
java.instrument to access. The spec does not state clearly if the agent
class is public but as it requires the premain method public, it is
sensible to say it's accessible (otherwise, premain method does not have
to be public), i.e. public premain method in a public (accessible)
class). So in the modular world, the public method in a public class
in a package exported at least to java.instrument.
Are you expecting differently?
There are two issues:
1. What does the spec say about the premain class and the premain method
For the method it says it must be public - hence this issue.
For the class it says nothing and we know we've previously relaxed a
check that forced it to be public when not required by the spec.
2. What accessibility will allow the instrumentation code to actually
call the premain method?
In a modular world this likely means the premain class must be exported
somehow. That might mean it is public in an exported package, or perhaps
it may mean that the appropriate --add-opens has been added to the
command-line; or that it has been explicitly exported to allow access
from the instrumentation package. I'm not at all clear which checks in a
modular world can be circumvented by calling setAccessible(true), but
I'm assuming that at least some scenario can be handled by continuing to
use that. If after calling setAccessible(true) we still get an
IllegalAccessException for a public premain method then we can give some
general error message about the premain class needing to be accessible
to the instrumentation package.
Cheers,
David
-
The spec should clarify. FWIW. I just realized that
https://bugs.openjdk.java.net/browse/JDK-6932391 is not resolved.
so it may not be right to checks its accessibility as that may fail
even though the premain method is public and should be invoked. In
that regard I think the setAccessible call has to remain to deal with
this situation. So the process would be:
- load premain class via appLoader
- lookup premain method using getDeclaredMethod()
- check premain method is public and reject if not
- call setAccessible(true) in case premain class is not accessible
- do invoke
David
Mandy
Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]
On 12/15/20 5:50 PM, David Holmes wrote:
On 16/12/2020 11:35 am, Serguei Spitsyn wrote:
On Wed, 16 Dec 2020 00:50:04 GMT, David Holmes
wrote:
The agent class doesn't have to be public it just has to be accessible.
The premain method should be queried for public modifier rather than
just relying on a failed invocation request.
David, thank you for catching this. I'm probably missing something here.
If the agent class is not public then the `m.canAccess(null)` check
is not passed and IAE is thrown with the message:
`Exception in thread "main" java.lang.IllegalAccessException:
method NonPublicAgent.premain must be declared public`
But the `NonPublicAgent.premain` is declared public as below:
public static void premain(String agentArgs, Instrumentation
inst) {
System.out.println("premain: NonPublicAgent was loaded");
}
It seems, the IAE is thrown because the agent class is not public.
Does it mean the `m.canAccess(null)` check is not fully correct?
canAccess will check both the type accessibility and the method
accessibility.
The premain class is not required to be public
(I think you meant "exported").
The premain method must be public as this issue about. I expect the
agent module must export the package of the Premain-Class (or
Agent-Class) premain method (qualifiedly or unconditionally) for
java.instrument to access. The spec does not state clearly if the agent
class is public but as it requires the premain method public, it is
sensible to say it's accessible (otherwise, premain method does not have
to be public), i.e. public premain method in a public (accessible)
class). So in the modular world, the public method in a public class
in a package exported at least to java.instrument.
Are you expecting differently?
The spec should clarify. FWIW. I just realized that
https://bugs.openjdk.java.net/browse/JDK-6932391 is not resolved.
so it may not be right to checks its accessibility as that may fail
even though the premain method is public and should be invoked. In
that regard I think the setAccessible call has to remain to deal with
this situation. So the process would be:
- load premain class via appLoader
- lookup premain method using getDeclaredMethod()
- check premain method is public and reject if not
- call setAccessible(true) in case premain class is not accessible
- do invoke
David
Mandy
Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]
On Wed, 16 Dec 2020 01:32:53 GMT, Serguei Spitsyn wrote:
>> The agent class doesn't have to be public it just has to be accessible.
>>
>> The premain method should be queried for public modifier rather than just
>> relying on a failed invocation request.
>
> David, thank you for catching this. I'm probably missing something here.
> If the agent class is not public then the `m.canAccess(null)` check is not
> passed and IAE is thrown with the message:
> `Exception in thread "main" java.lang.IllegalAccessException: method
> NonPublicAgent.premain must be declared public`
>
> But the `NonPublicAgent.premain` is declared public as below:
> public static void premain(String agentArgs, Instrumentation inst) {
> System.out.println("premain: NonPublicAgent was loaded");
> }
> It seems, the IAE is thrown because the agent class is not public.
> Does it mean the `m.canAccess(null)` check is not fully correct?
Thanks, David!
Yes, I was also thinking that the setAccessible has to be remained after all
the checks.
-
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 [v4]
On 16/12/2020 11:35 am, Serguei Spitsyn wrote:
On Wed, 16 Dec 2020 00:50:04 GMT, David Holmes wrote:
The agent class doesn't have to be public it just has to be accessible.
The premain method should be queried for public modifier rather than just
relying on a failed invocation request.
David, thank you for catching this. I'm probably missing something here.
If the agent class is not public then the `m.canAccess(null)` check is not
passed and IAE is thrown with the message:
`Exception in thread "main" java.lang.IllegalAccessException: method
NonPublicAgent.premain must be declared public`
But the `NonPublicAgent.premain` is declared public as below:
public static void premain(String agentArgs, Instrumentation inst) {
System.out.println("premain: NonPublicAgent was loaded");
}
It seems, the IAE is thrown because the agent class is not public.
Does it mean the `m.canAccess(null)` check is not fully correct?
canAccess will check both the type accessibility and the method
accessibility.
The premain class is not required to be public so it may not be right to
checks its accessibility as that may fail even though the premain method
is public and should be invoked. In that regard I think the
setAccessible call has to remain to deal with this situation. So the
process would be:
- load premain class via appLoader
- lookup premain method using getDeclaredMethod()
- check premain method is public and reject if not
- call setAccessible(true) in case premain class is not accessible
- do invoke
David
-
-
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 [v4]
On Wed, 16 Dec 2020 00:50:04 GMT, David Holmes wrote:
>> Mandy, thank you for the suggestion.
>> I'll retarget the bug and CSR to jdk 17 as nobody is objecting.
>>
>> Also, wanted to make it clear about Exception messages that are provided for
>> two different cases.
>>
>> The test java/lang/instrument/NonPublicPremainAgent.java defines a
>> non-public premain method:
>> `static void premain(String agentArgs, Instrumentation inst) {`
>>
>> With the m.canAccess check the following message is provided (it looks
>> right):
>> ` Exception in thread "main" java.lang.IllegalAccessException: method
>> NonPublicPremainAgent.premain must be declared public `
>>
>> Without this check the message was (not sure, it is good enough):
>> ` Exception in thread "main" java.lang.IllegalAccessException: class
>> sun.instrument.InstrumentationImpl (in module java.instrument) cannot access
>> a member of class NonPublicPremainAgent with modifiers "static"`
>>
>> Also, I've added a new test java/lang/instrument/NonPublicAgent.java which
>> defines a non-public agent class with a public premain method.
>>
>> With the m.canAccess check the following message is provided (it does not
>> look right):
>> ` Exception in thread "main" java.lang.IllegalAccessException: method
>> NonPublicPremainAgent.premain must be declared public `
>>
>> Without this check the message is (it looks pretty confusing):
>> ` Exception in thread "main" java.lang.IllegalAccessException: class
>> sun.instrument.InstrumentationImpl (in module java.instrument) cannot access
>> a member of class NonPublicAgent with modifiers "public static"`
>>
>> So, it seems we also need an explicit check for agent class being public
>> with a right message provided.
>> I've added the following check:
>> @@ -426,6 +426,12 @@ public class InstrumentationImpl implements
>> Instrumentation {
>> NoSuchMethodException firstExc = null;
>> boolean twoArgAgent = false;
>>
>> +// reject non-public owner agent class of premain or agentmain
>> method
>> +if ((javaAgentClass.getModifiers() &
>> java.lang.reflect.Modifier.PUBLIC) == 0) {
>> +String msg = "agent class of method " + classname + "." +
>> methodname + " must be declared public";
>> +throw new IllegalAccessException(msg);
>> +}
>> +
>> // The agent class must have a premain or agentmain method that
>> // has 1 or 2 arguments. We check in the following order:
>> //
>>
>> With the check above the message is:
>> ` Exception in thread "main" java.lang.IllegalAccessException: agent class
>> of method NonPublicAgent.premain must be declared public`
>>
>> Please, let me know what you think.
>>
>> I've done some tests refactoring motivated by some private requests from
>> Mandy. Will publish the update as soon as it is ready. Hopefully, today.
>
> The agent class doesn't have to be public it just has to be accessible.
>
> The premain method should be queried for public modifier rather than just
> relying on a failed invocation request.
David, thank you for catching this. I'm probably missing something here.
If the agent class is not public then the `m.canAccess(null)` check is not
passed and IAE is thrown with the message:
`Exception in thread "main" java.lang.IllegalAccessException: method
NonPublicAgent.premain must be declared public`
But the `NonPublicAgent.premain` is declared public as below:
public static void premain(String agentArgs, Instrumentation inst) {
System.out.println("premain: NonPublicAgent was loaded");
}
It seems, the IAE is thrown because the agent class is not public.
Does it mean the `m.canAccess(null)` check is not fully correct?
-
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 [v4]
On Wed, 16 Dec 2020 00:15:36 GMT, Serguei Spitsyn wrote:
>> Alan, David and Many, thank you for the comments!
>> I'll prepare an update according to the recent requests from you.
>> One question is if I need to clone this PR to the JDK 16 fork:
>> https://github.com/openjdk/jdk16
>> It depends on our chances to finalize this before RDP2.
>> An alternate approach would be to continue reviewing this PR until all
>> comment are resolved and then clone it to jdk16.
>
> Mandy, thank you for the suggestion.
> I'll retarget the bug and CSR to jdk 17 as nobody is objecting.
>
> Also, wanted to make it clear about Exception messages that are provided for
> two different cases.
>
> The test java/lang/instrument/NonPublicPremainAgent.java defines a non-public
> premain method:
> `static void premain(String agentArgs, Instrumentation inst) {`
>
> With the m.canAccess check the following message is provided (it looks right):
> ` Exception in thread "main" java.lang.IllegalAccessException: method
> NonPublicPremainAgent.premain must be declared public `
>
> Without this check the message was (not sure, it is good enough):
> ` Exception in thread "main" java.lang.IllegalAccessException: class
> sun.instrument.InstrumentationImpl (in module java.instrument) cannot access
> a member of class NonPublicPremainAgent with modifiers "static"`
>
> Also, I've added a new test java/lang/instrument/NonPublicAgent.java which
> defines a non-public agent class with a public premain method.
>
> With the m.canAccess check the following message is provided (it does not
> look right):
> ` Exception in thread "main" java.lang.IllegalAccessException: method
> NonPublicPremainAgent.premain must be declared public `
>
> Without this check the message is (it looks pretty confusing):
> ` Exception in thread "main" java.lang.IllegalAccessException: class
> sun.instrument.InstrumentationImpl (in module java.instrument) cannot access
> a member of class NonPublicAgent with modifiers "public static"`
>
> So, it seems we also need an explicit check for agent class being public with
> a right message provided.
> I've added the following check:
> @@ -426,6 +426,12 @@ public class InstrumentationImpl implements
> Instrumentation {
> NoSuchMethodException firstExc = null;
> boolean twoArgAgent = false;
>
> +// reject non-public owner agent class of premain or agentmain method
> +if ((javaAgentClass.getModifiers() &
> java.lang.reflect.Modifier.PUBLIC) == 0) {
> +String msg = "agent class of method " + classname + "." +
> methodname + " must be declared public";
> +throw new IllegalAccessException(msg);
> +}
> +
> // The agent class must have a premain or agentmain method that
> // has 1 or 2 arguments. We check in the following order:
> //
>
> With the check above the message is:
> ` Exception in thread "main" java.lang.IllegalAccessException: agent class
> of method NonPublicAgent.premain must be declared public`
>
> Please, let me know what you think.
>
> I've done some tests refactoring motivated by some private requests from
> Mandy. Will publish the update as soon as it is ready. Hopefully, today.
The agent class doesn't have to be public it just has to be accessible.
The premain method should be queried for public modifier rather than just
relying on a failed invocation request.
-
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 [v4]
On Mon, 14 Dec 2020 22:45:26 GMT, Serguei Spitsyn wrote:
>> Changes requested by dholmes (Reviewer).
>
> Alan, David and Many, thank you for the comments!
> I'll prepare an update according to the recent requests from you.
> One question is if I need to clone this PR to the JDK 16 fork:
> https://github.com/openjdk/jdk16
> It depends on our chances to finalize this before RDP2.
> An alternate approach would be to continue reviewing this PR until all
> comment are resolved and then clone it to jdk16.
Mandy, thank you for the suggestion.
I'll retarget the bug and CSR to jdk 17 as nobody is objecting.
Also, wanted to make it clear about Exception messages that are provided for
two different cases.
The test java/lang/instrument/NonPublicPremainAgent.java defines a non-public
premain method:
`static void premain(String agentArgs, Instrumentation inst) {`
With the m.canAccess check the following message is provided (it looks right):
` Exception in thread "main" java.lang.IllegalAccessException: method
NonPublicPremainAgent.premain must be declared public `
Without this check the message was (not sure, it is good enough):
` Exception in thread "main" java.lang.IllegalAccessException: class
sun.instrument.InstrumentationImpl (in module java.instrument) cannot access a
member of class NonPublicPremainAgent with modifiers "static"`
Also, I've added a new test java/lang/instrument/NonPublicAgent.java which
defines a non-public agent class with a public premain method.
With the m.canAccess check the following message is provided (it does not look
right):
` Exception in thread "main" java.lang.IllegalAccessException: method
NonPublicPremainAgent.premain must be declared public `
Without this check the message is (it looks pretty confusing):
` Exception in thread "main" java.lang.IllegalAccessException: class
sun.instrument.InstrumentationImpl (in module java.instrument) cannot access a
member of class NonPublicAgent with modifiers "public static"`
So, it seems we also need an explicit check for agent class being public with a
right message provided.
Please, let me know what you think.
I've done some tests refactoring motivated by some private requests from Mandy.
Will publish the update as soon as it is ready. Hopefully, today.
-
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 [v4]
On 12/14/20 2:47 PM, Serguei Spitsyn wrote: On Mon, 14 Dec 2020 02:39:30 GMT, David Holmes wrote: Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: added 8165276 to @bug list of impacted tests Changes requested by dholmes (Reviewer). Alan, David and Many, thank you for the comments! I'll prepare an update according to the recent requests from you. One question is if I need to clone this PR to the JDK 16 fork: https://github.com/openjdk/jdk16 It depends on our chances to finalize this before RDP2. An alternate approach would be to continue reviewing this PR until all comment are resolved and then clone it to jdk16. Since this already passes RDP1, this can be retarget to JDK 17 and continue with this PR to openjdk/jdk master. I see no urgency to fix this in JDK 16. Mandy
Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]
On Mon, 14 Dec 2020 02:39:30 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> added 8165276 to @bug list of impacted tests > > Changes requested by dholmes (Reviewer). Alan, David and Many, thank you for the comments! I'll prepare an update according to the recent requests from you. One question is if I need to clone this PR to the JDK 16 fork: https://github.com/openjdk/jdk16 It depends on our chances to finalize this before RDP2. An alternate approach would be to continue reviewing this PR until all comment are resolved and then clone it to jdk16. - 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 [v4]
On Sat, 12 Dec 2020 09:48:27 GMT, Alan Bateman wrote: >> 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 .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. I think an explicit check for a public premain method is better to disambiguate the cases and provide appropriate error messages. - 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 [v4]
On Sat, 12 Dec 2020 01:29:28 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 8165276 to @bug list of impacted tests Changes requested by dholmes (Reviewer). - 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 [v4]
On Sat, 12 Dec 2020 01:29:28 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 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 .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
Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1694/files - new: https://git.openjdk.java.net/jdk/pull/1694/files/095d96b0..877ce70b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1694=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1694=02-03 Stats: 22 lines in 20 files changed: 1 ins; 4 del; 17 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
