15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs

[David Holmes]

Hi Serguei,

On 24/06/2020 3:37 pm, serguei.spitsyn at 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.

>>
>> 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.

This will have a much bigger compatibility problem if agents with a 
public premain suddenly stop working.

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.spitsyn at 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.spitsyn at 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
>>>
>>
>