RFR 8145964: NoClassDefFound error in transforming lambdas

[David Holmes]

Hi Coleen,

On 13/08/2016 4:19 AM, Coleen Phillimore wrote:
>
> Okay, I've made retransforming a class an unmodifiable class error and
> will give whoever complains your phone number.   I've run the change
> through the redefinition tests in our colocated (our closed),
> noncolocated (also oracle-only), jdk/test/java/lang/instrument and
> jdk/test/com/sun/jdi testsets.
>
> Please see new webrev.
>
> http://cr.openjdk.java.net/~coleenp/8145964.03/webrev/index.html

Thanks, this is my preferred solution.

Nit:

!   // Cannot redefine an anonymous class.  Retransform filters them out.

The second sentence  is not needed now.

I didn't fully grok the test as I'm not familiar the various pieces.


You need a second reviewer anyway so hopefully John and Tom will still 
chime in.

Thanks,
David

> Thanks,
> Coleen
>
> On 8/11/16 11:44 PM, David Holmes wrote:
>> On 12/08/2016 12:23 AM, Coleen Phillimore wrote:
>>> On 8/11/16 8:20 AM, David Holmes wrote:
>>>> On 11/08/2016 10:09 PM, Coleen Phillimore wrote:
>>>>>
>>>>>
>>>>> On 8/10/16 8:37 PM, David Holmes wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 11/08/2016 12:45 AM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>> New webrev:
>>>>>>>
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~coleenp/8145964.02/webrev/index.html
>>>>>>
>>>>>> First I'm very surprised that the existing logic in
>>>>>> JvmtiEnv::RetransformClasses doesn't utilize
>>>>>> VM_RedefineClasses::is_modifiable_class to determine when to return
>>>>>> JVMTI_ERROR_UNMODIFIABLE_CLASS. It would seem easy for the two
>>>>>> bits of
>>>>>> code to get out of sync!
>>>>>>
>>>>>> Second, why do you silently ignore an attempt to redefine an
>>>>>> anonymous
>>>>>> class instead of returning JVMTI_ERROR_UNMODIFIABLE_CLASS? The
>>>>>> restriction on transforming anonymous classes seems no different
>>>>>> to me
>>>>>> to the restriction on transforming primitive or array classes.
>>>>>
>>>>> I believe that there are existing applications, as in the test, get
>>>>> all
>>>>> the loaded classes and try to transform them.  These will get the VM
>>>>> anonymous class, so we didn't want to give them an error. Or set CLFH
>>>>> and vm anonymous classes fall into the load hook. There's another bug
>>>>> that Rachel has that ignores them for CFLH (rather than crashing).
>>>>>
>>>>> From the Java standpoint, the existence of vm anonymous classes are a
>>>>> implementation detail and not real classes, and it's better to hide
>>>>> these as much as possible.
>>>>
>>>> I think this is a mistake. While anonymous classes may have started as
>>>> some obscure VM implementation detail related to JSR-292 and
>>>> InvokeDynamic they are now entities that are prevalent in a running
>>>> Java application due to the extensive use of Methodhandles and their
>>>> use by lambda expressions. Unless we can hide such classes completely
>>>> (we can't!) we need to define their semantics when treated like other
>>>> "normal" Java classes. This should have been addressed for JVM TI when
>>>> anonymous classes came into existence but it wasn't. But we have to
>>>> deal with it, so simply define them as non-modifiable classes just
>>>> like array classes and primitives. If code gets an error that is good
>>>> it _needs_ to get an error because it needs to realize that it is
>>>> dealing with anonymous classes not "real" ones!
>>>>
>>>
>>> I disagree, and please read the bug where the consensus is that we
>>> should ignore these for retransformation.  Redefinition is a different
>>> matter because you have to give the class with new bytecodes, which
>>> would be very bad, and that's why I chose to give an error message.
>>
>> I don't read such a consensus in the bug report.
>>
>>> This is John Rose's answer to a similar question:
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/038353.html
>>>
>>
>> Yes I added that link to the bug report initially. It reinforces that
>> retransform/redefinition of VMAC is not intended to work  so don't do
>> that: "Please don't lead your users to rely on them."
>>
>> I read more in John's response that the code doing the transformations
>> should be skipping the VMAC's, than that the VM code should ignore the
>> VMAC's if passed in.
>>
>> The current situation is inconsistent and that is what I object to,
>> most strenuously. When I suggested this be handled through
>> VM_RedefineClasses::is_modifiable_class I was expecting the VMAC to be
>> treated as an unmodifiable class in all cases/sense. It is not
>> acceptable to me to have IsModifiableClass return false yet
>> retransform does not report JVMTI_ERROR_UNMODIFIABLE_CLASS. There
>> should be tests that check that the two are consistent!
>>
>> Please, either silently ignore it everywhere, or else deal with it
>> consistently as unmodifiable.
>>
>>> I am not willing to break existing applications.
>>
>> Any existing application that retransforms an anonymous class is
>> already in unchartered waters. To quote John "Even if you were able to
>> 'transform' one of these classfiles, it wouldn't necessary do what you
>> think it should do". So if it is lucky any existing code that attempts
>> to transform an anonymous class simply does no harm - but more likely
>> it gets an error (as per this bug report), or worse it "does something
>> you didn't think it would do"! If such code is now run against JDK 9
>> then I think they would be grateful to be told that they are running
>> broken code, and exactly where it is broken.
>>
>> Cheers,
>> David
>>
>>> Coleen
>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> Coleen
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Reran jvmti tests.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 8/10/16 9:32 AM, Coleen Phillimore wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/9/16 7:40 PM, David Holmes wrote:
>>>>>>>>> Hi Coleen,
>>>>>>>>>
>>>>>>>>> On 10/08/2016 4:52 AM, Coleen Phillimore wrote:
>>>>>>>>>> Summary: Skip VM anonymous classes in retransformation and
>>>>>>>>>> give an
>>>>>>>>>> error
>>>>>>>>>> for redefinition.
>>>>>>>>>>
>>>>>>>>>> Contributed by Tom Rodriguez.
>>>>>>>>>>
>>>>>>>>>> Tested with redefinition colocated tests (tonga) and
>>>>>>>>>> java/lang/instrument tests, and added test case.
>>>>>>>>>>
>>>>>>>>>> open webrev at
>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/8145964.01/webrev
>>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8145964
>>>>>>>>>
>>>>>>>>> Shouldn't anonymous classes be added to the set of non-modifiable
>>>>>>>>> classes - so IsModifiableClass returns false and we just add this
>>>>>>>>> case to VM_RedefineClasses::is_modifiable_class.
>>>>>>>>
>>>>>>>> Yes, that might be a better way to do it.
>>>>>>>> thanks,
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>>>
>>>>>>>>> ??
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Coleen
>>>>>>>>
>>>>>>>
>>>>>
>>>
>