RFC (csr): 8221528: Introduce compatibility mode with VM option -XX:AllowRedefinitionToAddOrDeleteMethods

[David Holmes]

On 28/03/2019 8:19 pm, Michael Rasmussen wrote:
> It was my assumption that the reason adding private methods were allowed with redefineClass was to support adding/removing lambdas from method bodies, since javac converts those into private methods.

IIUC it predates that and was added to support redefinition of native 
methods.

> Removing this capability would remove this support. I would assume that would cause users a lot of headache, since from a user's point-of-view, they are just changing the method body, which should be supported!

It will depend on how the user is generating the modified classfile 
bytes for the redefinition. And we certainly want to hear about such cases.

David
-----

> /Michael
> 
> 
> From: serviceability-dev <serviceability-dev-bounces at openjdk.java.net> on behalf of David Holmes <david.holmes at oracle.com>
> Sent: 28 March 2019 12:10
> To: Thomas Stüfe
> Cc: serviceability-dev at openjdk.java.net; Coleen Phillimore
> Subject: Re: RFC (csr): 8221528: Introduce compatibility mode with VM option -XX:AllowRedefinitionToAddOrDeleteMethods
>    
> 
> On 28/03/2019 4:55 pm, Thomas Stüfe wrote:
>> On Thu, Mar 28, 2019 at 8:40 AM David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>       Hi Thomas,
>>
>>       On 28/03/2019 5:02 pm, Thomas Stüfe wrote:
>>        > Hi Serguei,
>>        >
>>        > This is planned to be introduced in jdk 13?
>>        >
>>        > The only concern I have is that both paths (old and new behavior)
>>       should
>>        > be well tested, and hiding the old behavior behind an optional
>>       switch
>>        > may reduce its test coverage considerably.
>>
>>       The old behaviour is hardly ever used - I'm not even sure we currently
>>       have a test that tries to use it - so it's already not "well tested"
>>       and
>>       there's no intent here of increasing test coverage for the code we plan
>>       to remove. The new path will be tested as we do have a test that
>>       expects
>>       to get the error. And Serguei will of course have a simple test that
>>       checks the flag with both values.
>>
>>
>> I remember at least one case causing crashes in our code base where the
>> method ordering array was not reordered on RedefineClasses. May have
>> been this one: JDK-8149743.
>>
>> My point is, I have seen crashes in the field coming from bytecode
>> agents redefining classes with method added or substracted, so it is no
>> theoretical problem.
> 
> My point is that we don't really care if the code works well or not,
> we're intending to kill it off not improve it. It's a capability that
> really should never have snuck in to the code.
> 
> Cheers,
> David
> 
>> Cheers Thomas
>>
>>
>>       Cheers,
>>       David
>>
>>
>>        > Cheers, Thomas
>>        >
>>        > (p.s. html format does not work well with the OpenJDK mailing list
>>        > archive, see
>>        >
>>        https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027612.html)
>>        >
>>        > On Thu, Mar 28, 2019 at 1:57 AM serguei.spitsyn at oracle.com
>>       <mailto:serguei.spitsyn at oracle.com>
>>        > <mailto:serguei.spitsyn at oracle.com
>>       <mailto:serguei.spitsyn at oracle.com>> <serguei.spitsyn at oracle.com
>>       <mailto:serguei.spitsyn at oracle.com>
>>        > <mailto:serguei.spitsyn at oracle.com
>>       <mailto:serguei.spitsyn at oracle.com>>> wrote:
>>        >
>>        >     Hi All,
>>        >
>>        >     This is request for comments and potentially reviews for the
>>        >     following CSR:
>>        > https://bugs.openjdk.java.net/browse/JDK-8221528
>>        >
>>        >
>>        >     *Problem*
>>        >
>>        >     Now, the implementation of JVMTI RedefineClasses and
>>        >     RetransformClasses allows to
>>        >     add/delete private static and private final instance methods
>>       in the
>>        >     new class versions.
>>        >
>>        >     It means that current implementation does not follow the JVM
>>       TI spec
>>        >     which explicitly states:
>>        >
>>        >     "The redefinition must not add, remove or rename fields or
>>       methods,
>>        >     change the signatures
>>        >       of methods, change modifiers, or change inheritance."
>>        >
>>        >     For details, please, see the spec:
>>        >
>>        >
>>        https://docs.oracle.com/en/java/javase/12/docs/specs/jvmti.html#RedefineClasses
>>        >
>>        https://docs.oracle.com/en/java/javase/12/docs/specs/jvmti.html#RetransformClasses
>>        >
>>        >     The decision was made to align the implementation with the spec.
>>        >     For reference, please, see the
>>        > https://bugs.openjdk.java.net/browse/JDK-8192936.
>>        >
>>        >     This decision is going to cause some inconveniences to the
>>       customers.
>>        >
>>        >
>>        >     *Solution*
>>        >
>>        >     The solution is to introduce a compatibility mode with new
>>        >     command-line VM option:
>>        >         -XX:{+|-}AllowRedefinitionToAddOrDeleteMethods
>>        >
>>        >     New option will enable old behavior and allow the JVM TI
>>        >     RedefineClasses and RetransformClasses
>>        >     to add/delete private static and private final instance
>>       methods in
>>        >     the new class versions.
>>        >     Without this option the old behavior will be disabled.
>>        >
>>        >     New option is deprecated right away.
>>        >     The plan is to keep it for a couple of releases to allow
>>       customers
>>        >     (tool vendors)
>>        >     to remove dependency on old behavior from their tools.
>>        >
>>        >     Thanks,
>>        >     Serguei
>>        >
>>
>      
>