Hi David,
On 6/23/20 22:50, David Holmes wrote:
Hi Serguei,
On 24/06/2020 3:37 pm, serguei.spit...@oracle.com wrote:
Hi Larry,
Thank you for looking at this!
On 6/23/20 21:32, Laurence Cable wrote:
should we not consider some form of depreciation here, and continue
to support non-public pre-main invocation for some time while
issuing a warning???
I'm not sure what form of deprecation we can use as it has to be a
deprecation of a spec non-compliant implementation. :)
There's obviously no TCK test for this. :)
You could just issue a warning if the premain is not public and say
this will be disallowed in a future release; then disallow it in 17.
But I'm not sure it's worth it.
Yes, it is not clear it is worth it.
while we have a sample of agents that will not be affected there may
be some agent that will fail terminally with this change
There is more important problem now.
A big number or j.l.instrument started to fail with my fix with
messages like this:
Exception in thread "main" java.lang.IllegalAccessException: class
sun.instrument.InstrumentationImpl
(in module java.instrument) cannot access a member of class
SimpleAgent with modifiers "public static"
It sounds like the use of setAccessible was hiding the need to disable
some module related access checks.
It sounds so.
In this particular case, the setAccessible was used just to disable some
module related access checks.
A side affect is that non-public premain methods became allowed as well.
This will have a much bigger compatibility problem if agents with a
public premain suddenly stop working.
One approach would be to continue using the setAccessible and add extra
check for non-public premain method.
Something like should probably work:
if (!(Modifier.isPublic(m.getModifiers())) {
throw new IllegalAccessException("premain method is not
public");
}
Thanks,
Serguei
David
-----
I'm not sure if there can be a version of the
Method.setAccessible(boolean flag) api that works for public methods
only.
One alternate approach is to relax the current spec to allow premain
methods to be non-public.
Thanks,
Serguei
just a thought
- Larry
On 6/23/20 8:42 PM, serguei.spit...@oracle.com wrote:
Hi Mandy,
Thank you for looking at this!
On 6/23/20 20:21, Mandy Chung wrote:
Hi Serguei,
I'm glad that you have a patch for this.
On 6/23/20 7:05 PM, serguei.spit...@oracle.com wrote:
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8165276
CSR draft (one CSR reviewer is needed before finalizing it):
https://bugs.openjdk.java.net/browse/JDK-8248189
The compatibility risk should be low (rather than minimal).
I was not sure if it has to be minimal or low.
Made it low now.
It says "All known Java agents define the premain method as
public". It'd be useful to add a comment in the JBS issue to list
the Java agents you have checked.
I'm relying on the Alan's comments posted in the bug report:
"I checked a number of popular java agents and their premain
methods are public, I haven't found any where the premain was not
public."
"I think we should just bite the bullet on this so that the
premain must be public as originally intended."
Probably, my statement in the CSR is too strong.
I've changed it to:
"No popular Java agent that defines the premain method as a
non-public was found."
Does it looks better or you think we have to investigate existing
popular Java agents?
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.1/
Looks okay. Can you add a test to verify this fix?
Yes, I can add a test but it will be trivial.
Thanks,
Serguei
Mandy
